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

[Daniel D. Daugherty]

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"

     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.

     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.

         But what are you trying to do with these updates to 'mark'?

         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)'?

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.

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.


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.

Outstanding job on a very arcane and complicated part of the system.

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
>