On Tue, 2 Nov 2021 16:34:34 GMT, Volker Simonis <simo...@openjdk.org> wrote:
>> This PR changes nmethods names in `METHOD NAMES for CodeHeap` section to be >> qualified. >> Testing: >> - `make test TEST="gtest"`: Passed >> - `make run-test TEST="tier1"`: Passed >> - `make run-test TEST="tier2"`: Passed >> - `make run-test >> TEST=serviceability/dcmd/compiler/CodeHeapAnalyticsMethodNames.java`: Passed > > src/hotspot/share/code/codeHeapState.cpp line 2340: > >> 2338: >> 2339: Klass* klass = method->method_holder(); >> 2340: assert(klass->is_loader_alive(), "must be alive"); > > Are you sure `klass` is always valid here and that its class loader has to be > alive (i.e. the corresponding class hasn't been unloaded in the meantime)? > > In [https://bugs.openjdk.java.net/browse/JDK-8275729](JDK-8275729) you say > that the Top50 list already has qualified names but as far as I know, that > information is already collected in the aggregation step where it is safe. > You now query this information in the reporting step. > > I know we had problems due to access to dead methods before (see > [JDK-8219586: CodeHeap State Analytics processes dead > nmethods](https://bugs.openjdk.java.net/browse/JDK-8219586) and I just want > to make sure we don't re-introduce such problems. > > Maybe @RealLucy or @fisk can have an additional look? @simonis The code is guarded by checks: // access nmethod and Method fields only if we own the CodeCache_lock. // This fact is implicitly transported via nm != NULL. if (nmethod_access_is_safe(nm)) { ... bool get_name = (cbType == nMethod_inuse) || (cbType == nMethod_notused); ... if (get_name) { I was thinking whether I should use `if (klass->is_loader_alive())` or `assert(klass->is_loader_alive())`. I chose the assert because if it is safe to access `Method` than its holder `Klass` must be alive. ------------- PR: https://git.openjdk.java.net/jdk/pull/6200
