Reduce String allocations in ActionController::Parameters#permitted_scal... #19037
Conversation
| @@ -537,7 +537,10 @@ def permitted_scalar_filter(params, key) | |||
| params[key] = self[key] | |||
| end | |||
|
|
|||
| keys.grep(/\A#{Regexp.escape(key)}\(\d+[if]?\)\z/) do |k| | |||
| @@permitted_scalar_filter_regexps ||= {} | |||
| @@permitted_scalar_filter_regexps[key] ||= /\A#{Regexp.escape(key)}\(\d+[if]?\)\z/ | |||
There was a problem hiding this comment.
Not sure if there is a better way to memoize this, I'm open to suggestions.
There was a problem hiding this comment.
Won't storing it in a class variable make Parameters unsafe for multiple threads?
|
I can never understand the output of What kind of a performance boost does these reduced allocations give us? 😄 |
|
@tgxworld can you post the top 5 for after no just the ones still related to strong_parameters. Can you additionally run the StackProf and tell us what the garbage collection was before and after your change? Sometimes the allocation tracer isn't correct. If you run it a couple more times the number of allocations can change drastically. @kaspth this is showing that the allocations to the regex and string are no longer in the top 5 allocations. This doesn't have performance gains as much as we are trying to reduce the amount of time that integration tests spend in the garbage collector. Previously we were spending 13% of our time there. |
|
@eileencodes Got it 👍 |
|
Before top 5 allocations: After top 5 allocations: Also, how were you able to measure the time spent in GC consistently? StackProf varies every run. I've gotten results from 6% to 8%.
|
|
You should use |
|
Can we figure out a way to do this without class variables? I don't really want to add more caches if we can avoid it. |
…calar_filter. Script used: https://github.com/eileencodes/integration_performance_test/blob/master/test/integration/documents_allocations_create_test.rb. Before: ``` [["rails/actionpack/lib/action_controller/metal/strong_parameters.rb", 540, :T_STRING], [52820, 0, 52406, 0, 2, 5676390]] [["rails/actionpack/lib/action_controller/metal/strong_parameters.rb", 540, :T_NODE], [17448, 0, 17318, 0, 2, 675600]] [["rails/actionpack/lib/action_controller/metal/strong_parameters.rb", 540, :T_ARRAY], [17448, 0, 17321, 0, 2, 675600]] [["rails/actionpack/lib/action_controller/metal/strong_parameters.rb", 540, :T_REGEXP], [5816, 0, 5774, 0, 2, 3777730]] ``` After: ``` [["rails/actionpack/lib/action_controller/metal/strong_parameters.rb", 543, :T_NODE], [18006, 0, 17503, 0, 2, 696240]] [["rails/actionpack/lib/action_controller/metal/strong_parameters.rb", 543, :T_ARRAY], [12004, 0, 11670, 0, 2, 464160]] ```
tgxworld
force-pushed
the
reduce_string_allocation_in_ac_parameters
branch
from
f007b3b
to
a6caa7a
Compare
|
@eileencodes May I know how you determine if we're spending less time in GC? I used stackprof but the percentage of time spent in GC varies quite a bit giving inconsistent results. Since we're allocating 50k lesser Strings, I was expecting to see an improvement since the other 'reducing allocations' pull requests normally reduces them in the range of 10k. Thanks in advance! |
|
@tgxworld I need to look at this more closely but am strapped for time this week. I'll try to get a look at it on Thursday but it probably won't be before then. 😄 As for the GC it depends; one other way we determined that there was a change was looking at total allocated objects. The problem with the allocations that we didn't realize before is it changes a lot if GC runs but then maybe on subsequent runs there's not change. We're still figuring out the best method of measurement overall. Aaron and I saw a drop in GC that didn't vary more than 0.03-0.07% with our last change. I'm sorry that's not very clear 😞 |
No rush, whenever you're free 😄
Thanks for clearing things up! I did use |
|
I decided to see where the strings were being allocated. Just by interpolating within If we were to use Switching to The best way I can think of right now is still to cache the permitted |
|
Closing this. Not sure how I can reduce allocations here without caching. |
...ar_filter.
Script used: https://github.com/eileencodes/integration_performance_test/blob/master/test/integration/documents_allocations_create_test.rb.
Before:
After:
cc/ @eileencodes @tenderlove