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

[Daniel D. Daugherty]

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
>