-
-
Notifications
You must be signed in to change notification settings - Fork 925
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
Fixnum/Float 'fast-ops' working even when re-opened #4736
Conversation
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.
The patch seems pretty straightforward. I guess you added a second cache so you wouldn't be blowing out the Fixnum cache with Float values? I'm unclear on that change but the rest is fine.
core/src/main/java/org/jruby/runtime/callsite/CachingCallSite.java
Outdated
Show resolved
Hide resolved
exactly seemed like it would make sense since we manage that call-site manually - we know when a Fixnum and when a Float class is to be expected (unlike if it were a generic - truly polymorphic cache). |
Seems ok to me then. I see there's no patch for indy, so that might be some additional work worth doing. |
thanks, will merge this one than ... will check what I can do about indy but no promises :) |
so I see what you've meant - the fixnum/float invalidators are still based on Fixnum/Float re-opening logic. btw. had a related/similar question on my mind for a while - it seems a bit more complicated but if we wanted to get rid of the |
@kares early on we tried that without much success (we called it active invalidation at the time). There are a couple of issues: 1) eager invalidation being extra execution cost for code never hit again 2) concurrency consistency (unless I lock down the world and one site updated but not the next one before it fires the wrong method version). Some inconsistency is just indeterminism of threading but I think this sort of invalidation goes beyond that. A second point for invokedynamic with invalidation is switchpoint invalidation is incredibly cheap. |
I did experiment with including a switchpoint inside each method object, so that if that method were ever overwritten we could do a one-shot invalidation of all sites that might have cached it. This would reduce the call site churn caused by invalidating a single class and having all methods cached from it get kicked out, even if they're still valid. I don't recall exactly what problems I ran into but I do remember that determining when to invalidate was tricky. You need to invalidate a method if any subclass overrides it, which meant all new methods had to walk up the hierarchy looking for the method they might be shadowing and then invalidate that method. This ended up being a very high startup cost even if it might save us walking down the hierarchy in the current model. It's something to consider, though...there may still be an efficient way to invalidate all call sites containing a given method without invalidating the whole class. |
I pushed something similar to my experiment from years ago to the active_invalidation branch. The strategy there works a bit differently than I described above:
This would eliminate the down-hierarchy search required to invalidate all descendant classes, replacing them with a usually-shorter and finer-grained search up the hierarchy for a single cache entry. It would also mean we are invalidating methods based both on the class they're in and the name of the method, reducing the impact on unrelated method names. However, it doesn't pass. I haven't dug into the failures. |
@headius you are using the same term but it is different that what @kares suggested and what I described about the past. This is what I would consider fine-grained inactive invalidation since cacheentry would only invalidate and replace upon getting called after the method has changed. Perhaps we need different vocabulary? method changing and notifying all sites (old days inactive) is probably better called eager. What we do today is lazy. What your branch does is method-grained and what we do today is type-grained. This last distinction because runtime-grained exists and no doubt other interesting subsets. Seems a fair description? |
@enebo Yes, that's not a bad description. My branch was an attempt to make invalidation work like it does today, but on a method slot basis. So only call sites that cache a method that gets replaced or overridden would invalidate. Granularity-wise, this is close to the finest grain we can invalidate. Where currently we invalidate only based on the serial number of the class (and method changes spin all serials below), this additionally separates invalidation based on the name and where the class and method are in the hierarchy (e.g. invalidating foo in a parent class doesn't need to invalidate children that never saw it). |
41b2879
to
e051cd5
Compare
@kares This is marked for 9.2.1. Can it be updated? |
0764282
to
ff52286
Compare
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.
The two level cache class and method names have bugged me. The 2 represents looking at second cache instead of 1 but it looks like 2 as in rb_new_ary2. Could the three of us maybe come up with another naming convention here?
TwoLevelCachingCallsite? isbuiltin2 is one I don't have any idea on...
since the class is an internal base class - naming is more of a reflection on having |
as promised ... here's some numbers: https://gist.github.com/kares/7188d075d614ed023ccad31cf6d968bb |
@kares re-naming...yeah I am having a hard time with a name here too. Perhaps @headius will get inspiration. 2 can be Bi or Two or Dual. The method itself is more challenging to me too :( @kares It is neat that math will be faster in Rails apps with this. I wonder if we do much math in ARJDBC to ever notice this? (not that it matters I just hope we see a positive effect there anyways). |
} | ||
|
||
public static boolean fixnumBooleanFail(ThreadContext context, IRubyObject caller, IRubyObject self, JRubyCallSite site, RubyFixnum value) throws Throwable { | ||
static boolean fixnumTestWithGen(IRubyObject self, RubyClass klass, final int gen) { | ||
return self instanceof RubyFixnum && klass.getGeneration() == gen; |
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.
This is a nice addition but ideally the indy way would be to use a switch point rather than a generation check. The generation check will always require traversing the class and the field, where the switch point is compiled as a safe point with basically zero overhead.
This is fine for now; we have continued to flip the generation since many other places can't use switch point, but the latter will optimize better.
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.
this was an addition on top of the method invalidation, as explained:
https://github.com/jruby/jruby/pull/4736/files#r297135999
we can get rid of it and see how it goes.
target = ((SwitchPoint)context.runtime.getFloatInvalidator().getData()) | ||
.guardWithTest(target, fallback); | ||
|
||
target = ((SwitchPoint) classFloat.getInvalidator().getData()).guardWithTest(target, fallback); |
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.
This is a bit confusing to me...why do we need to check both the SwitchPoint and the generation? They should both invalidate on class hierarchy changes.
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.
okay, will remove.
this is smt I wasn't sure about due the comment history where it was noted that the callback invalidation approach wasn't working in the past well. so I did the generation check as well as an attempt to invalidate methods "early" (besides the class-level method invalidator)
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.
No objections for merging. Only concern is with the use of both generation and switchpoint to invalidate the math operations. Unless I've missed some detail, I think only one should be required.
I have submitted a technical review. As for the naming...I'm with @enebo that "2" is not descriptive enough. I'm thinking along these lines: NormalCachingCallSite becomes common abstract parent to MonomorphicCallSite and BimorphiCallSite (caching seems irrelevant at this level) or goes away entirely (binary compat issue; perhaps it just becomes an empty class). MonomorphicCallSite keeps the methods named without any numbering, since those are definitely used inside and outside of JRuby. BimorphicCallSite methods are trickier. I would think of the second cache entry as "secondary" but maybe that's a little long? |
okay, PR feedback should be addressed by now, |
will make these take the desired route even with ActiveSupport loaded
instead of relying on
isFixnumReopened
and similarwe check at call-site whether the cached method
isBuiltin
while its slightly slower it does not degrade whenever the Fixnum/Float gets re-opened ...
... thus also works (fast-opt path being triggered) with active_support/core_ext being loaded
for polymorphic style behaviour (e.g. with
+
) deciding on Fixnum/Float argument there's now 2 caches used so it should play nicely (not invalidate) if a site does1+1
,1+1.0
,1+1
in a row ... let me know if its premature :)some numbers
... really just some quick "bmbm" runs to confirm its getting better and also not getting worse :)