RFR 8191890: Biased locking still uses the inferior stop the world safepoint for revocation
Patricio Chilano
patricio.chilano.mateo at oracle.com
Fri Jun 21 16:27:10 UTC 2019
Hi David,
On 6/21/19 11:55 AM, David Holmes wrote:
> On 21/06/2019 7:49 am, Patricio Chilano wrote:
>> Hi David,
>>
>> On 6/20/19 9:41 PM, David Holmes wrote:
>>> Hi Patricio,
>>>
>>> On 18/06/2019 10:08 pm, Patricio Chilano wrote:
>>>> Hi all,
>>>>
>>>> Here is v03, it contains the fixes suggested by Markus and Coleen:
>>>>
>>>> Full: http://cr.openjdk.java.net/~pchilanomate/8191890/v03/webrev/
>>>> <http://cr.openjdk.java.net/%7Epchilanomate/8191890/v03/webrev/>
>>>> Inc:
>>>> http://cr.openjdk.java.net/~pchilanomate/8191890/v03/inc/webrev/
>>>> <http://cr.openjdk.java.net/%7Epchilanomate/8191890/v03/inc/webrev/>
>>>>
>>>> The only extra change that I made was moving the clean of the cache
>>>> and the declaration of ResourceMark outside of
>>>> walk_stack_and_revoke(), since that was preventing the optimization
>>>> of using the cache when revoking with revoke(GrowableArray<Handle>*
>>>> objs, JavaThread *).
>>>
>>> I don't quite follow how the ResourceMark placement relates to that.
>>> If walk_stack_and_revoke contains code that needs the RM then surely
>>> better to place it where needed rather than callers having to
>>> remember to use a RM themselves.
>> The issue of leaving the ResourceMark in walk_stack_and_revoke() is
>> that the array created by get_or_compute_monitor_info() would be
>> destroyed upon return, so to avoid a new call to
>> get_or_compute_monitor_info() returning the old array we would be
>> forced to clear the cache.
>> I agree that from a user's perspective it would be better to place it
>> in walk_stack_and_revoke(), but also since it's a private method it
>> will not be called outside of BiasedLocking class. Also calling
>> walk_stack_and_revoke() without a ResourceMark would trigger an
>> assertion so it would be easy to spot. How about if I add the comment
>> "Caller should have instantiated a ResourceMark object before calling
>> this method" on top of walk_stack_and_revoke() ?
> Ok.
Ok, not sure if you needed to see another webrev but just in case here
it is:
Inc: http://cr.openjdk.java.net/~pchilanomate/8191890/v04/inc/webrev/
<http://cr.openjdk.java.net/%7Epchilanomate/8191890/v04/inc/webrev/>
Full: http://cr.openjdk.java.net/~pchilanomate/8191890/v04/webrev/
<http://cr.openjdk.java.net/%7Epchilanomate/8191890/v04/webrev/>
Thanks!
Patricio
> Thanks,
> David
>
>>
>> Thanks,
>> Patricio
>>> Thanks,
>>> David
>>>
>>>> Coleen: I didn't simplified methods revoke(GrowableArray<Handle>*
>>>> objs...) and revoke_at_safepoint(GrowableArray<Handle>* objs) after
>>>> all. I tried to do it by replacing them with a single method that
>>>> would mostly call walk_stack_and_revoke() for revocations, but that
>>>> causes problems similar to 8225351, since the passed array might
>>>> contain an object not biased towards the expected JavaThread.
>>>>
>>>> Tested with mach5 tiers1-6.
>>>>
>>>> Thanks!
>>>>
>>>> Patricio
>>>>
>>>> On 6/17/19 6:55 PM, Patricio Chilano wrote:
>>>>> Hi Coleen,
>>>>>
>>>>> On 6/17/19 4:55 PM, coleen.phillimore at oracle.com wrote:
>>>>>>
>>>>>> On 6/17/19 2:14 PM, Patricio Chilano wrote:
>>>>>>> Hi Coleen,
>>>>>>>
>>>>>>> On 6/14/19 7:08 PM, coleen.phillimore at oracle.com wrote:
>>>>>>>>
>>>>>>>> Sorry for being late to the party.
>>>>>>>>
>>>>>>>> http://cr.openjdk.java.net/~pchilanomate/8191890/v02/webrev/src/hotspot/share/runtime/biasedLocking.cpp.frames.html
>>>>>>>>
>>>>>>>>
>>>>>>>> 586 if (_biased_locker == mark->biased_locker()) {
>>>>>>>> 587 if (mark->bias_epoch() == prototype->bias_epoch()) {
>>>>>>>>
>>>>>>>> Can you add a comment what this means? The object's biased
>>>>>>>> locker matches what we thought it was, and the epoch being the
>>>>>>>> same means?
>>>>>>>> The epoch being equal means that this biaser actually might
>>>>>>>> have this lock? A comment would be good here.
>>>>>>> Yes, if the epoch is still valid it means the biaser could be
>>>>>>> currently synchronized on this object. If that's the case then
>>>>>>> we must walk its stack and change those monitor records into
>>>>>>> thin locks. Added comment.
>>>>>>>
>>>>>>>
>>>>>>>> 785 mark = res_mark; // Refresh mark with the latest value.
>>>>>>>>
>>>>>>>>
>>>>>>>> I don't see what this does either. I had to download your
>>>>>>>> patch. 'mark' isn't used outside the loop and it is reloaded at
>>>>>>>> the top of the loop.
>>>>>>> If the CAS fails, the mark needs to be updated with the new
>>>>>>> value so that when we get the current biaser (HR_SINGLE_REVOKE
>>>>>>> case) we actually get the updated biaser and not the old one. If
>>>>>>> we don't do that we could be handshaking the wrong thread, or
>>>>>>> worst we could hit an assert in walk_stack_and_revoke() for the
>>>>>>> "blt == THREAD", since the old thread could be ourselves.
>>>>>>
>>>>>> I found where it's used below now. Maybe once this function is
>>>>>> refactored a bit, it'll be easier to see for next time. Looks good!
>>>>>>>
>>>>>>>
>>>>>>>> 796 obj->cas_set_mark(prototype_header->set_age(mark->age()),
>>>>>>>> mark);
>>>>>>>>
>>>>>>>>
>>>>>>>> As an later enhancement, there should be some inline function
>>>>>>>> in markOop that returns the prototype header preserving the age
>>>>>>>> of the object, but I'll leave it to you to name.
>>>>>>> Ok, sounds good. That particular line was preexistent but I did
>>>>>>> added in some places
>>>>>>> "markOopDesc::prototype()->set_age(mark->age())", which is doing
>>>>>>> the same thing.
>>>>>>>
>>>>>>>
>>>>>>>> 555 RevokeOneBias(Handle* obj, JavaThread* requesting_thread,
>>>>>>>> JavaThread* biased_locker) ... 565 oop o = (*_obj)();
>>>>>>>>
>>>>>>>>
>>>>>>>> This was pre-existing your change, but passing Handle* is not
>>>>>>>> generally done, and is suspicious when it is because it must be
>>>>>>>> allocated with the thread calling the function. Can you change
>>>>>>>> this to Handle (not pointer)? I can't think why this would be
>>>>>>>> done this way.
>>>>>>> Fixed.
>>>>>>>
>>>>>>>
>>>>>>
>>>>>> Great. I was afraid there was some subtlety I didn't see. Please
>>>>>> retest with this though just in case. Sometimes bits of wierdness
>>>>>> have a strange reason that isn't documented.
>>>>> Yes, I'll retest just in case.
>>>>>
>>>>>
>>>>>>>> 870 // All objects in objs should be locked by biaser
>>>>>>>> 871 void BiasedLocking::revoke(GrowableArray<Handle>* objs,
>>>>>>>> JavaThread *biaser) {
>>>>>>>>
>>>>>>>>
>>>>>>>> I don't see why BiasedLocking::revoke() and
>>>>>>>> BiasedLocking::revoke_at_safepoint() are so different.
>>>>>>>>
>>>>>>>> The name "revoke" should be something more descriptive of the
>>>>>>>> situation though, like revoke_for_current_thread() or something
>>>>>>>> like that (revoke_at_safepoint's objects are from the stack too
>>>>>>>> for the current thread...) I keep thinking "revoke" should be
>>>>>>>> a leaf function in biasedLocking.
>>>>>>> Yes, nice observation. Method
>>>>>>> revoke_at_safepoint(GrowableArray<Handle>* objs) could be
>>>>>>> removed and we could just use
>>>>>>> BiasedLocking::revoke(GrowableArray<Handle>* objs ...) instead,
>>>>>>> since it's called from deoptimization.cpp where all the objects
>>>>>>> in the array belong to the same JavaThread. The difference is
>>>>>>> that we don't do update_heuristics() for the non-safepoint case
>>>>>>> since it might trigger a bulk operation. For the safepoint case
>>>>>>> it doesn't matter because we are already at one, we don't have
>>>>>>> the overhead of requesting it. But I could combine them into one
>>>>>>> method and do the update_heuristics() only if we are at a
>>>>>>> safepoint, what do you think?
>>>>>>
>>>>>> You could file a follow-up RFE for this if you want, since the
>>>>>> current version has gone through all the testing.
>>>>> I need to test again anyways and I think this could be a nice
>>>>> simplification. I'll test it and try to include it in v3.
>>>>>
>>>>>> The change looks great!
>>>>> Thanks Coleen! : )
>>>>>
>>>>>
>>>>> Patricio
>>>>>> Coleen
>>>>>>>
>>>>>>>> The change looks really good to me and I look forward to
>>>>>>>> further cleanups so maybe it'll make sense someday!
>>>>>>> Thanks for looking at this Coleen!
>>>>>>>
>>>>>>>
>>>>>>> Patricio
>>>>>>>> Thanks!
>>>>>>>> Coleen
>>>>>>>>
>>>>>>>> On 6/7/19 12:56 AM, Patricio Chilano wrote:
>>>>>>>>> Hi all,
>>>>>>>>>
>>>>>>>>> Here is v02 addressing comments made by Dan and David.
>>>>>>>>>
>>>>>>>>> Full webrev:
>>>>>>>>> http://cr.openjdk.java.net/~pchilanomate/8191890/v02/webrev/
>>>>>>>>> <http://cr.openjdk.java.net/%7Epchilanomate/8191890/v02/webrev/>
>>>>>>>>> Inc webrev:
>>>>>>>>> http://cr.openjdk.java.net/~pchilanomate/8191890/v02/inc/webrev/
>>>>>>>>> <http://cr.openjdk.java.net/%7Epchilanomate/8191890/v02/inc/>
>>>>>>>>>
>>>>>>>>> Thanks!
>>>>>>>>>
>>>>>>>>> Patricio
>>>>>>>>>
>>>>>>>>> On 6/6/19 7:37 PM, David Holmes wrote:
>>>>>>>>>> Hi Patricio,
>>>>>>>>>>
>>>>>>>>>> On 7/06/2019 9:19 am, Patricio Chilano wrote:
>>>>>>>>>>> Hi David,
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 6/6/19 3:37 AM, David Holmes wrote:
>>>>>>>>>>>> Hi Patricio,
>>>>>>>>>>>>
>>>>>>>>>>>> First thanks for taking this on!
>>>>>>>>>>>>
>>>>>>>>>>>> I have some higher-level general discussion around this
>>>>>>>>>>>> before deep diving into the actual code review (not that I
>>>>>>>>>>>> have much there either :)).
>>>>>>>>>>>>
>>>>>>>>>>>> First to clarify how biased-locking works. I'm unclear when
>>>>>>>>>>>> an object can be rebiased after having its bias revoked?
>>>>>>>>>>>> This particularly relates to some of your assertions (as
>>>>>>>>>>>> Markus has queried) after the CAS to update the mark so
>>>>>>>>>>>> that the bias is revoked, and you then re-read the mark and
>>>>>>>>>>>> assert the bias has been revoked - what stops another
>>>>>>>>>>>> thread from rebiasing the object in between those two
>>>>>>>>>>>> statements? Is it that rebiasing cannot happen, or that it
>>>>>>>>>>>> could only happen if there were an intervening safepoint
>>>>>>>>>>>> which in turn cannot happen?
>>>>>>>>>>> Once the bias of the object is revoked it will stay like
>>>>>>>>>>> that forever, it cannot happen that it goes back to having
>>>>>>>>>>> the 0x5 pattern.
>>>>>>>>>>> Also, once the bias pattern in the prototype header for a
>>>>>>>>>>> class is revoked during a bulk revocation operation, if
>>>>>>>>>>> there is an object of that class that still has the bias
>>>>>>>>>>> pattern, a JavaThread that wants to synchronize on that
>>>>>>>>>>> object will always revoke the bias first. This is why I
>>>>>>>>>>> don't check if the CAS succeeded if the prototype of the
>>>>>>>>>>> class does not has the bias pattern, I just assert that the
>>>>>>>>>>> object is not biased anymore.
>>>>>>>>>>>
>>>>>>>>>>> Below I describe the cases where an object can be rebiased.
>>>>>>>>>>>
>>>>>>>>>>> Once a JavaThread biases an object for the first time, there
>>>>>>>>>>> are two cases that allows for that object to be rebiased:
>>>>>>>>>>> 1) If the epoch in the markword becomes invalid. For this to
>>>>>>>>>>> happen a bulk rebias operation is needed. This is why I do
>>>>>>>>>>> check if the CAS succeeded or not for these cases, since
>>>>>>>>>>> some other JavaThread could have rebiased it.
>>>>>>>>>>> 2) During a full GC, objects that are biased ( some
>>>>>>>>>>> JavaThread is set in the biaser bits) could have their
>>>>>>>>>>> markword be reset to 0x5. This means they will become
>>>>>>>>>>> anonymously biased again and so will look as if they were
>>>>>>>>>>> not biased yet. As to how this logic works: At the beginning
>>>>>>>>>>> of the full GC, BiasedLocking::preserve_marks() saves all
>>>>>>>>>>> the markwords for those objects that are currently locked
>>>>>>>>>>> and have a bias pattern. After that,
>>>>>>>>>>> markOopDesc::must_be_preserved_with_bias() will be called to
>>>>>>>>>>> decide if the markword of an object should be preserved or
>>>>>>>>>>> not. If the markword contains the bias pattern it is never
>>>>>>>>>>> preserved. At the end BiasedLocking::restore_marks() is
>>>>>>>>>>> called to restore the marks for those objects that we saved
>>>>>>>>>>> before. So this means that even if an object has a valid
>>>>>>>>>>> biaser, with valid epoch, if the object is not currently
>>>>>>>>>>> locked it could be reset during the GC. I'm not sure though
>>>>>>>>>>> if whenever markOopDesc::must_be_preserved_with_bias()
>>>>>>>>>>> returns false the garbage collector always does the reset or
>>>>>>>>>>> it just means it could reset it if it wants to. In any case
>>>>>>>>>>> I've seen that reset happening when doing handshakes. In
>>>>>>>>>>> fact, this is one of the reasons why the handshake could
>>>>>>>>>>> return that the bias was not revoked, since I don't check
>>>>>>>>>>> for the anonymously biased case in RevokeOneBias.
>>>>>>>>>>
>>>>>>>>>> Thanks for that very detailed set of descriptions. I won't
>>>>>>>>>> pretend to fully grok all the details as I'm not completely
>>>>>>>>>> clear on the role of the "epoch" or being anonymously biased,
>>>>>>>>>> but I'm convinced you have a full understanding of such
>>>>>>>>>> things. :) In revoke_and_rebias it was always a struggle for
>>>>>>>>>> me to figure out exactly when the "rebias" part could come
>>>>>>>>>> into play.
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>> The main concern with a change like this (as with all the
>>>>>>>>>>>> handshake changes) is what new races this may allow and
>>>>>>>>>>>> whether they have all been accounted for. IIUC the
>>>>>>>>>>>> handshake will still be conducted by the VMThread so that
>>>>>>>>>>>> still ensures serialization wrt. safepoints (which makes it
>>>>>>>>>>>> simpler to reason about things). I've looked at some of the
>>>>>>>>>>>> races you anticipated (like the "resurrected" thread) and
>>>>>>>>>>>> they seem to be handled correctly. I'm unable to construct
>>>>>>>>>>>> other races that might be problematic (but that isn't
>>>>>>>>>>>> saying a lot :) ).
>>>>>>>>>>> I agree that since we are now doing the revocation outside
>>>>>>>>>>> safepoints there is potential for additional races. But also
>>>>>>>>>>> one thing to note is that RevokeOneBias, which contains the
>>>>>>>>>>> logic of the handshake and is now replacing what we used to
>>>>>>>>>>> do at a safepoint, is not really different from the initial
>>>>>>>>>>> code in revoke_and_rebias() which is done outside
>>>>>>>>>>> safepoints. The handshake logic is like executing that
>>>>>>>>>>> initial part but with the right JavaThread so that if the
>>>>>>>>>>> object has a valid biaser, then that biaser is either
>>>>>>>>>>> ourselves or we are the VMThread while the biaser is
>>>>>>>>>>> blocked, so that we can execute revoke_own_lock(). In fact I
>>>>>>>>>>> was thinking at some point to combine them in some method
>>>>>>>>>>> (maybe try_fast_revoke()). The attempt_rebias flag and the
>>>>>>>>>>> update_heuristics() in revoke_and_rebias() complicated
>>>>>>>>>>> things so I kept them separate.
>>>>>>>>>>> I have also tried to think on all possible racy scenarios
>>>>>>>>>>> and couldn't find additional problems beside the
>>>>>>>>>>> "resurrected thread" one (although it's also not a guarantee
>>>>>>>>>>> of anything). But that's why I was thinking to check this in
>>>>>>>>>>> 14, so that if there are any problems we have plenty of
>>>>>>>>>>> testing time to detect them.
>>>>>>>>>>
>>>>>>>>>> Yes that is a good idea. No need to rush this into 13.
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>> src/hotspot/share/jfr/metadata/metadata.xml
>>>>>>>>>>>>
>>>>>>>>>>>> Is it the case that this event is now never generated from
>>>>>>>>>>>> a safepoint? Or have you just deleted the safepoint_id from
>>>>>>>>>>>> the event because it might not be at a safepoint? If the
>>>>>>>>>>>> latter can't we keep it and use 0 to indicate "not at a
>>>>>>>>>>>> safepoint"? I think the JFR folk need to comment on this
>>>>>>>>>>>> part of the change anyway.
>>>>>>>>>>> This event will be created and commited only from
>>>>>>>>>>> BiasedLocking::single_revoke_with_handshake(). Now, the
>>>>>>>>>>> actual handshake that revoked the bias could be executed at
>>>>>>>>>>> a safepoint only if ThreadLocalHandshakes is false. But I
>>>>>>>>>>> understand that this is true for all platforms so the
>>>>>>>>>>> handshake should always be executed outside safepoints.
>>>>>>>>>>
>>>>>>>>>> Ok.
>>>>>>>>>>
>>>>>>>>>>> It would be great if JFR folks review this part.
>>>>>>>>>>
>>>>>>>>>> Try to grab Markus :)
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>> src/hotspot/share/runtime/biasedLocking.cpp
>>>>>>>>>>>>
>>>>>>>>>>>> I second Dan's comment about combining cleanup and code
>>>>>>>>>>>> motion in a big change like this - it does make it much
>>>>>>>>>>>> harder to spot the real difference.
>>>>>>>>>>> Ok, already two objections on this so I'll revert moving the
>>>>>>>>>>> heuristics part. I think I also moved
>>>>>>>>>>> clean_up_cached_monitor_info() and I will double check any
>>>>>>>>>>> other movements.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>> I note Dan picked up on the lack of p2i and other stuff
>>>>>>>>>>>> related to the logging statements, and that you indicated
>>>>>>>>>>>> they were fixed. I note that all that stuff is pre-existing
>>>>>>>>>>>> so I'm unclear now whether you have fixed all the logging
>>>>>>>>>>>> in the file or only the statements in the code you have
>>>>>>>>>>>> changed or added? Again such cleanup may be best done
>>>>>>>>>>>> separately.
>>>>>>>>>>> I haven't fixed the existing log statements, only the ones
>>>>>>>>>>> Dan mentioned which are in single_revoke_with_handshake(),
>>>>>>>>>>> revoke_own_lock(), and in VM_HandshakeOneThread(). Ok, I can
>>>>>>>>>>> fix the other ones in a cleanup later along with code
>>>>>>>>>>> movement and the removal of the attemp_rebias flag which we
>>>>>>>>>>> are not using.
>>>>>>>>>>
>>>>>>>>>> Okay. To be clear I don't expect you to fix all the existing
>>>>>>>>>> uses I just wanted to clarify which ones you had fixed.
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>> 640 void BiasedLocking::revoke_own_lock(oop obj,
>>>>>>>>>>>> JavaThread* biased_locker) {
>>>>>>>>>>>> 641 assert(!SafepointSynchronize::is_at_safepoint() ||
>>>>>>>>>>>> !ThreadLocalHandshakes,
>>>>>>>>>>>> 642 "if ThreadLocalHandshakes is enabled this
>>>>>>>>>>>> should always be executed outside safepoints");
>>>>>>>>>>>> 643 assert(Thread::current() == biased_locker ||
>>>>>>>>>>>> Thread::current()->is_VM_thread(), "wrong thread");
>>>>>>>>>>>> 644
>>>>>>>>>>>>
>>>>>>>>>>>> This is called "revoke_own_lock" but it can also be
>>>>>>>>>>>> executed by the VMThread - so its not its own lock. Also we
>>>>>>>>>>>> don't revoke anything related to a "lock" - we revoke a
>>>>>>>>>>>> bias from the markword of an oop. I think a better name is
>>>>>>>>>>>> needed.
>>>>>>>>>>> Yes, I didn't really like it either. How about
>>>>>>>>>>> walk_stack_and_revoke() ?
>>>>>>>>>>
>>>>>>>>>> That sounds good to me. Roll on v2 :)
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> David
>>>>>>>>>> -----
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>> src/hotspot/share/runtime/handshake.cpp
>>>>>>>>>>>>
>>>>>>>>>>>> 125 log_trace(handshake)("JavaThread " INTPTR_FORMAT
>>>>>>>>>>>> " is not alive", (intptr_t)_target);
>>>>>>>>>>>>
>>>>>>>>>>>> Use p2i(_target) rather than cast to intptr_t.
>>>>>>>>>>> Fixed.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>> That's all from me.
>>>>>>>>>>> Thanks for looking into this David! If you are okay with the
>>>>>>>>>>> "walk_stack_and_revoke()" name then I can send v2.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Thanks!
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Patricio
>>>>>>>>>>>
>>>>>>>>>>>> Thanks,
>>>>>>>>>>>> David
>>>>>>>>>>>> -----
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On 30/05/2019 2:29 am, Patricio Chilano wrote:
>>>>>>>>>>>>> Hi all,
>>>>>>>>>>>>>
>>>>>>>>>>>>> Could you review this patch that uses thread local
>>>>>>>>>>>>> handshakes instead of safepoints to revoke the biases of
>>>>>>>>>>>>> locked objects?
>>>>>>>>>>>>>
>>>>>>>>>>>>> Webrev:
>>>>>>>>>>>>> http://cr.openjdk.java.net/~pchilanomate/8191890/v01/webrev/
>>>>>>>>>>>>> Bug:
>>>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8191890
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Today whenever a JavaThread needs to revoke the bias of an
>>>>>>>>>>>>> object that has been biased by another JavaThread (and
>>>>>>>>>>>>> where the epoch is still valid and the prototype header of
>>>>>>>>>>>>> the class still has the bias pattern) it needs to request
>>>>>>>>>>>>> a safepoint operation. The VMThread inside the safepoint
>>>>>>>>>>>>> walks the stack of the biaser looking for lock records
>>>>>>>>>>>>> associated with the biased object, and converts them to
>>>>>>>>>>>>> thin locks if any are found.
>>>>>>>>>>>>> This patch uses thread local handshakes instead, since we
>>>>>>>>>>>>> actually only need to be able to safely walk the stack of
>>>>>>>>>>>>> the JavaThread that biased the object and not other
>>>>>>>>>>>>> JavaThreads.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Some notes about the patch:
>>>>>>>>>>>>>
>>>>>>>>>>>>> - Thanks to Robbin for initial work on this patch and for
>>>>>>>>>>>>> advice and feedback!
>>>>>>>>>>>>> - We still execute bulk rebias and bulk revoke operations
>>>>>>>>>>>>> inside safepoints, since in those cases all the
>>>>>>>>>>>>> JavaThread's stacks need to be walked to potentially
>>>>>>>>>>>>> update lock records.
>>>>>>>>>>>>> - The method revoke_bias() was renamed to
>>>>>>>>>>>>> single_revoke_at_safepoint(). This method is still kept
>>>>>>>>>>>>> because there are places where we check whether we are
>>>>>>>>>>>>> already at safepoint when trying to revoke. In those
>>>>>>>>>>>>> cases, if we are already at a safepoint we simply end up
>>>>>>>>>>>>> calling this method.
>>>>>>>>>>>>> - Handshakes are executed as VMOperations so the VMThread
>>>>>>>>>>>>> is still involved in the revocation. This means we cannot
>>>>>>>>>>>>> have different revocations being executed in parallel
>>>>>>>>>>>>> (same as with safepoints). Ideally we would like to
>>>>>>>>>>>>> execute thread local handshakes without needing for the
>>>>>>>>>>>>> VMThread to participate. However, now other JavaThreads
>>>>>>>>>>>>> that do not participate in the revocation are allow to
>>>>>>>>>>>>> continue making progress.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Run several benchmarks and mostly performance seems
>>>>>>>>>>>>> unaffected. Measured the average time it takes for
>>>>>>>>>>>>> revoking bias with a handshake and with a safepoint and
>>>>>>>>>>>>> numbers are pretty similar varying between benchmarks.
>>>>>>>>>>>>> Some numbers are shown below:
>>>>>>>>>>>>>
>>>>>>>>>>>>> specjbb2015
>>>>>>>>>>>>> Handshakes Safepoints
>>>>>>>>>>>>> Linux 4ms 4.6ms
>>>>>>>>>>>>> Windows 11ms 19ms
>>>>>>>>>>>>>
>>>>>>>>>>>>> startup benchmarks
>>>>>>>>>>>>> Handshakes Safepoints
>>>>>>>>>>>>> Linux 159us 248us
>>>>>>>>>>>>> Windows 150us 111us
>>>>>>>>>>>>>
>>>>>>>>>>>>> Overall the variation is not enough to show significant
>>>>>>>>>>>>> difference in performance, considering also that
>>>>>>>>>>>>> revocations of a valid biaser are usually a fraction of
>>>>>>>>>>>>> the overall running time of a benchmark (specially
>>>>>>>>>>>>> jbb2015). In any case using handshakes allows other
>>>>>>>>>>>>> JavaThreads to make progress during that time, minimizing
>>>>>>>>>>>>> STW operations.
>>>>>>>>>>>>>
>>>>>>>>>>>>> In terms of testing, the patch passed several runs of
>>>>>>>>>>>>> tiers1-6 in mach5 on Windows, Linux, MacOS and Solaris.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks!
>>>>>>>>>>>>>
>>>>>>>>>>>>> Patricio
>>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>
More information about the hotspot-runtime-dev
mailing list