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

[Daniel D. Daugherty]

Hi David,

Thanks for re-reviewing!


On 1/29/20 2:20 AM, David Holmes wrote:
> Hi Dan,
>
> Sorry I didn't get to this earlier today.

No apology needed. There's a lot going on right now. :-)


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

I like it! I'll update that 4 line comment to your 3 line version.
I particularly like describing it in terms of CAS so that I don't
duplicate wording here.

I'm not planning to send out a webrev for just this comment change.

Dan


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