RFR: 8330051: Small ObjectMonitor spinning code cleanups [v2]

Tue Apr 16 22:16:45 UTC 2024

On Wed, 10 Apr 2024 18:36:42 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> Please review this patch that refactors some functions in the adaptive spinning code and moves the Knobs to where they're used and can be modified for experiments in the following code.  Also changed 'goto's to a 'break'.  I didn't change the names of TrySpin or TryLock, since all the names in this file are this style.
>> 
>> The only non-cosmetic change is for the pre-spin.  If the TryLock returns that a CAS failed, then the pre-spin ends, which shouldn't make a difference wrt to performance because the pre-spin is only for 10 iterations, but somehow my numbers are a little bit better for both Dacapo pmd and xalan.
>> 
>> Tested with tier1-7.
>
> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Move first fixed spin after the comment that describes why we do it.

There are some changes in behavior that I've flagged in this pass.

They might actually be okay, but I have to mull on it. For now I'm
not yet ready to approve.

src/hotspot/share/runtime/objectMonitor.cpp line 118:

> 116: 
> 117: static int Knob_Bonus               = 100;     // spin success bonus
> 118: static int Knob_BonusB              = 100;     // spin success bonus

`Knob_BonusB` has been deleted. Is this intentional?

src/hotspot/share/runtime/objectMonitor.cpp line 395:

> 393:   // transitions.  The following spin is strictly optional ...
> 394:   // Note that if we acquire the monitor from an initial spin
> 395:   // we forgo posting JVMTI events and firing DTRACE probes.

This has always bothered me, but is existing and unrelated to your PR.

src/hotspot/share/runtime/objectMonitor.cpp line 713:

> 711: 
> 712:   // Try the lock - TATAS
> 713:   if (TryLock (current) == SUCCESS) {

nit: s/TryLock (/TryLock(/

src/hotspot/share/runtime/objectMonitor.cpp line 784:

> 782:     // Interference - the CAS failed because _cxq changed.  Just retry.
> 783:     // As an optional optimization we retry the lock.
> 784:     if (TryLock (current) == SUCCESS) {

nit: s/TryLock (/TryLock(/

src/hotspot/share/runtime/objectMonitor.cpp line 984:

> 982:     assert(owner_raw() != current, "invariant");
> 983: 
> 984:     if (TrySpin(current)) break;

This is also a change in behavior. The old code is `TryLock()` followed by `TrySpin()`
and you've replaced that with just a `TrySpin()`.

src/hotspot/share/runtime/objectMonitor.cpp line 1849:

> 1847: static int Knob_Poverty             = 1000;
> 1848: static int Knob_FixedSpin           = 0;
> 1849: static int Knob_PreSpin             = 10;      // 20-100 likely better, but it's not

The `but it's not` part of the comment is not in the original.

src/hotspot/share/runtime/objectMonitor.cpp line 1855:

> 1853:   if (x < ObjectMonitor::Knob_SpinLimit) {
> 1854:     if (x < Knob_Poverty) { x = Knob_Poverty; }
> 1855:     return x + Knob_Bonus;

In the original code this was:
`_SpinDuration = x + Knob_BonusB;`

so the `Knob_Bonus` variable is now used instead of `Knob_BonusB` which
had the same value in the original code. I can live with out being able to tune
that separately, but it is a change in behavior.

src/hotspot/share/runtime/objectMonitor.cpp line 1886:

> 1884:     } else if (status == CAS_FAILED) {
> 1885:       break;
> 1886:     }

This is the new behavior you talked about happening when CAS failed.

src/hotspot/share/runtime/objectMonitor.cpp line 2005:

> 2003:   }
> 2004: 
> 2005:  Abort:

Okay. It took a few back-and-forth pans of the code, but I've
convinced myself that your switch from `goto Abort` is equivalent
to your `break` logic *except when the break occurs when `ctr < 0` is true.*
In that case, you'll now do the `adjust_down()` call when the `goto Abort`
would have skipped it.

I don't know (yet) if this causes a problem, but it does appear to be another
change in behavior.

src/hotspot/share/runtime/objectMonitor.hpp line 358:

> 356: 
> 357: 
> 358:   enum TryLockResult { CAS_FAILED = -1, HAS_OWNER = 0, SUCCESS = 1};

nit: Perhaps just one blank line to separate the new enum.

-------------

Changes requested by dcubed (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18730#pullrequestreview-2004612213
PR Review Comment: https://git.openjdk.org/jdk/pull/18730#discussion_r1567967807
PR Review Comment: https://git.openjdk.org/jdk/pull/18730#discussion_r1567959451
PR Review Comment: https://git.openjdk.org/jdk/pull/18730#discussion_r1567957248
PR Review Comment: https://git.openjdk.org/jdk/pull/18730#discussion_r1567958279
PR Review Comment: https://git.openjdk.org/jdk/pull/18730#discussion_r1567961371
PR Review Comment: https://git.openjdk.org/jdk/pull/18730#discussion_r1567968256
PR Review Comment: https://git.openjdk.org/jdk/pull/18730#discussion_r1567976075
PR Review Comment: https://git.openjdk.org/jdk/pull/18730#discussion_r1567973324
PR Review Comment: https://git.openjdk.org/jdk/pull/18730#discussion_r1567990230
PR Review Comment: https://git.openjdk.org/jdk/pull/18730#discussion_r1567949874