# HG changeset patch # User lucy # Date 1556003621 -7200 # Node ID cf5e857f34339e6d5953fc1de38d842be39f32eb # Parent f203906d0dde14ed8f44bb0dd8fc320b726bdc7e 8219586: CodeHeap State Analytics processes dead nmethods Reviewed-by: diff --git a/src/hotspot/share/code/codeHeapState.cpp b/src/hotspot/share/code/codeHeapState.cpp --- a/src/hotspot/share/code/codeHeapState.cpp +++ b/src/hotspot/share/code/codeHeapState.cpp @@ -753,22 +753,23 @@ CodeBlob* cb = (CodeBlob*)heap->find_start(h); if (cb != NULL) { cbType = get_cbType(cb); - if (cb->is_nmethod()) { - compile_id = ((nmethod*)cb)->compile_id(); - comp_lvl = (CompLevel)((nmethod*)cb)->comp_level(); - if (((nmethod*)cb)->is_compiled_by_c1()) { + nmethod* nm = cb->as_nmethod_or_null(); + if (nm != NULL) { // no is_readable check required, nm = (nmethod*)cb. + compile_id = nm->compile_id(); + comp_lvl = (CompLevel)(nm->comp_level()); + if (nm->is_compiled_by_c1()) { cType = c1; } - if (((nmethod*)cb)->is_compiled_by_c2()) { + if (nm->is_compiled_by_c2()) { cType = c2; } - if (((nmethod*)cb)->is_compiled_by_jvmci()) { + if (nm->is_compiled_by_jvmci()) { cType = jvmci; } switch (cbType) { case nMethod_inuse: { // only for executable methods!!! // space for these cbs is accounted for later. - int temperature = ((nmethod*)cb)->hotness_counter(); + int temperature = nm->hotness_counter(); hotnessAccumulator += temperature; n_methods++; maxTemp = (temperature > maxTemp) ? temperature : maxTemp; @@ -1375,7 +1376,7 @@ //---< print size, name, and signature (for nMethods) >--- // access nmethod and Method fields only if we own the CodeCache_lock. // This fact is implicitly transported via nm != NULL. - if (CompiledMethod::nmethod_access_is_safe(nm)) { + if (nmethod::access_is_safe(nm)) { ResourceMark rm; Method* method = nm->method(); if (nm->is_in_use()) { @@ -2281,7 +2282,7 @@ // access nmethod and Method fields only if we own the CodeCache_lock. // This fact is implicitly transported via nm != NULL. - if (CompiledMethod::nmethod_access_is_safe(nm)) { + if (nmethod::access_is_safe(nm)) { Method* method = nm->method(); ResourceMark rm; //---< collect all data to locals as quickly as possible >--- diff --git a/src/hotspot/share/code/compiledMethod.cpp b/src/hotspot/share/code/compiledMethod.cpp --- a/src/hotspot/share/code/compiledMethod.cpp +++ b/src/hotspot/share/code/compiledMethod.cpp @@ -619,21 +619,6 @@ return true; } -// Iterating over all nmethods, e.g. with the help of CodeCache::nmethods_do(fun) was found -// to not be inherently safe. There is a chance that fields are seen which are not properly -// initialized. This happens despite the fact that nmethods_do() asserts the CodeCache_lock -// to be held. -// To bundle knowledge about necessary checks in one place, this function was introduced. -// It is not claimed that these checks are sufficient, but they were found to be necessary. -bool CompiledMethod::nmethod_access_is_safe(nmethod* nm) { - Method* method = (nm == NULL) ? NULL : nm->method(); // nm->method() may be uninitialized, i.e. != NULL, but invalid - return (nm != NULL) && (method != NULL) && (method->signature() != NULL) && - !nm->is_zombie() && !nm->is_not_installed() && - os::is_readable_pointer(method) && - os::is_readable_pointer(method->constants()) && - os::is_readable_pointer(method->signature()); -} - class HasEvolDependency : public MetadataClosure { bool _has_evol_dependency; public: diff --git a/src/hotspot/share/code/compiledMethod.hpp b/src/hotspot/share/code/compiledMethod.hpp --- a/src/hotspot/share/code/compiledMethod.hpp +++ b/src/hotspot/share/code/compiledMethod.hpp @@ -255,8 +255,6 @@ return _mark_for_deoptimization_status != deoptimize_noupdate; } - static bool nmethod_access_is_safe(nmethod* nm); - // tells whether frames described by this nmethod can be deoptimized // note: native wrappers cannot be deoptimized. bool can_be_deoptimized() const { return is_java_method(); } diff --git a/src/hotspot/share/code/nmethod.cpp b/src/hotspot/share/code/nmethod.cpp --- a/src/hotspot/share/code/nmethod.cpp +++ b/src/hotspot/share/code/nmethod.cpp @@ -845,6 +845,24 @@ #undef LOG_OFFSET +// Iterating over all nmethods, e.g. with the help of CodeCache::nmethods_do(fun) was found +// to not be inherently safe. There is a chance that fields are seen which are not properly +// initialized. This happens despite the fact that nmethods_do() asserts the CodeCache_lock +// to be held. +// To bundle knowledge about necessary checks in one place, this function was introduced. +// It is not claimed that these checks are sufficient, but they were found to be necessary. +bool nmethod::access_is_safe(nmethod* nm) { + Method* method = (nm == NULL) ? NULL : nm->method(); // nm->method() may be uninitialized, i.e. != NULL, but invalid + return (nm != NULL) && (method != NULL) && + nm->is_alive() && !nm->is_not_installed() && + os::is_readable_pointer(method) && + os::is_readable_pointer(method->constMethod()) && + os::is_readable_pointer(method->constants()) && + (method->signature() != NULL) && + os::is_readable_pointer(method->signature()); +} + + // Print out more verbose output usually for a newly created nmethod. void nmethod::print_on(outputStream* st, const char* msg) const { if (st != NULL) { diff --git a/src/hotspot/share/code/nmethod.hpp b/src/hotspot/share/code/nmethod.hpp --- a/src/hotspot/share/code/nmethod.hpp +++ b/src/hotspot/share/code/nmethod.hpp @@ -514,6 +514,9 @@ void verify_scopes(); void verify_interrupt_point(address interrupt_point); + // verify that accessing object fields (nmethod and parent) is safe. + static bool access_is_safe(nmethod* nm); + // printing support void print() const; void print_relocations() PRODUCT_RETURN; diff --git a/src/hotspot/share/compiler/compileBroker.cpp b/src/hotspot/share/compiler/compileBroker.cpp --- a/src/hotspot/share/compiler/compileBroker.cpp +++ b/src/hotspot/share/compiler/compileBroker.cpp @@ -2678,33 +2678,44 @@ // CodeHeapStateAnalytics_lock could be held by a concurrent thread for a long time, // leading to an unnecessarily long hold time of the CodeCache_lock. ts.update(); // record starting point - MutexLockerEx mu1(CodeHeapStateAnalytics_lock, Mutex::_no_safepoint_check_flag); + MutexLockerEx mu0(CodeHeapStateAnalytics_lock, Mutex::_no_safepoint_check_flag); out->print_cr("\n__ CodeHeapStateAnalytics lock wait took %10.3f seconds _________\n", ts.seconds()); + // Unfortunately, it is not sufficient to only hold the CodeCache_lock. + // When a new nmethod is created via ciEnv::register_method(), the + // Compile_lock is taken first. After some initializations, + // nmethod::new_nmethod() takes over, grabbing the CodeCache_lock + // immediately (after finalizing the oop references). To lock out concurrent + // modifiers, we have to grab both locks as well in the described sequence. + // // If we serve an "allFun" call, it is beneficial to hold the CodeCache_lock // for the entire duration of aggregation and printing. That makes sure // we see a consistent picture and do not run into issues caused by // the CodeHeap being altered concurrently. - Monitor* global_lock = allFun ? CodeCache_lock : NULL; - Monitor* function_lock = allFun ? NULL : CodeCache_lock; + Monitor* global_lock_1 = allFun ? Compile_lock : NULL; + Monitor* global_lock_2 = allFun ? CodeCache_lock : NULL; + Monitor* function_lock_1 = allFun ? NULL : Compile_lock; + Monitor* function_lock_2 = allFun ? NULL : CodeCache_lock; ts_global.update(); // record starting point - MutexLockerEx mu2(global_lock, Mutex::_no_safepoint_check_flag); - if (global_lock != NULL) { - out->print_cr("\n__ CodeCache (global) lock wait took %10.3f seconds _________\n", ts_global.seconds()); + MutexLockerEx mu1(global_lock_1); + MutexLockerEx mu2(global_lock_2, Mutex::_no_safepoint_check_flag); + if (global_lock_1 != NULL) { + out->print_cr("\n__ Compile & CodeCache (global) lock wait took %10.3f seconds _________\n", ts_global.seconds()); ts_global.update(); // record starting point } if (aggregate) { ts.update(); // record starting point - MutexLockerEx mu3(function_lock, Mutex::_no_safepoint_check_flag); - if (function_lock != NULL) { - out->print_cr("\n__ CodeCache (function) lock wait took %10.3f seconds _________\n", ts.seconds()); + MutexLockerEx mu11(function_lock_1, Mutex::_no_safepoint_check_flag); + MutexLockerEx mu22(function_lock_2, Mutex::_no_safepoint_check_flag); + if (function_lock_1 != NULL) { + out->print_cr("\n__ Compile & CodeCache (function) lock wait took %10.3f seconds _________\n", ts.seconds()); } ts.update(); // record starting point CodeCache::aggregate(out, granularity); - if (function_lock != NULL) { - out->print_cr("\n__ CodeCache (function) lock hold took %10.3f seconds _________\n", ts.seconds()); + if (function_lock_1 != NULL) { + out->print_cr("\n__ Compile & CodeCache (function) lock hold took %10.3f seconds _________\n", ts.seconds()); } } @@ -2716,13 +2727,14 @@ if (methodNames) { // print_names() has shown to be sensitive to concurrent CodeHeap modifications. // Therefore, request the CodeCache_lock before calling... - MutexLockerEx mu3(function_lock, Mutex::_no_safepoint_check_flag); + MutexLockerEx mu11(function_lock_1, Mutex::_no_safepoint_check_flag); + MutexLockerEx mu22(function_lock_2, Mutex::_no_safepoint_check_flag); CodeCache::print_names(out); } if (discard) CodeCache::discard(out); - if (global_lock != NULL) { - out->print_cr("\n__ CodeCache (global) lock hold took %10.3f seconds _________\n", ts_global.seconds()); + if (global_lock_1 != NULL) { + out->print_cr("\n__ Compile & CodeCache (global) lock hold took %10.3f seconds _________\n", ts_global.seconds()); } out->print_cr("\n__ CodeHeapStateAnalytics total duration %10.3f seconds _________\n", ts_total.seconds()); } diff --git a/src/hotspot/share/runtime/mutexLocker.cpp b/src/hotspot/share/runtime/mutexLocker.cpp --- a/src/hotspot/share/runtime/mutexLocker.cpp +++ b/src/hotspot/share/runtime/mutexLocker.cpp @@ -339,7 +339,7 @@ def(UnsafeJlong_lock , PaddedMutex , special, false, Monitor::_safepoint_check_never); #endif - def(CodeHeapStateAnalytics_lock , PaddedMutex , leaf, true, Monitor::_safepoint_check_never); + def(CodeHeapStateAnalytics_lock , PaddedMutex , nonleaf+6, true, Monitor::_safepoint_check_never); def(NMethodSweeperStats_lock , PaddedMutex , special, true, Monitor::_safepoint_check_sometimes); def(ThreadsSMRDelete_lock , PaddedMonitor, special, false, Monitor::_safepoint_check_never); diff --git a/src/hotspot/share/runtime/sharedRuntime.cpp b/src/hotspot/share/runtime/sharedRuntime.cpp --- a/src/hotspot/share/runtime/sharedRuntime.cpp +++ b/src/hotspot/share/runtime/sharedRuntime.cpp @@ -2214,7 +2214,7 @@ static int _max_size; // max. arg size seen static void add_method_to_histogram(nmethod* nm) { - if (CompiledMethod::nmethod_access_is_safe(nm)) { + if (nmethod::access_is_safe(nm)) { Method* method = nm->method(); ArgumentCount args(method->signature()); int arity = args.size() + (method->is_static() ? 0 : 1);