On Tue, 2 Nov 2021 17:03:50 GMT, Evgeny Astigeevich <d...@openjdk.java.net> wrote:
>> 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. Hi, the code is safe. Not because of the checks cited by @eastig but because print_names() is only called if the required locks (Compile_lock and CodeCache_lock) have been continuously held since the aggregation step. See src/hotspot/share/compiler/compileBroker.cpp. A lot of effort has been spent to be less restrictive on print_names(), with no success. Thanks for the enhancement. ------------- PR: https://git.openjdk.java.net/jdk/pull/6200
