RFR: 8256425: Obsolete Biased Locking in JDK 18

[David Holmes]

On Thu, 17 Jun 2021 15:37:40 GMT, Patricio Chilano Mateo <pchilanomate at openjdk.org> wrote:

> Hi all,
> 
> Please review the following patch which handles the removal of biased locking code. 
> 
> The third least significant bit of the markword is now always unused. I didn't try to give it back to the age field as it was prior to biased locking introduction since it will likely be taken away by other projects (probably Valhalla). 
> 
> Regarding c1 changes, the scratch register passed to LIRGenerator::monitor_enter() was only used by biased locking code except in ppc, so in all other platforms I removed the scratch parameter from C1_MacroAssembler::lock_object() (except in s390 where it wasn't defined already). 
> We could probably just always use R0 as a temp register in lock_object() for ppc, since we were already using it as temp in biased_locking_enter(), and remove the scratch parameter from there too. Then we could remove the scratch field from LIR_OpLock. I haven't done that in this patch though.
> 
> For c2, type.hpp defined XorXNode, StoreXConditionalNode, LoadXNode and StoreXNode as needed by UseOptoBiasInlining. I see that LoadXNode and StoreXNode are also used by shenandoahSupport so I kept those two defines. I removed only the biased locking comments from the storeIConditional/storeLConditional implementations in .ad files since I don't know if they might be needed.
> 
> There are some tests that were only meaningful when run with biased locking enabled so I removed them.
> 
> Tested in mach5 tiers 1-7. I tested it builds also on ppc, s390 and arm32 but can't run any tests on those platforms so it would be good if somebody can do some sanity check on those ones.
> 
> Thanks,
> Patricio

Hi Patricio,

Huge cleanup! Looks great to see so much red. :)

I looked through everything and have a few minor comments below. Can't comment in detail on JIT changes (or whether further improvements are possible) but it all looks okay from an "eradicate biased-locking" perspective.

Thanks,
David

src/hotspot/share/code/nmethod.hpp line 281:

> 279:   // will never cause Class instances to be biased but this code
> 280:   // handles the static synchronized case as well.
> 281:   // JVMTI's GetLocalInstance() also uses these offsets to find the receiver

Not obvious that this entire comment is no longer relevant. The basic description of the use of the offsets seems applicable even if not actually needed for revoking the bias.

src/jdk.internal.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/HotSpotVMConfig.java line 156:

> 154:     long prototypeMarkWord() {
> 155:         return markWordNoHashInPlace | markWordNoLockInPlace;
> 156:     }

It is not immediately obvious that this is correct for all object types. Does this match what the initial_mark() is now?

test/hotspot/jtreg/compiler/c2/Test8062950.java line 2:

> 1: /*
> 2:  * Copyright (c) 2018, 2021, Oracle and/or its affiliates. All rights reserved.

I'd argue this test serves no purpose now.

test/hotspot/jtreg/runtime/handshake/HandshakeDirectTest.java line 65:

> 63:                 // Inflate locks[handshakee] if possible
> 64:                 System.identityHashCode(locks[handshakee]);
> 65:                 walked = wb.handshakeReadMonitors(workingThreads[handshakee]);

It is not at all obvious that this revised test and the new WB routine actually test what was previously being tested. Do we actually need to involve monitors here or is that just something that has been picked to examine while in a handshake?

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

Marked as reviewed by dholmes (Reviewer).

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