RFR(S): 8236035: refactor ObjectMonitor::set_owner() and _owner field setting
Daniel D. Daugherty
daniel.daugherty at oracle.com
Tue Jan 28 19:56:23 UTC 2020
I've made additional changes based on Kim Barrett's code review on CR1.
JDK-8236035 refactor ObjectMonitor::set_owner() and _owner field
Here's the incremental webrev URL:
Here's the full webrev URL:
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.
On 1/27/20 3:06 PM, Daniel D. Daugherty wrote:
> 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
> Here's the incremental webrev URL:
> Here's the full webrev URL:
> 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()
> 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
> 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.
> On 12/17/19 4:35 PM, Daniel D. Daugherty wrote:
>> I'm extracting another standalone fix from the Async Monitor Deflation
>> project (JDK-8153224) and sending it out for review (and testing)
>> JDK-8236035 refactor ObjectMonitor::set_owner() and _owner field
>> Here's the webrev URL:
>> 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*
>> 239 // Simply set _owner field to self; current value must match
>> 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
>> 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
>> - 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.
More information about the hotspot-runtime-dev