Reduce allocations when extracting AR models #12185
Conversation
| # Used to be: Hash[column_names_with_alias.map{|cn, an| [cn, row[an]]}] | ||
| # that is fairly inefficient cause all the values are first copied | ||
| # in to an array only to construct the Hash | ||
| # This code is performance critical as it is called per row. |
There was a problem hiding this comment.
I wonder if this comment is better as just this final line. We can get the history via git.
There was a problem hiding this comment.
The rest of the comment is useful though. Would possibly be better to either link to the PR, or add the information into the commit message?
Something like:
Reduce allocations when extracting AR models
Replace inefficient idiomatic code as all the values are first copied
in to an array only to construct the hash. This code is performance
critical as it is called per row
There was a problem hiding this comment.
You could also use something like:
column_names_with_alias.each_with_object({}) do |(cn, an), hash|
hash[cn]= row[an]
endwhich is more idiomatic but avoids the array allocations.
There was a problem hiding this comment.
@steveklabnik updated the comment to point back to the PR :)
|
This strikes me as generic and a candidate for extraction to |
|
@baroquebobcat I micro optimised here cause its a per-row thing, definitely not something that should be added willy nilly unless there is a clear hotspot. I think mapping the data from the db back to AR objects is a definite hotspot we need to be pretty diligent with. |
| hash = {} | ||
|
|
||
| index = 0 | ||
| while index < column_names_with_alias.length do |
There was a problem hiding this comment.
Would move length to a variable cause any perf impact? Also I believe Rails conventions are mostly not to use do with while, similer to not using then with if.
Always great stuff @SamSaffron 👍
There was a problem hiding this comment.
thanks, fixing that up. doing the length upfront has a very marginal perf improvement, however we are micro-optimising here so we might as well go the whole way
|
Does this PR needed any Changelog entries? |
Reduce allocations when extracting AR models
|
❤️ |
Reduce allocations when extracting AR models
* '4-0-stable' of github.com:rails/rails: Merge pull request #12135 from dylanahsmith/avoid_empty_transaction Merge pull request #12185 from SamSaffron/join_dep Merge pull request #12194 from thedarkone/readonly-merger-fix Merge pull request #12148 from gzohari/callback-typo Merge pull request #12143 from rajcybage/fixing_typos Merge pull request #12139 from vipulnsward/typos_av Merge pull request #12130 from egilburg/patch-1
Similar to: #12065
Reduce the allocations when extracting rows from a join dependency
For a typical home page request at Discourse this reduced the objects allocated by 2000 and the memory allocated by 100k
(analyzed using https://github.com/SamSaffron/memory_profiler )
This is extremely safe and can be backported to rails 4 , cc @tenderlove