Reduce memory usage of .store_path
#54
Conversation
NickLaMuro
force-pushed
the
reduce_store_path_memory_usage
branch
from
d6ec119 to
7ecd458
Compare
|
Conceptually, this looks good to me. One thing that concerns me is that I don't think we have any specs for when Hashes and Arrays are intermingled, for example, you can do this: h = {:a => [{:a1 => nil}, {:a2 => nil}]}
h.store_path(:a, 0, :a1, "blah")
# => {:a=>[{:a1=>"blah"}, {:a2=>nil}]}Can you verify if we have specs covering that and if not, can you add them? |
|
cc @bdunne |
Pretty sure we do, as I was using the existing specs to drive these changes, and it definitely took me a bit to get there, but I will confirm since I am only basing it on what I remember while working on this until midnight last night 😅 |
|
@Fryguy Looks like you are correct in your assumption. I will add the test as I agree it is very much needed. That said, curious if you think this should live in a separate test file just for |
|
I was actually surprised to find that the existing tests were not already in a spec/shared/nested_spec.rb. Perhaps they should all really live there, but I'm not sure. I'd also be fine with just adding the "cross concerns" specs to a spec/shared/nested_spec.rb |
|
@Fryguy Thanks for the suggestion, currently working on the specs for this now. That said, ran into something that probably is an edge case that hasn't been considered, and isn't really tested yet, and I am curious of how we want to handle it moving forward. Part of irb> [{"a" => [1,2,3]}].store_path(0, "b", 0, 4)
#=> [{"a" => [1,2,3], "b" => {0 => 4}]Because it is implemented with recursion, so the class will change as it goes down the stack. But with my current implementation, irb> [{"a" => [1,2,3]}].store_path(0, "b", 0, 4)
#=> [{"a" => [1,2,3], "b" => [4]}]I can change this behavior in this PR to match what currently in master, but those aren't tested, and I am honestly not sure which is the true/correct intent. |
|
That's so funny...I was just about to post here about the class.new call, and that it doesn't use the type of the object at that nesting. |
|
I was aware that it used the type at the nesting. If I recall, part of the reason is we had things like Arrays of Hashes, and HWIA stored inside of regular Hashes. |
| if args.length > 1 || args.first.kind_of?(Array) | ||
| keys = args.first.kind_of?(Array) ? args.first : args | ||
| keys.each_with_index do |cur_key, i| | ||
| raise ArgumentError, "must be a number" if kind_of?(Array) && !cur_key.kind_of?(Numeric) |
There was a problem hiding this comment.
Just noticed this while writing some specs for mixed groups, and this needs to be if child.kind_of?(Arrray) ... or it won't function properly. I will make this change.
Not sure if this was a suggestion to do this, but regardless, I am going to make the call say we should stick with matching the implementation that currently exists, and reason about whether or not that implementation is a bug in a separate issue/PR. Should have been what I suggested in the first place since the intent of this PR is strictly improving performance, so sorry for the extra noise by trying to reason about that here. |
NickLaMuro
force-pushed
the
reduce_store_path_memory_usage
branch
from
7ecd458 to
780c30f
Compare
|
Specs added in #56 |
NickLaMuro
force-pushed
the
reduce_store_path_memory_usage
branch
from
780c30f to
8cc6a2a
Compare
By using recursion to loop through the keys for store path, we are creating new objects that are not needed every time with go further down into the stack, because we are required to do a `args[1..-1].push`, which will create a new array from the subset of the args, and pass it into the next call of `.store_path`. By not using recursion, we can completely remove any object allocations that aren't necessary (besides the ones that are needed for the method's spec), and still maintain the same functionality. The added benefit is that this also now not prone to `stack level too deep` errors because it no longer requires recursion to function.
|
Checked commit NickLaMuro@8cc6a2a with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
|
🎉 |
Background
By using recursion to loop through the keys for store path, we are creating new objects that are not needed (for the end result of the method) every time with go further down into the stack, because we are required to do a
args[1..-1].pushfor each recursive call, which will create a new array from the subset of the args, and pass it into the next call of.store_path.By not using recursion, we can completely remove any object allocations that aren't necessary (besides the ones that are needed for the method's spec), and still maintain the same functionality. The added benefit is that this also now not prone to
stack level too deeperrors because it no longer requires recursion to function.Benchmarks
Using the changes from ManageIQ/manageiq#15757, and some of the data found after the introduction of @chrisarcand 's PR, ManageIQ/manageiq#15710 , which uses
store_pathas part of changes that weren't present prior, we can significantly reduce the number of objects allocated in that introduced because of that change (and probably reduce other cases where.store_pathis used in the application as well:.fetch_pathhas a similar issue (I might address that in another PR), which is why there is still a decent amount still being used by it, but this significantly reduces what was being allocated by the gem overall (in this benchmark).Links