Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use arity hashCode instead of proc#hash in CallableSelector #4494

Merged
merged 1 commit into from Feb 19, 2017

Conversation

@snowp
Copy link
Contributor

@snowp snowp commented Feb 16, 2017

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.

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.
@kares
Copy link
Member

@kares kares commented Feb 17, 2017

thanks @snowp ... vaguely recall this logic.
not sure what happens when the same args are passed but the last argument (proc/block) changes.
we should be able to test things out with a proc-to-iface test/spec ...
(either spec/java_integration/* or test/jruby/test_*java*.rb)

@snowp
Copy link
Contributor Author

@snowp snowp commented Feb 17, 2017

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:

package com.test; 

public class ProcTest {
  void foo(int x, Predicate<Integer> pred) {}
  void foo(int x, int y, Predicate<Integer> pred) {}
}
pt = com.test.ProcTest.new; loop { pt.foo(0) { } }

causes:

Java::JavaLang::OutOfMemoryError: Java heap space
	from org.jruby.util.collections.NonBlockingHashMapLong$CHM.<init>(NonBlockingHashMapLong.java:514)
	from org.jruby.util.collections.NonBlockingHashMapLong$CHM.resize(NonBlockingHashMapLong.java:788)
	from org.jruby.util.collections.NonBlockingHashMapLong$CHM.putIfMatch(NonBlockingHashMapLong.java:651)
	from org.jruby.util.collections.NonBlockingHashMapLong$CHM.access$000(NonBlockingHashMapLong.java:419)
	from org.jruby.util.collections.NonBlockingHashMapLong.putIfMatch(NonBlockingHashMapLong.java:335)
	from org.jruby.util.collections.NonBlockingHashMapLong.put(NonBlockingHashMapLong.java:291)
	from org.jruby.java.invokers.RubyToJavaInvoker.putSignature(RubyToJavaInvoker.java:240)
	from org.jruby.java.dispatch.CallableSelector.matchingCallableArityTwo(CallableSelector.java:126)
	from org.jruby.java.invokers.RubyToJavaInvoker.findCallableArityTwo(RubyToJavaInvoker.java:454)
	from org.jruby.java.invokers.InstanceMethodInvoker.call(InstanceMethodInvoker.java:111)
	from org.jruby.runtime.callsite.CachingCallSite.callBlock(CachingCallSite.java:171)
	from org.jruby.runtime.callsite.CachingCallSite.call(CachingCallSite.java:177)
	from org.jruby.ir.interpreter.InterpreterEngine.processCall(InterpreterEngine.java:333)
	from org.jruby.ir.interpreter.InterpreterEngine.interpret(InterpreterEngine.java:159)
	from org.jruby.ir.interpreter.Interpreter.INTERPRET_BLOCK(Interpreter.java:132)
	from org.jruby.runtime.InterpretedIRBlockBody.yieldDirect(InterpretedIRBlockBody.java:109)
	from org.jruby.runtime.IRBlockBody.yieldSpecific(IRBlockBody.java:76)
	from org.jruby.runtime.Block.yieldSpecific(Block.java:136)
	from org.jruby.RubyKernel.loop(RubyKernel.java:1298)
	from org.jruby.RubyKernel$INVOKER$s$0$0$loop.call(RubyKernel$INVOKER$s$0$0$loop.gen)
	from org.jruby.internal.runtime.methods.JavaMethod$JavaMethodZeroBlock.call(JavaMethod.java:497)
	from org.jruby.runtime.callsite.CachingCallSite.cacheAndCall(CachingCallSite.java:298)
	from org.jruby.runtime.callsite.CachingCallSite.callBlock(CachingCallSite.java:79)
	from org.jruby.runtime.callsite.CachingCallSite.call(CachingCallSite.java:83)
	from org.jruby.ir.instructions.CallBase.interpret(CallBase.java:428)
	from org.jruby.ir.interpreter.InterpreterEngine.processCall(InterpreterEngine.java:356)
	from org.jruby.ir.interpreter.StartupInterpreterEngine.interpret(StartupInterpreterEngine.java:73)
	from org.jruby.ir.interpreter.Interpreter.INTERPRET_EVAL(Interpreter.java:122)
	from org.jruby.ir.interpreter.Interpreter.evalCommon(Interpreter.java:176)
	from org.jruby.ir.interpreter.Interpreter.evalWithBinding(Interpreter.java:200)
	from org.jruby.RubyKernel.evalCommon(RubyKernel.java:1033)
	from org.jruby.RubyKernel.eval19(RubyKernel.java:1000)
@snowp
Copy link
Contributor Author

@snowp snowp commented Feb 17, 2017

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?

@kares kares merged commit b0b1d52 into jruby:master Feb 19, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kares kares added this to the JRuby 9.1.8.0 milestone Feb 19, 2017
@kares
Copy link
Member

@kares kares commented Feb 19, 2017

you're right - looked closer and only the arity matters at that (CallableSelector) point.
... will try to add a regression (leak) spec like the one you mentioned above. thanks!

kares added a commit that referenced this pull request Feb 19, 2017
@kares kares mentioned this pull request Mar 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.