RFR(S): 8236035: refactor ObjectMonitor::set_owner() and _owner field setting
Daniel D. Daugherty
daniel.daugherty at oracle.com
Wed Jan 29 16:42:19 UTC 2020
Hi Robbin,
Thanks for jumping in on this review thread.
On 1/29/20 5:10 AM, Robbin Ehn wrote:
> Hi Dan, sorry for being late here.
>
> Yes, seems good.
Thanks!
> But we have a dozen reads of _owner and two of them uses preferred
> syntax of Atomic::load.
>
> To get consistency of reads, in future patches, please consider a:
> " void* get_owner() {
> Atomic::load(&_owner);
> }
> "
>
> So don't add this now to this change-set since David and Kim are fine
> with as-is!
Right. Adding a get_owner() accessor would be a part of the general
_owner field cleanup bug that I've mentioned in this review thread
as a follow-up task. So I've filed this new RFE:
JDK-8238174 migrate ObjectMonitor::_owner field away from C++
volatile semantics
https://bugs.openjdk.java.net/browse/JDK-8238174
Again, thanks for chiming in on this review!
Dan
>
> Thanks, Robbin
>
> On 1/28/20 8:56 PM, 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/
>>
>> 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