RFR: 8256425: Obsolete Biased Locking in JDK 18 [v2]

[Daniel D.Daugherty]

On Fri, 18 Jun 2021 15:04:28 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
>
> Patricio Chilano Mateo has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - remove test Test8062950.java + fix commments
>  - fix comment in vm_version_ppc.cpp

Wow! 163 files touched... Biased Locking was certainly more wide
spread than I imagined/remembered.

Thumbs up! I only spotted very minor nits.

Thanks for persisting on this patch. Keeping it up to date over a
couple of release is hard with a 163 file footprint...

src/hotspot/cpu/arm/arm.ad line 5457:

> 5455:     __ b(loop, eq);
> 5456:     __ teq($tmp$$Register, 0);
> 5457:     __ membar(MacroAssembler::Membar_mask_bits(MacroAssembler::LoadStore | MacroAssembler::LoadLoad), noreg);

Does the comment you deleted mean that storeXConditional() is only used
by biased locking or that only biased locking's use of storeXConditional() is
the only caller that needed the membar?

src/hotspot/cpu/arm/vm_version_arm_32.cpp line 362:

> 360:   // Therefore the Biased Locking is enabled on ARMv5 and ARM MP only.
> 361:   //
> 362:   return (!os::is_MP() && (arm_arch() > 5)) ? false : true;

Wow. The gory details are amazing...

src/hotspot/cpu/ppc/ppc.ad line 12140:

> 12138:                                  _rtm_counters, _stack_rtm_counters,
> 12139:                                  ((Method*)(ra_->C->method()->constant_encoding()))->method_data(),
> 12140:                                  /*TM*/ true, ra_->C->profile_rtm());

Not your bug, but that "TM" should be "RTM".

src/hotspot/cpu/ppc/ppc.ad line 12174:

> 12172:     __ compiler_fast_unlock_object($crx$$CondRegister, $oop$$Register, $box$$Register,
> 12173:                                    $tmp1$$Register, $tmp2$$Register, $tmp3$$Register,
> 12174:                                    /*TM*/ true);

Not your bug, but that "TM" should be "RTM".

src/hotspot/cpu/x86/templateTable_x86.cpp line 4026:

> 4024:     // initialize object header only.
> 4025:     __ bind(initialize_header);
> 4026:     __ movptr(Address(rax, oopDesc::mark_offset_in_bytes ()),

Not your bug, but can you delete the space before `()`?

src/hotspot/share/runtime/deoptimization.cpp line 1440:

> 1438:           if (mark.has_locker() && fr.sp() > (intptr_t*)mark.locker()) {
> 1439:             // With exec_mode == Unpack_none obj may be thread local and locked in
> 1440:             // a callee frame. // Make the lock in the callee a recursive lock and restore the displaced header.

Please delete the embedded `//`.

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

Marked as reviewed by dcubed (Reviewer).

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