Remove N+1 obj creation in flatten_arranged_rels #17325
Conversation
The recursive nature of the original code, this meant that a new array was created for every call of the method. This new form now only creates 2 arrays, and speeds up the process to flatten the rels by 2x in most cases.
NickLaMuro
force-pushed
the
flatten_arranged_rels_optimization
branch
from
1f91343 to
fcfd86d
Compare
|
Checked commit NickLaMuro@fcfd86d with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
|
@NickLaMuro Doesn't this end up modifying the caller (i.e. modifying the |
|
@Fryguy No (at least I don't think), because I am pretty sure both |
|
Yes, the |
|
@Fryguy okay, I see where it looks like it does that, but in reality, it doesn't pop it from the irb:001> hash = {:foo => [{:bar => []}, {:baz => [{:qux => []}]}]}
=> {:foo=>[{:bar=>[]}, {:baz=>[{:qux=>[]}]}]}
irb:002> remaining_children = hash.values
=> [[{:bar=>[]}, {:baz=>[{:qux=>[]}]}]]You will notice that the array that is created is a "doubly nested" at the top level. This means that when [{:bar=>[]}, {:baz=>[{:qux=>[]}]}]which is the [[]]So we are only mutating the |
|
I think another thing to note is that we are doing a |
|
This is good 👍 I think your example Hash there threw me a bit, because it's not accurate. So, I instrumented and got the following Those numbers are object_ids, and you can see that while the "real" values are being used directly as I expected, they are ultimately only being shuffled around, and not modified directly. As you said, the .pop is only called on the outer array only, which is a copy anyway. |
|
@Fryguy oh crap, you are right, the (would also explain why I was having issues when trying this in IRB...). Cool, glad it made sense and I wasn't mutating anything I shouldn't have. I was conscious of making sure I didn't do that when I wrote it, but kinda forgot how I ended up pulling it off after not looking at it for a week... Pretty sure I had some existing specs in there that failed when I did it wrong, but thanks for making sure I wasn't crazy and doing some due diligence. Very much appreciated! |
|
Added |
…ization Remove N+1 obj creation in flatten_arranged_rels (cherry picked from commit 32fadeb) https://bugzilla.redhat.com/show_bug.cgi?id=1593798
|
Fine backport details: |
…ization Remove N+1 obj creation in flatten_arranged_rels (cherry picked from commit 32fadeb) https://bugzilla.redhat.com/show_bug.cgi?id=1593797
|
Gaprindashvili backport details: |
The recursive nature of the original code, this meant that a new array was created for every call of the method.
This new form now only creates 2 arrays, and speeds up the process to flatten the rels by 2x.
Benchmarks
With 2.7k records
With 8.3k records