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

Faster MixedModeIRMethod#call #4786

Merged
merged 1 commit into from Sep 17, 2017
Merged

Conversation

@original-brownbear
Copy link
Contributor

@original-brownbear original-brownbear commented Sep 9, 2017

One performance improvement and one potential bugfix here:

Performance improvement:

  • DynamicMethodBox adds a needless level of indirection. Moved the fields directly into MixedModeIRMethod
  • Stopped making a copy of actualMethod for null check and invocation. I think this is fine, there is no place in the code that could possibly set a null here as far as I can see, right?

Bugfix:

  • volatile int callCount is not safe for use as a counter here I think. It is incremented in a synchronized block (box.callCount++) in the jit method, but can also be set without synchronization from setCallCount(int callCount).
    • Fixed by synchronizing all write access to callCount

With indy on, this gets me from 0.055 ops/ns (master) to 0.061ops/ns (this branch) for the JMH org.jruby.benchmark.JavaInterfaceBenchmark#benchHalfRubyVersion :)


if (jittedMethod != null) {
return jittedMethod.call(context, self, clazz, name, args, block);
if (actualMethod != null) {
Copy link
Member

@kares kares Sep 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

used to be one volatile field read, now its 2 right?
not sure it matters - but I think Java 8 still doesn't re-arrange volatile reads

Loading

Copy link
Contributor Author

@original-brownbear original-brownbear Sep 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kares you're right, even though this isn't really contended it's still better to copy. Sec adding the copy back :)

Loading

Copy link
Contributor Author

@original-brownbear original-brownbear Sep 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed :)

Loading

@kares
Copy link
Member

@kares kares commented Sep 17, 2017

looking good, always wondered whether we actually need to care about volatile semantics around call-count
they're mostly to be reached from a single-thread and if they aren't exact well it does not matter much ...

Loading

@kares kares merged commit c07da80 into jruby:master Sep 17, 2017
1 check failed
Loading
@original-brownbear original-brownbear deleted the faster-method-ir branch Sep 17, 2017
@original-brownbear
Copy link
Contributor Author

@original-brownbear original-brownbear commented Sep 17, 2017

@kares yea I think volatile on the call count doesn't get us much except keeping the count exact.
Also, I wonder we couldn't just use AtomicBoolean's compareAndSet to guard against compiling multiple times. Might improve startup times to not have a hard synchronization in tryJit (though it could lead to other concurrency issues that I just don't see?).

Loading

@enebo enebo added this to the JRuby 9.1.14.0 milestone Nov 8, 2017
@enebo enebo removed this from the JRuby 9.1.14.0 milestone Nov 8, 2017
@enebo enebo added this to the JRuby 9.2.0.0 milestone Nov 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants