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

David Holmes david.holmes at oracle.com
Wed Jan 29 07:20:24 UTC 2020


Hi Dan,

Sorry I didn't get to this earlier today.

On 29/01/2020 5:56 am, Daniel D. Daugherty wrote:
> 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/

Only suggestion - which you can take or leave - is that:

    // Try to set _owner field to new_value if the current value matches
    // old_value. Otherwise, does not change the _owner field. Returns
    // the prior value of the _owner field. try_set_owner_from() provides:
    //   <fence> compare-and-exchange <membar StoreLoad|StoreStore>

could more simply be:

    // Try to set _owner field to new_value if the current value matches
    // old_value, using Atomic::cmpxchg. Otherwise, does not change the
    // _owner field. Returns the prior value of the _owner field.

The memory semantics are then implicitly the same as CAS and no need to 
repeat that awkward comment (which tends to always raise questions about 
the presumed bi-directional "fence" provided by the atomic rmw methods).

Thanks,
David
-----

> 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
>>>
>>
> 


More information about the hotspot-runtime-dev mailing list