RFR(S): 8236035: refactor ObjectMonitor::set_owner() and _owner field setting

[Daniel D. Daugherty]

Greetings,

I've made additional changes based on Kim Barrett's code review on CR1.
Thanks Kim!!

     JDK-8236035 refactor ObjectMonitor::set_owner() and _owner field 
setting
     https://bugs.openjdk.java.net/browse/JDK-8236035

Here's the incremental webrev URL:

http://cr.openjdk.java.net/~dcubed/8236035-webrev/2-for-jdk15.inc/

Here's the full webrev URL:

http://cr.openjdk.java.net/~dcubed/8236035-webrev/2-for-jdk15.full/

Here's what changed between CR1 and CR2:

   - rearrange some loads of _owner field to be more efficient
   - clarify header comment for try_set_owner_from() declaration
   - make some loads of _owner field DEBUG_ONLY since they only exist
     for assert()'s; update related logging calls to use the existing
     function parameter instead.

I'm in the process of testing these changes with a Mach5 Tier[1-3].
I've done a local build on my MBP13 and done KitchensinkSanity testing.

Thanks, in advance, for comments, questions or suggestions.

Dan


On 1/27/20 3:06 PM, Daniel D. Daugherty wrote:
> Greetings,
>
> I'm still looking for a second reviewer on this thread. I've gone ahead
> and made changes based on David H's comments on CR0.
>
>     JDK-8236035 refactor ObjectMonitor::set_owner() and _owner field 
> setting
>     https://bugs.openjdk.java.net/browse/JDK-8236035
>
> Here's the incremental webrev URL:
>
> http://cr.openjdk.java.net/~dcubed/8236035-webrev/1-for-jdk15.inc/
>
> Here's the full webrev URL:
>
> http://cr.openjdk.java.net/~dcubed/8236035-webrev/1-for-jdk15.full/
>
> Here's what changed between CR0 and CR1:
>
>   - rename simply_set_owner_from() -> set_owner_from() and
>     simply_set_owner_from_BasicLock() -> set_owner_from_BasicLock()
>   - rename release_clear_owner_with_barrier() -> release_clear_owner() 
> and
>     refactor barrier code back into the call sites.
>
> These changes have been tested in a Mach5 Tier[1-3] run with no
> regressions. They have also been merged with 8235931 and 8235795 and
> included in a Mach5 Tier[1-8] run with no known regressions (so far
> since Tier8 is not quite finished).
>
> I did a SPECjbb2015 run on these bits with a jdk-14+32 baseline and 25 
> runs:
>
>     criticalJOPS                -0.40%  (Non-significant)
>                    66754.32   66484.36
>                   ± 1209.80  ± 2099.86
>                              p = 0.581
>
>     maxJOPS                     -0.30%  (Non-significant)
>                    90965.80   90695.68
>                   ± 1788.39  ± 1998.67
>                              p = 0.617
>
> All of these results were flagged as "Non-significant" by the perf
> testing system. Looks like "p" values are still too high.
>
> Thanks, in advance, for comments, questions or suggestions.
>
> Dan
>
>
> On 12/17/19 4:35 PM, Daniel D. Daugherty wrote:
>> Greetings,
>>
>> I'm extracting another standalone fix from the Async Monitor Deflation
>> project (JDK-8153224) and sending it out for review (and testing)
>> separately.
>>
>>     JDK-8236035 refactor ObjectMonitor::set_owner() and _owner field 
>> setting
>>     https://bugs.openjdk.java.net/browse/JDK-8236035
>>
>> Here's the webrev URL:
>>
>> http://cr.openjdk.java.net/~dcubed/8236035-webrev/0-for-jdk15/
>>
>> Folks that have reviewed JDK-8153224 will recognize these changes as
>> a subset of the _owner field changes from the Async Monitor Deflation
>> project. It's a subset because the Async Monitor Deflation project
>> needs one additional simply_set_owner_from() variant.
>>
>> In this set of changes:
>>
>> - We replace the one use of set_owner() and the direct _owner field
>>   updates with four distinct functions:
>>
>>  235   // Clear _owner field; current value must match old_value.
>>  236   void      release_clear_owner_with_barrier(void* old_value, 
>> bool needs_fence);
>>  237   // Simply set _owner field to new_value; current value must 
>> match old_value.
>>  238   void      simply_set_owner_from(void* old_value, void* 
>> new_value);
>>  239   // Simply set _owner field to self; current value must match 
>> basic_lock_p.
>>  240   void      simply_set_owner_from_BasicLock(void* basic_lock_p, 
>> Thread* self);
>>  241   // Try to set _owner field to new_value if the current value 
>> matches
>>  242   // old_value. Otherwise, does not change the _owner field.
>>  243   void*     try_set_owner_from(void* old_value, void* new_value);
>>
>> - Each function has an assert() to verify the pre-condition and has new
>>   log_trace(monitorinflation, owner) calls for logging ownership 
>> changes.
>> - The gory details about necessary memory syncing or why memory syncing
>>   is not needed is now in one place rather than at each call site.
>>
>> These changes are being tested in a Mach5 Tier[1-3] run that's still
>> running (JDK14 ATR has priority). I'm also running these changes thru
>> a set of 10 SPECjbb2015 runs.
>>
>> Thanks, in advance, for comments, questions or suggestions.
>>
>> Dan
>>
>