This project is archived and is in readonly mode.
ActiveSupport::Memoizable flush_cache is very slow
With one of my heftier models, I noticed that calling flush_cache or unmemoize_all was incredibly expensive. In the model was a method that would run some code and then flush the memoized cache. I had an array of 1000 objects on which I ran the method on each one and it ran for a long time. When I removed the cache clearing, the method ran almost immediately.
Essentially, calling methods + private_methods + protected_methods and then looping over all of them with a regex is overly expensive, when we already know what the memoized methods are when they are defined. By storing the known memoized methods, we can just loop over that much smaller array without a need for a regex.
Attached is a patch to speed up flush_cache and prime_cache (and memoize_all and unmemoize_all by extension). It is also available at http://github.com/Fryguy/rails/commit/4d1a36fb952bb63858657f7269bad...
Below is a Benchmark example run to show you my timings of unmemoize_all, where v is an instance of my model.
Before patch:
>> Benchmark.bmbm { |x| x.report { 100.times { v.unmemoize_all } } }
Rehearsal ------------------------------------
1.669000 0.015000 1.684000 ( 1.705682)
--------------------------- total: 1.684000sec
user system total real
1.419000 0.063000 1.482000 ( 1.520608)
After patch:
>> Benchmark.bmbm { |x| x.report { 100.times { v.unmemoize_all } } }
Rehearsal ------------------------------------
0.031000 0.000000 0.031000 ( 0.027010)
--------------------------- total: 0.031000sec
user system total real
0.000000 0.000000 0.000000 ( 0.010004)
-
Jason FreyAfter playing around with this patch locally for while, it seems there are some issues with it. Specifically, it doesn't handle methods that are included (even though somehow it passed the memoizable_test.rb). I made some changes locally to maybe try to get the methods from self.included_modules, but then I ran into issues with methods that are extended.
So, ignore the patch above, but the issue still stands. flush_cache is really slow since it has to call methods + private_methods + instanced_methods every time. My idea was to store the methods as an Array, which I think it still workable, but I haven't been able to do it just yet.
-
Jason Frey
- Tag changed from activesupport, memoizable to activesupport, memoizable, patch
Ok, round 2.
After more playing around with this, I realized the issue is not with calling methods + private_methods + protected_methods directly, but instead they are being called IN A LOOP of the parameters passed in. So, if you pass, for example, 20 method names to flush cache, it will call methods + private_methods + protected_methods 20 times. This is why I was seeing such horrible performance. flush_cache by itself wasn't the problem, it was flush_cache with parameters...the more the worse.
Attached is a patch that avoids that calling in a loop with much better results, plus all tests passing. Below is new benchmarks (MEMOIZED_METHODS is an array of 20 symbols):
Before patch:
>> Benchmark.realtime { 100.times { v.unmemoize_all }} => 1.62846398353577 >> Benchmark.realtime { 100.times { v.flush_cache *MEMOIZED_METHODS }} => 40.7326259613037After patch:
>> Benchmark.realtime { 100.times { v.unmemoize_all }} => 0.400160074234009 >> Benchmark.realtime { 100.times { v.flush_cache *MEMOIZED_METHODS }} => 0.0130050182342529 -
Jason Frey
Patch also available at http://github.com/Fryguy/rails/commit/9f223d7fc1e0374d86b3017eea915...
-
Santiago Pastorino- State changed from new to open
This issue has been automatically marked as stale because it has not been commented on for at least three months.
The resources of the Rails core team are limited, and so we are asking for your help. If you can still reproduce this error on the 3-0-stable branch or on master, please reply with all of the information you have about it and add "[state:open]" to your comment. This will reopen the ticket for review. Likewise, if you feel that this is a very important feature for Rails to include, please reply with your explanation so we can consider it.
Thank you for all your contributions, and we hope you will understand this step to focus our efforts where they are most helpful.
-
Santiago Pastorino
- State changed from open to stale