RFR: 8225344: Avoid VM_EnableBiasedLocking VM operation during bootstrap if BiasedLockingStartupDelay is 0

[Claes Redestad]

Hi Dan,

On 2019-06-26 20:30, Daniel D. Daugherty wrote:
> On 6/26/19 9:10 AM, Claes Redestad wrote:
>> Hi,
>>
>> please review this cleanup to avoid a safepointing VM operation during
>> bootstrap to enable biased locking if the configured delay is zero
>> (current default).
>>
>> Webrev: http://cr.openjdk.java.net/~redestad/8225344/open.00/
> 
> src/hotspot/share/runtime/biasedLocking.cpp
>      L45: static bool _biased_locking_enabled = false;
> 
>          In old code set here:
>              old L69:     _biased_locking_enabled = true;
>          in VM_EnableBiasedLocking::doit() (so at a safepoint).
> 
>          In new code set here:
>              new L56:   _biased_locking_enabled = true;
>          in enable_biased_locking() which is called from:
> 
>              new L72:     enable_biased_locking();
>              new L108:       enable_biased_locking();
> 
>          L72 is in VM_EnableBiasedLocking::doit() so no change.
>          L108 is in BiasedLocking::init() which is called from
>          fairly late in Threads::create_vm() so this is new.
> 
>          _biased_locking_enabled was queried in BiasedLocking::enabled():
> 
>              old L116:   return _biased_locking_enabled;
> 
>          and is now queried like this:
> 
>              new L116:   return _biased_locking_enabled || 
> BiasedLockingStartupDelay == 0;
> 
>          By checking "BiasedLockingStartupDelay == 0" you are guarding
>          against an early call to BiasedLocking::enabled() seeing the
>          initial _biased_locking_enabled value of 'false' before
>          BiasedLocking::init() can set it true. I don't know if it is
>          possible for another thread to call BiasedLocking::enabled()
>          before the "creat_vm()" thread has called BiasedLocking::init(),
>          but your check of "BiasedLockingStartupDelay == 0" should fix
>          that case. I think you need a short comment explaining your
>          reasoning. Perhaps:
> 
>            // We check "BiasedLockingStartupDelay == 0" here to cover the
>            // possibility of a call to BiasedLocking::enabled() before
>            // BiasedLocking::init() has been called.

Right, for correctness it's not _strictly_ necessary to set
_biased_locking = true at all in the BiasedLockingStartupDelay 0 case
(which is when we do it from L108 / BiasedLocking::init(), but there are
tests that get unhappy if the -Xlog:biasedlocking doesn't log that it's
been enabled ;-) so it seemed prudent to factor it into a method and
be consistent.

I'll add your suggested comment.

> 
> src/hotspot/share/classfile/systemDictionary.cpp
>      No comments.
> 
> Thumbs up! Thanks for detangling this.

Thanks!

/Claes