RFR 8191890: Biased locking still uses the inferior stop the world safepoint for revocation

[David Holmes]

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?

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

---

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.

---

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.

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.

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.

---

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.

---

That's all from me.

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
>