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
MINOR: Make CachingCallSite more JIT friendly #4753
Conversation
d78c100
to
4bef608
Compare
4bef608
to
a12e5f1
Compare
return typeOk(this, incomingType); | ||
} | ||
|
||
public static final boolean typeOk(CacheEntry entry, RubyClass incomingType) { |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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):
- Can I do anything besides running tests + full-text search to make sure I don't kill such a method accidentally?
- Am I correct to understand that the
@JIT
annotation should be put on methods which could be called dynamically like you described?
Thanks!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kares thanks!
... merging this piece so I do not have a conflict with my fast-opts call-site changes later |
Some improvements I found when profiling the red black benchmark ( + some trivialities along the way :)):
org.jruby.RubyModule#searchWithCache(java.lang.String, boolean)
into hot and cold path methods to decrease the size of its callerorg.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)
org.jruby.runtime.callsite.CacheEntry#typeOk
and removed itsstatic
version (thestatic
version really made no sense as it required having an instance to feed into it anyhow + the instance version compiles a little smaller)updateCache(entry);
to save a byteboolean cacheAndGetBuiltin(RubyClass selfType, String methodName)
static
since it contains no reference tothis
Trivial:
@override
, where missing to improve readabilityCachingCallingSite
final
to improve readability (I make JIT converge to the final state a little faster I guess :D)private
that were needlesslyprotected
Results:
2.2%
faster for me, measured over 1k iterations6.5M
instead of6.8M