Fix wrong method lookup cache used when class is prepended #4581
+1
−1
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While benchmarking a @Talkdesk JRuby application during our recent
hackaton, I noticed quite a lot of thread contention in a workload where
there should be no shared data.
Stack traces showed that there was contention while performing writes to
the method lookup cache:
I ended up instrumenting the JRuby code and noticed that this kept
happening (on this workload) for calls to
Array.respond_to?(:to_ary)
which happened quite often and never seemed to be cached.
But why were threads repeatedly failing to lookup
#to_ary
, saving iton the cache, and then not finding it the next time around?
Every instance of the
RubyModule
class keeps a method lookup cache inthe
myCachedMethods
and normally reads and writes to it. Butadditionally, every
RubyModule
instance keeps a reference calledmethodLocation
to the object where it keeps its methods.Usually,
methodLocation
is set tothis
(e.g. theRubyModule
storesits own methods), but in some cases, e.g. when a class is prepended,
methodLocation
changes, and points elsewhere.The problem here is that when looking up on the
myCachedMethods
, thecacheHit
method always checks the localmyCachedMethods
reference,while the
addToCache
writes to the cache inside themethodLocation
reference. This means that normally the method cache works fine as
this.myCachedMethods
andthis.methodLocation.myCachedMethods
are thesame, but when
methodLocation
changes, the cache reads will never findthe correct cached method.
As a fix, I updated the
cacheHit
method to also read fromthis.methodLocation.myCachedMethods
.I believe this is enough to fix the issue (and is a correct fix), but feedback is
very welcome as I'm still rather noob in the codebase.
Thanks go to @Talkdesk for letting me work on making JRuby kick even
more ass.
Included below are my test cases and results from executing them.
Single-threaded performance impact test-case:
Before fix:
After fix:
Multi-threaded contention test case:
Thread state in VisualVM, without fix, without prepending
Array
:(All ok)
Thread state in VisualVM, without fix, prepending
Array
:(Heavy contention)
Thread state in VisualVM, with fix, prepending
Array
:(Back to normal)