[NO-TICKET] Optimize CodeProvenance#record_loaded_files to avoid allocations #3757
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
This PR replaces a
Hash#find
with aHash#any?
in the ProfilerCodeProvenance
collector.This seemingly innocuous change actually gets rid of a bunch of memory allocations.
It turns out that when
Hash#find
is being used, Ruby allocates an array to contain the[key, value]
pair that's passed into the block, for each entry in the hash (on top of a few more objects -- but this is the biggest offender unless the hash is empty/almost empty).The VM actually has an optimization for a lot of operations, including
Hash#any?
that avoids allocating any extra memory, but becauseHash#find
is actually inherited fromEnumerable
, this optimization does not kick in.Motivation:
Eliminate unneeded memory allocations.
Additional Notes:
Here's a very simple reproducer that shows the issue:
and here's how it looks on the latest Ruby version:
(Note that there's a few more allocations going on with
Hash#find
, not only the arrays for the pairs, and we get rid of them ALL!)Here's a quick comparison of the internal
prof-fibonacci
test service with/without this change (+ sampling every allocation):How to test the change?
This operation already has test coverage. I considered adding some counting of allocated objects, but we've had quite a bunch of flakiness in some of the profiler specs when counting objects, so I've decided to not add any performance-specific tests for this.