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.

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.
good enough challenge for me to finally write some indy code ... but what can be done is doing the built-in check in a method-handle guard. I'm on it.

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 isBuiltin check completely - would need to observe changes to a RubyClass -> could dispatch to listeners with details every time a method is added/removed. what's the flaw in general with such an idea?

@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:

• Each CacheEntry, which is immutable, gets a new, valid SwitchPoint.
• When redefining a method, a search is done for that method name and any cached entry is invalidated.
• All uses of CacheEntry check that it is still valid before proceeding.

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).

@kares This is marked for 9.2.1. Can it be updated?

since NormalCachingCallSite is kind of a MonomorphicCachingCallSite
thus in that sense NormalCachingCallSite2 could become a BimorphicCachingCallSite
or a BiNormalCachingCallSite ... not sure if that sounds weird or not 😉

the class is an internal base class - naming is more of a reflection on having cache2 (along w cache)
so all 'duplicate' methods simply got the 2 suffix - haven't had a better idea wout refactoring NormalCachingCallSite extensively ... but I am open to suggestions

as promised ... here's some numbers: https://gist.github.com/kares/7188d075d614ed023ccad31cf6d968bb
for indy=true it might not be a "fair" comparison since when (Float/Fixnum) re-opened we always invalidate, but since its common e.g. in a Rails env or even when loading some stdlib (bigdecimal/util.rb) parts

@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).

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? isSecondaryBuiltin, retrieveSecondaryCache, cacheAndGetSecondary...maybe it's not too bad for code that's intended only to be used internally?

okay, PR feedback should be addressed by now,
will need to do a rebase due OracleJDK11 failing CI + a conflict resolution but otherwise hopefully all good ...