On Wed, 26 Apr 2023 08:12:41 GMT, Quan Anh Mai <qa...@openjdk.org> wrote:
>> Roman Kennke has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Remove unnecessary comments > > src/hotspot/cpu/x86/c2_CodeStubs_x86.cpp line 81: > >> 79: // C2CodeStubList::emit() will throw an assertion and report the >> actual size that >> 80: // is needed. >> 81: return 33; > > This should be 36 with `ASSERT` and 21 without. If you are sure that > `JavaThread::lock_stack_top_offset()` or > `OM_OFFSET_NO_MONITOR_VALUE_TAG(owner)` fits within an `int8_t` then it > reduces 3 bytes for each usage. This stub has 2 instructions, and it seems not really uncommon, is it worth it to have a stub here? > src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 781: > >> 779: #ifdef _LP64 >> 780: C2HandleAnonOMOwnerStub* stub = new >> (Compile::current()->comp_arena()) C2HandleAnonOMOwnerStub(tmpReg, boxReg); >> 781: Compile::current()->output()->add_stub(stub); > > This should be added only if we are really emitting the code (i.e. not > emitting into a scratch buffer to measure the node size) Also, I think this `if (LockingMode == LM_LIGHTWEIGHT)` block should be moved out of the enclosing if block, we are checking for inflation here, it seems logical to separate the inflation path out. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/10907#discussion_r1177570766 PR Review Comment: https://git.openjdk.org/jdk/pull/10907#discussion_r1177304198
