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

[David Holmes]

Hi Dan,

On 18/12/2019 7:35 am, 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.

This is obviously functionally fine so the only issues are 
stylistic/naming ones. Apologies if what I'm about to say contradicts 
anything I've said previously. :)

1. I think the simply_ prefix is unnecessary.

2. I think release_clear_owner_with_barrier is too complex/awkward. It 
would be better IMO to just have release_clear_owner() and leave the 
correct trailing barrier in the caller.

3. try_ operations are typically boolean operations. In this code one of 
the try_set_owner_from calls appears to need the returned Thread*, but 
on closer inspection this seems not to be the case:

1684       ox = (Thread*)try_set_owner_from(NULL, Self);
1685       if (ox == NULL) {
... // implied else
1707       // The CAS failed ... we can take any of the following actions:
...
1712       prv = ox;
1713       goto Abort;

So we set prv from ox but then goto Abort which never references prv, so 
AFAICS we don't need to set it, so all we need from try_set_owner_from 
is true or false.

4. In my own Java objectMonitor work I call 
simply_set_owner_from_BasicLock "transition_from_basic_lock". It makes 
it clearer (to me at least) exactly what purpose this serves and avoids 
the need for comments to that effect.


One remaining concern with this refactoring and the associated logging 
is the potential impact it may have on performance due to inlining 
changes and the need to check if logging is enabled.

The above comments carry no more or less weight than anyone else's so 
I'll leave it to you and other reviewers as to whether anything actually 
changes. :)

Thanks,
David
-----

> Dan