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

[Fredrik Bredberg]

On Thu, 18 Apr 2024 17:20:13 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:
> 
>   Better name for enum for trylock interference.

Looks good to me. Introducing `TryLockResult` really improves the readability.

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

> 984:     if (TrySpin(current)) break;
> 985: 
> 986:     {

This is really a general comment.
I don't like when `break`/`continue`/`return`/`goto` is on the same line as the `if` statement.
Mostly because it makes it very hard to put a breakpoint that triggers on the `break`/`continue`/`return`/`goto`.
In this case (line 984) it's also a bit of a readability issue since you might think that the local block starting at line 986 belongs to the `if` statement, until you see the `break`.
I would prefer if the `break` is put on a line of it's own.
There are many lines like this is the code base and I don't think we should change all, but I do think that we could change it when we do other changes to such a line.

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

Marked as reviewed by fbredberg (Committer).

PR Review: https://git.openjdk.org/jdk/pull/18730#pullrequestreview-2011238247
PR Review Comment: https://git.openjdk.org/jdk/pull/18730#discussion_r1572261755