RFR: 8331911: Reconsider locking for recently disarmed nmethods

[Aleksey Shipilev]

On Fri, 17 May 2024 13:51:57 GMT, Neethu <duke at openjdk.org> wrote:

> **Notes**
> We are spending significant time on acquiring the per-nmethod as all the
> threads are in same nmethod.
> Adding double-check lock by calling is_armed before lock acquisition.
> 
> **Verification**
> 
> Shenendoah
> 
>> % /home/neethp/Development/opensource/jdk/build/linux-x86_64-server-release/images/jdk/bin/java -Xmx1g -Xms1g -XX:+UseShenandoahGC -Xlog:gc ManyThreadsStacks.java 2>&1 | grep "marking roots"
>> [0.706s][info][gc] GC(0) Concurrent marking roots 11.519ms
>> [0.752s][info][gc] GC(1) Concurrent marking roots 9.833ms
>> [0.814s][info][gc] GC(2) Concurrent marking roots 10.000ms
>> [0.855s][info][gc] GC(3) Concurrent marking roots 9.314ms
>> [0.895s][info][gc] GC(4) Concurrent marking roots 8.937ms
>> [1.213s][info][gc] GC(5) Concurrent marking roots 12.582ms
>> [1.340s][info][gc] GC(6) Concurrent marking roots 9.574ms
>> [1.465s][info][gc] GC(7) Concurrent marking roots 12.791ms
> 
> ZGC
> 
>> % /home/neethp/Development/opensource/jdk/build/linux-x86_64-server-release/images/jdk/bin/java -Xmx1g -Xms1g -XX:+UseShenandoahGC -Xlog:gc ManyThreadsStacks.java 2>&1 | grep "marking roots"
>> [0.732s][info][gc] GC(0) Concurrent marking roots 10.694ms
>> [0.782s][info][gc] GC(1) Concurrent marking roots 14.614ms
>> [0.825s][info][gc] GC(2) Concurrent marking roots 12.700ms
>> [0.863s][info][gc] GC(3) Concurrent marking roots 9.622ms
>> [0.904s][info][gc] GC(4) Concurrent marking roots 12.892ms
>> [1.244s][info][gc] GC(5) Concurrent marking roots 12.422ms
>> [1.375s][info][gc] GC(6) Concurrent marking roots 12.756ms
>> [1.503s][info][gc] GC(7) Concurrent marking roots 12.265ms
>> [1.628s][info][gc] GC(8) Concurrent marking roots 12.309ms
>> [1.754s][info][gc] GC(9) Concurrent marking roots 12.996ms
>> [1.879s][info][gc] GC(10) Concurrent marking roots 9.416ms
> 
> **Issue**
> https://bugs.openjdk.org/browse/JDK-8331911

I like this. Some stylistic comments.

src/hotspot/share/gc/shenandoah/shenandoahBarrierSetNMethod.cpp line 2:

> 1: /*
> 2:  * Copyright (c) 2019, 2024, Red Hat, Inc. All rights reserved.

This update is unnecessary.

src/hotspot/share/gc/shenandoah/shenandoahBarrierSetNMethod.cpp line 39:

> 37: 
> 38: bool ShenandoahBarrierSetNMethod::nmethod_entry_barrier(nmethod* nm) {
> 39: 

Here and later: no need for new line at the beginning of the method.

src/hotspot/share/gc/shenandoah/shenandoahBarrierSetNMethod.cpp line 42:

> 40:   if (!is_armed(nm)) {
> 41:     // Some other thread got here first and healed the oops
> 42:     // and disarmed the nmethod.

Suggestion for the comment (here and later):


    // Some other thread got here first and healed the oops
    // and disarmed the nmethod. No need to continue.


...and then later, under the lock:


    // Some other thread managed to complete while we were
    // waiting for lock. No need to continue.

src/hotspot/share/gc/z/zBarrierSetNMethod.cpp line 42:

> 40: 
> 41:   if (!is_armed(nm)) {
> 42:     log_develop_trace(gc, nmethod)("nmethod: " PTR_FORMAT " visited by entry (disarmed)", p2i(nm));

Should it be "(disarmed before lock)" to disambiguate against "(disarmed)" later?

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

PR Review: https://git.openjdk.org/jdk/pull/19285#pullrequestreview-2070558538
PR Review Comment: https://git.openjdk.org/jdk/pull/19285#discussion_r1609570657
PR Review Comment: https://git.openjdk.org/jdk/pull/19285#discussion_r1609563104
PR Review Comment: https://git.openjdk.org/jdk/pull/19285#discussion_r1609578850
PR Review Comment: https://git.openjdk.org/jdk/pull/19285#discussion_r1609571647