Use arity hashCode instead of proc#hash in CallableSelector#4494
Merged
kares merged 1 commit intojruby:masterfrom Feb 19, 2017
Merged
Use arity hashCode instead of proc#hash in CallableSelector#4494kares merged 1 commit intojruby:masterfrom
kares merged 1 commit intojruby:masterfrom
Conversation
Using proc#hash as the cache key means that you're only guaranteed to get a cache hit if exactly the same proc is being passed. Using arity.hashCode instead should satisfy the original reason for hashing the proc (disambiguate signatures based on the arity of the proc) while preventing cache misses. Also note that this was causing a memory leak: executing the code that relies on this caching would create a new cache entry on every invocation, slowly growing the size of the cache.
Member
|
thanks @snowp ... vaguely recall this logic. |
Contributor
Author
|
I can look into adding a test to verify that the correct Java method gets called, but not sure how to test the cache growth? Here's some code I used to demonstrate the leak: causes: |
Contributor
Author
|
actually, looks like the method resolution is already tested in integration_spec.rb:213. i don't expect my change to cause any functional change, so not sure if adding more functional tests would be helpful? |
Member
|
you're right - looked closer and only the arity matters at that ( |
kares
added a commit
that referenced
this pull request
Feb 19, 2017
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Using proc#hash as the cache key means that you're only
guaranteed to get a cache hit if exactly the same proc is
being passed. Using arity.hashCode instead should satisfy the
original reason for hashing the proc (disambiguate signatures
based on the arity of the proc) while preventing cache misses.
Also note that this was causing a memory leak: each time we get a cache miss it creates a new cache entry, and the cache is unbounded.
Tried to add a test for this, but it doesn't seem like CallableSelectorTest runs / works.