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

[Coleen Phillimore]

On Tue, 16 Apr 2024 21:36:40 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

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

Yes, it was the same value as Knob_Bonus.

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

I had some logging in one version of this patch and this call to TrySpin, succeeded a greater percentage of time, vs. the other calls to TrySpin.

> src/hotspot/share/runtime/objectMonitor.cpp line 713:
> 
>> 711: 
>> 712:   // Try the lock - TATAS
>> 713:   if (TryLock (current) == SUCCESS) {
> 
> nit: s/TryLock (/TryLock(/

Fixed.

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

fixed.

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

I removed this and thought I added it back.  It seemed pointless, since the first thing TrySpin's going to do is TryLock.

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

Yes, I added that.  It's not better in my testing.

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

Yes, getting rid of a knob for tuning seems to reduce complexity.  BonusB was if we grabbed the lock in the 10 count pre-spin.  I tested variations of not adjusting the spin count in the pre-spin and got different (sometimes worse) results.  If there are further adjustment ideas, I could add back some parameter.  The thing I wanted to do is stop squinting at the duplicated code for adjusting up, taking in account the lower bound.  I should fix the formatting here.

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

Yes.  Since we returned 0, 1, and -1, I wanted to keep that and see if that mattered.  If the lock changes ownership below, we abort the spin.  I thought it should do the same in the prespin.  I didn't think it would materially affect the performance, other than variations, but since we're returning values, might as well use them somewhere.

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

Yes, the only time we adjust down is when we hit the spin count, but the code in Abort has to be run on all paths to fix up succ and fence.  Not sure if that following TryLock would ever succeed.  Seems very unlikely!

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18730#discussion_r1568735324
PR Review Comment: https://git.openjdk.org/jdk/pull/18730#discussion_r1568741098
PR Review Comment: https://git.openjdk.org/jdk/pull/18730#discussion_r1568741700
PR Review Comment: https://git.openjdk.org/jdk/pull/18730#discussion_r1568742413
PR Review Comment: https://git.openjdk.org/jdk/pull/18730#discussion_r1568743371
PR Review Comment: https://git.openjdk.org/jdk/pull/18730#discussion_r1568743954
PR Review Comment: https://git.openjdk.org/jdk/pull/18730#discussion_r1568752394
PR Review Comment: https://git.openjdk.org/jdk/pull/18730#discussion_r1568755636
PR Review Comment: https://git.openjdk.org/jdk/pull/18730#discussion_r1568758962