RFR: 8276901: Implement UseHeavyMonitors consistently [v4]

[David Holmes]

On Wed, 17 Nov 2021 20:50:15 GMT, Roman Kennke <rkennke at openjdk.org> wrote:

>> The flag UseHeavyMonitors seems to imply that it makes Hotspot always use inflated monitors, rather than stack locks. However, it is only implemented in the interpreter that way. When it calls into runtime, it would still happily stack-lock. Even worse, C1 uses another flag UseFastLocking to achieve something similar (with the same caveat that runtime would stack-lock anyway). C2 doesn't have any such mechanism at all.
>> I would like to experiment with disabling stack-locking, and thus, having this flag work as expected would seem very useful.
>> 
>> The change removes the C1 flag UseFastLocking, and replaces its uses with equivalent (i.e. inverted) UseHeavyMonitors instead. I think it makes sense to make UseHeavyMonitors develop (I wouldn't want anybody to use this in production, not currently without this change, and not with this change). I also added a flag VerifyHeavyMonitors to be able to verify that stack-locking is really disabled. We can't currently verify this uncondiftionally (e.g. in debug builds) because all non-x86_64 platforms would need work.
>> 
>> Testing:
>>  - [x] tier1
>>  - [x] tier2
>>  - [x] tier3
>>  - [ ] tier4
>
> Roman Kennke has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - Fix formatting
>  - Keep UseHeavyMonitors as release flag, but deprecate it

HI Roman,

I have a number of initial comments/suggestions/requests - see below.

IIUC you are only making UseHeavyMonitors work properly on x86_64, but in that case you cannot convert UseFastLocks to UseHeavyMonitors on all platforms as it won't work correctly on those other platforms.

Cheers,
David

src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 487:

> 485: 
> 486: #if INCLUDE_RTM_OPT
> 487:   if (!UseHeavyMonitors && UseRTMForStackLocks && use_rtm) {

Rather than do this shouldn't `UseHeavyMonitors` and `UseRTMForStackLocks` be mutually exclusive flags, checked in arguments.cpp?

src/hotspot/share/runtime/arguments.cpp line 531:

> 529:   { "FilterSpuriousWakeups",        JDK_Version::jdk(18), JDK_Version::jdk(19), JDK_Version::jdk(20) },
> 530:   { "MinInliningThreshold",         JDK_Version::jdk(18), JDK_Version::jdk(19), JDK_Version::jdk(20) },
> 531:   { "UseHeavyMonitors",             JDK_Version::jdk(18), JDK_Version::jdk(19), JDK_Version::jdk(20) },

Per my CSR request comment this needs to be product only code.

src/hotspot/share/runtime/synchronizer.cpp line 442:

> 440:       // Fall through to inflate() ...
> 441:     } else if (mark.has_locker() &&
> 442:     current->is_lock_owned((address)mark.locker())) {

indent for expression continuation is wrong

src/hotspot/share/runtime/synchronizer.cpp line 816:

> 814:     intptr_t hash;
> 815:     markWord mark = read_stable_mark(obj);
> 816:     if (UseHeavyMonitors && VerifyHeavyMonitors) {

VerifyHeavyMonitors should require that UseHeavyMonitors be set. There should be logic in arguments.cpp to check that.

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

Changes requested by dholmes (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/6320