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

[Patricio Chilano]

Hi Dan,

On 5/30/19 3:28 PM, Daniel D. Daugherty wrote:
> On 5/29/19 12:29 PM, 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/
>
> src/hotspot/share/runtime/biasedLocking.hpp
>     No comments.
>
> src/hotspot/share/runtime/vmOperations.hpp
>     No comments.
>
> src/hotspot/share/runtime/biasedLocking.cpp
>     L170:   assert(Thread::current()->is_VM_thread(), "should be 
> VMThread");
>     L334:   assert(Thread::current()->is_VM_thread(), "should be 
> VMThread");
>         Since you are assert()'ing the condition: s/should/must/
>
>     L463:   virtual bool doit_prologue()   { return true; }
>         You get a doit_prologue() that simply return 'true' for
>         free from the VM_Operation super class.
>
>     L592:   log_info(biasedlocking, handshake)("JavaThread " 
> INTPTR_FORMAT " handshaking JavaThread "
>     L593:                                      INTPTR_FORMAT " to 
> revoke object " INTPTR_FORMAT, (intptr_t)requester,
>     L594:                                      (intptr_t)biaser, 
> p2i((void *)obj()));
>         Use p2i(requester) and p2i(biaser) instead of the casts.
>         p2i(obj()) should also work without the cast.
>
>     L602:     log_info(biasedlocking, handshake)("Handshake revocation 
> for object " INTPTR_FORMAT " succeded. Bias was %srevoked",
>         Typo - s/succeded/succeeded/
>
>     L603:                                         p2i((void *)obj()), 
> (revoke.status_code() == BIAS_REVOKED ? "" : "already "));
>         p2i(obj()) should also work without the cast.
>
>     L609:   }
>     L610:   else {
>         nit - generally HotSpot style is '} else {'
>
>     L654:                            (intptr_t)Thread::current(),
>     L655:                            p2i((void *)obj),
>     L656:                            (intptr_t) mark,
>     L658:                            (intptr_t) 
> obj->klass()->prototype_header(),
>     L659:                            (intptr_t) biased_locker,
>         Use p2i(foo) instead of (intptr_t)foo.
>         p2i(obj()) should also work without the cast.
>
>     L671:                                p2i((void *) obj));
>     L679:                                p2i((void *) obj));
>         p2i(obj()) should also work without the cast.
>
>     L672:       // Assume recursive case and fix up highest lock later
>         Perhaps:
>                 // Assume recursive case and fix up the actual highest 
> lock below.
>
>     L687:     // Must release storing the lock address for platforms 
> without TSO
>         Typo - s/storing/store/
>
>     L699:   assert(!obj->mark()->has_bias_pattern(), "why not?");
>         Perhaps: "must not be biased"
Fixed all above.


>     old L313: enum HeuristicsResult {
>     old L321: static HeuristicsResult update_heuristics(oop o, bool 
> allow_rebias) {
>     new L703: enum HeuristicsResult {
>     new L711: static HeuristicsResult update_heuristics(oop o) {
>         This is all code motion with the exception of removing the
>         unused 'allow_rebias' parameter. That makes the code harder
>         to review.
Yes, I wanted to moved it above revoke_and_rebias() where it is actually 
used. Also I wanted to put single_revoke_at_safepoint() and 
bulk_revoke_or_rebias_at_safepoint() together. But I can move it back 
where it was if you want.


> L767:   while(true) {
>         nit - need a space before '('
>
>     L786:       // Read the updated mark
>     L787:       mark = res_mark;
>     L826:         // Read the updated mark
>     L827:         mark = res_mark;
>         Perhaps:
>                 mark = res_mark;  // Refresh mark with the latest value.
Fixed both.


> But what are you trying to do with these updates to 'mark'?
If the CAS failed in those cases then it means that something changed in 
the markword. Either is not biased anymore, or somebody biased the 
object. If it's the first case then update_heuristics() will return 
HR_NOT_BIASED, so the update doesn't matter. Otherwise somebody biased 
the object and update_heuristics() will either return HR_SINGLE_REVOKE 
or HR_BULK_REVOKE. For the latter case that update does not matter 
either. But for the HR_SINGLE_REVOKE case we will have to read the 
biaser (JavaThread *blt = mark->biased_locker()) to see which JavaThread 
to handshake. If we use the old mark then we would be handshaking the 
old thread (it could even be ourselves in which case we would hit an 
assert in revoke_own_lock()).
We could just try to avoid the update and do the following when reading 
the biaser:

JavaThread *blt = obj->mark()->biased_locker();

but we could also hit an assert this time in biased_locker(), since by 
the time update_heuristics() returned and before we execute that line, 
the biaser could have chosen to revoke the bias. We could do an 
additional check for the bias pattern (once more) before trying to read 
the biaser. But I think this would look weird since we are already in a 
branch where the mark should be biased.
Alternatively, two other options could be:
1) update_heuristics() could be passed a (JavaThread*) which it would 
set to the biaser.
2) use the old check (mark->biased_locker() == THREAD && 
prototype_header->bias_epoch() == mark->bias_epoch()). But I don't like 
this approach because it involves reading more values, which are sort of 
redundant since we can already tell by updating the markword above which 
condition we are in.


> This line at the top of the loop obviates those assignments.
>             L772:     markOop mark = obj->mark();
>
>         Perhaps you meant for L772 to be before the 'while (true)'?
The loop will only be executed again if we do not succeed in revoking 
from single_revoke_with_handshake() (the biaser could have change and 
that prevents us from revoking). In that case we would have to execute 
revoke_and_rebias() as if it was the first time.


> src/hotspot/share/runtime/handshake.cpp
>     L125:       log_trace(handshake)("JavaThread " INTPTR_FORMAT " is 
> not alive", (intptr_t)_target);
>     L129:     log_trace(handshake)("JavaThread " INTPTR_FORMAT " 
> signaled, begin attempt to process by VMThtread", (intptr_t)_target);
>         Use 'p2i(_target)' instead of cast.
Fixed.


> src/hotspot/share/runtime/deoptimization.cpp
>     No comments.
>
> src/hotspot/share/interpreter/interpreterRuntime.cpp
>     No comments.
>
> src/hotspot/share/jfr/metadata/metadata.xml
>     No comments.
>
> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/VMOps.java
>     No comments.
>
> test/jdk/jdk/jfr/event/runtime/TestBiasedLockRevocationEvents.java
>     Please update copyright year.
Fixed.


> This looks great. As with anything to do with locking, I'll have to
> let this percolate in the background and I may have more comments or
> questions. None of my comments are critical/must-fix right now type
> stuff.
Great, I'll wait then to send v02 in case you have more comments.

> Outstanding job on a very arcane and complicated part of the system.
Thanks Dan! : )  And thanks for reviewing this!


Patricio
> Dan
>
>
>> 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
>>
>