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

MINOR: Make CachingCallSite more JIT friendly #4753

Merged
merged 1 commit into from
Aug 29, 2017

Conversation

original-brownbear
Copy link
Contributor

@original-brownbear original-brownbear commented Aug 27, 2017

Some improvements I found when profiling the red black benchmark ( + some trivialities along the way :)):

  • split org.jruby.RubyModule#searchWithCache(java.lang.String, boolean) into hot and cold path methods to decrease the size of its caller org.jruby.runtime.callsite.CachingCallSite#cacheAndCall(org.jruby.runtime.builtin.IRubyObject, org.jruby.RubyClass, org.jruby.runtime.Block, org.jruby.runtime.ThreadContext, org.jruby.runtime.builtin.IRubyObject, org.jruby.runtime.builtin.IRubyObject, org.jruby.runtime.builtin.IRubyObject, org.jruby.runtime.builtin.IRubyObject)
    • Also removed redundant getter use in it
  • Reduced the indirection in org.jruby.runtime.callsite.CacheEntry#typeOk and removed its static version (the static version really made no sense as it required having an instance to feed into it anyhow + the instance version compiles a little smaller)
  • Removed the redundant setter updateCache(entry); to save a byte
  • Made boolean cacheAndGetBuiltin(RubyClass selfType, String methodName) static since it contains no reference to this

Trivial:

  • Added missing @override, where missing to improve readability
  • Made a few methods around the CachingCallingSite final to improve readability (I make JIT converge to the final state a little faster I guess :D)
  • Made a few methods private that were needlessly protected

Results:

  • Redblack benchmark becomes 2.2% faster for me, measured over 1k iterations
  • Codecache size for the red black benchmark converges to 6.5M instead of 6.8M

@original-brownbear original-brownbear force-pushed the cachingcallsite-improvements branch from d78c100 to 4bef608 Compare August 27, 2017 13:58
@original-brownbear original-brownbear force-pushed the cachingcallsite-improvements branch from 4bef608 to a12e5f1 Compare August 27, 2017 14:04
return typeOk(this, incomingType);
}

public static final boolean typeOk(CacheEntry entry, RubyClass incomingType) {
Copy link
Member

Choose a reason for hiding this comment

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

you need to careful when removing methods on places such as these.
might be called from JIT-ed byte-code (so you're not seeing it show up as used)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kares yea, I learnt that today in some other spot already :) Questions (if you have a sec):

  1. Can I do anything besides running tests + full-text search to make sure I don't kill such a method accidentally?
  2. Am I correct to understand that the @JIT annotation should be put on methods which could be called dynamically like you described?

Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Can I do anything besides running tests + full-text search to make sure I don't kill such a method accidentally?

usually just grep for the method-name string -> "typeOf" should be safe to find whether its used :)

Am I correct to understand that the @jit annotation should be put on methods which could be called dynamically like you described?

well yeah - have run into that as well ... lately.
but what I dislike is that now some places might have a dependency on the IR package (org.jruby.ir.JIT) -> should maybe get relocated somewhere else I guess ... not sure
haven't raised that concern yet -> you should chat with @enebo or @headius on IRC if it was meant to be somehow IR specific or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kares thanks!

@kares
Copy link
Member

kares commented Aug 29, 2017

... merging this piece so I do not have a conflict with my fast-opts call-site changes later

@kares kares merged commit 68d9779 into jruby:master Aug 29, 2017
@kares kares added this to the JRuby 9.2.0.0 milestone Aug 29, 2017
@original-brownbear original-brownbear deleted the cachingcallsite-improvements branch August 29, 2017 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants