8222446: assert(C->env()->system_dictionary_modification_counter_changed()) failed: Must invalidate if TypeFuncs differ
Summary: Remove SystemDictionary::modification_counter optimization
Reviewed-by: dlong, eosterlund
--- a/src/hotspot/share/ci/ciEnv.cpp Wed Jul 10 05:12:23 2019 +0100
+++ b/src/hotspot/share/ci/ciEnv.cpp Wed Jul 10 07:58:24 2019 -0400
@@ -98,7 +98,7 @@
// ------------------------------------------------------------------
// ciEnv::ciEnv
-ciEnv::ciEnv(CompileTask* task, int system_dictionary_modification_counter)
+ciEnv::ciEnv(CompileTask* task)
: _ciEnv_arena(mtCompiler) {
VM_ENTRY_MARK;
@@ -118,7 +118,6 @@
assert(!firstEnv, "not initialized properly");
#endif /* !PRODUCT */
- _system_dictionary_modification_counter = system_dictionary_modification_counter;
_num_inlined_bytecodes = 0;
assert(task == NULL || thread->task() == task, "sanity");
if (task != NULL) {
@@ -183,7 +182,6 @@
firstEnv = false;
#endif /* !PRODUCT */
- _system_dictionary_modification_counter = 0;
_num_inlined_bytecodes = 0;
_task = NULL;
_log = NULL;
@@ -919,17 +917,6 @@
return JavaThread::current()->thread_state() == _thread_in_vm;
}
-bool ciEnv::system_dictionary_modification_counter_changed_locked() {
- assert_locked_or_safepoint(Compile_lock);
- return _system_dictionary_modification_counter != SystemDictionary::number_of_modifications();
-}
-
-bool ciEnv::system_dictionary_modification_counter_changed() {
- VM_ENTRY_MARK;
- MutexLocker ml(Compile_lock, THREAD); // lock with safepoint check
- return system_dictionary_modification_counter_changed_locked();
-}
-
// ------------------------------------------------------------------
// ciEnv::validate_compile_task_dependencies
//
@@ -938,8 +925,7 @@
void ciEnv::validate_compile_task_dependencies(ciMethod* target) {
if (failing()) return; // no need for further checks
- bool counter_changed = system_dictionary_modification_counter_changed_locked();
- Dependencies::DepType result = dependencies()->validate_dependencies(_task, counter_changed);
+ Dependencies::DepType result = dependencies()->validate_dependencies(_task);
if (result != Dependencies::end_marker) {
if (result == Dependencies::call_site_target_value) {
_inc_decompile_count_on_failure = false;
--- a/src/hotspot/share/ci/ciEnv.hpp Wed Jul 10 05:12:23 2019 +0100
+++ b/src/hotspot/share/ci/ciEnv.hpp Wed Jul 10 07:58:24 2019 -0400
@@ -51,7 +51,6 @@
private:
Arena* _arena; // Alias for _ciEnv_arena except in init_shared_objects()
Arena _ciEnv_arena;
- int _system_dictionary_modification_counter;
ciObjectFactory* _factory;
OopRecorder* _oop_recorder;
DebugInformationRecorder* _debug_info;
@@ -291,9 +290,6 @@
// Helper routine for determining the validity of a compilation with
// respect to method dependencies (e.g. concurrent class loading).
void validate_compile_task_dependencies(ciMethod* target);
-
- // Call internally when Compile_lock is already held.
- bool system_dictionary_modification_counter_changed_locked();
public:
enum {
MethodCompilable,
@@ -301,7 +297,7 @@
MethodCompilable_never
};
- ciEnv(CompileTask* task, int system_dictionary_modification_counter);
+ ciEnv(CompileTask* task);
// Used only during initialization of the ci
ciEnv(Arena* arena);
~ciEnv();
@@ -456,9 +452,6 @@
CompileLog* log() { return _log; }
void set_log(CompileLog* log) { _log = log; }
- // Check for changes to the system dictionary during compilation
- bool system_dictionary_modification_counter_changed();
-
void record_failure(const char* reason); // Record failure and report later
void report_failure(const char* reason); // Report failure immediately
void record_method_not_compilable(const char* reason, bool all_tiers = true);
--- a/src/hotspot/share/classfile/systemDictionary.cpp Wed Jul 10 05:12:23 2019 +0100
+++ b/src/hotspot/share/classfile/systemDictionary.cpp Wed Jul 10 07:58:24 2019 -0400
@@ -98,7 +98,6 @@
SymbolPropertyTable* SystemDictionary::_invoke_method_table = NULL;
ProtectionDomainCacheTable* SystemDictionary::_pd_cache_table = NULL;
-int SystemDictionary::_number_of_modifications = 0;
oop SystemDictionary::_system_loader_lock_obj = NULL;
InstanceKlass* SystemDictionary::_well_known_klasses[SystemDictionary::WKID_LIMIT]
@@ -1039,11 +1038,7 @@
// Add to class hierarchy, initialize vtables, and do possible
// deoptimizations.
add_to_hierarchy(k, CHECK_NULL); // No exception, but can block
-
// But, do not add to dictionary.
-
- // compiled code dependencies need to be validated anyway
- notice_modification();
}
// Rewrite and patch constant pool here.
@@ -1880,7 +1875,6 @@
void SystemDictionary::initialize(TRAPS) {
// Allocate arrays
_placeholders = new PlaceholderTable(_placeholder_table_size);
- _number_of_modifications = 0;
_loader_constraints = new LoaderConstraintTable(_loader_constraint_size);
_resolution_errors = new ResolutionErrorTable(_resolution_error_size);
_invoke_method_table = new SymbolPropertyTable(_invoke_method_size);
@@ -2164,8 +2158,6 @@
InstanceKlass* sd_check = find_class(d_hash, name, dictionary);
if (sd_check == NULL) {
dictionary->add_klass(d_hash, name, k);
-
- notice_modification();
}
#ifdef ASSERT
sd_check = find_class(d_hash, name, dictionary);
--- a/src/hotspot/share/classfile/systemDictionary.hpp Wed Jul 10 05:12:23 2019 +0100
+++ b/src/hotspot/share/classfile/systemDictionary.hpp Wed Jul 10 07:58:24 2019 -0400
@@ -362,13 +362,6 @@
static void print_on(outputStream* st);
static void dump(outputStream* st, bool verbose);
- // Monotonically increasing counter which grows as classes are
- // loaded or modifications such as hot-swapping or setting/removing
- // of breakpoints are performed
- static inline int number_of_modifications() { assert_locked_or_safepoint(Compile_lock); return _number_of_modifications; }
- // Needed by evolution and breakpoint code
- static inline void notice_modification() { assert_locked_or_safepoint(Compile_lock); ++_number_of_modifications; }
-
// Verification
static void verify();
@@ -555,11 +548,6 @@
// Hashtable holding placeholders for classes being loaded.
static PlaceholderTable* _placeholders;
- // Monotonically increasing counter which grows with
- // loading classes as well as hot-swapping and breakpoint setting
- // and removal.
- static int _number_of_modifications;
-
// Lock object for system class loader
static oop _system_loader_lock_obj;
--- a/src/hotspot/share/code/dependencies.cpp Wed Jul 10 05:12:23 2019 +0100
+++ b/src/hotspot/share/code/dependencies.cpp Wed Jul 10 07:58:24 2019 -0400
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2005, 2018, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2005, 2019, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -627,32 +627,10 @@
guarantee(FIRST_TYPE <= dept && dept < TYPE_LIMIT, "invalid dependency type: %d", (int) dept);
}
-Dependencies::DepType Dependencies::validate_dependencies(CompileTask* task, bool counter_changed, char** failure_detail) {
- // First, check non-klass dependencies as we might return early and
- // not check klass dependencies if the system dictionary
- // modification counter hasn't changed (see below).
- for (Dependencies::DepStream deps(this); deps.next(); ) {
- if (deps.is_klass_type()) continue; // skip klass dependencies
- Klass* witness = deps.check_dependency();
- if (witness != NULL) {
- return deps.type();
- }
- }
-
- // Klass dependencies must be checked when the system dictionary
- // changes. If logging is enabled all violated dependences will be
- // recorded in the log. In debug mode check dependencies even if
- // the system dictionary hasn't changed to verify that no invalid
- // dependencies were inserted. Any violated dependences in this
- // case are dumped to the tty.
- if (!counter_changed && !trueInDebug) {
- return end_marker;
- }
-
+Dependencies::DepType Dependencies::validate_dependencies(CompileTask* task, char** failure_detail) {
int klass_violations = 0;
DepType result = end_marker;
for (Dependencies::DepStream deps(this); deps.next(); ) {
- if (!deps.is_klass_type()) continue; // skip non-klass dependencies
Klass* witness = deps.check_dependency();
if (witness != NULL) {
if (klass_violations == 0) {
@@ -667,12 +645,7 @@
}
}
klass_violations++;
- if (!counter_changed) {
- // Dependence failed but counter didn't change. Log a message
- // describing what failed and allow the assert at the end to
- // trigger.
- deps.print_dependency(witness);
- } else if (xtty == NULL) {
+ if (xtty == NULL) {
// If we're not logging then a single violation is sufficient,
// otherwise we want to log all the dependences which were
// violated.
@@ -681,15 +654,6 @@
}
}
- if (klass_violations != 0) {
-#ifdef ASSERT
- if (task != NULL && !counter_changed && !PrintCompilation) {
- // Print out the compile task that failed
- task->print_tty();
- }
-#endif
- assert(counter_changed, "failed dependencies, but counter didn't change");
- }
return result;
}
--- a/src/hotspot/share/code/dependencies.hpp Wed Jul 10 05:12:23 2019 +0100
+++ b/src/hotspot/share/code/dependencies.hpp Wed Jul 10 07:58:24 2019 -0400
@@ -476,7 +476,7 @@
void copy_to(nmethod* nm);
- DepType validate_dependencies(CompileTask* task, bool counter_changed, char** failure_detail = NULL);
+ DepType validate_dependencies(CompileTask* task, char** failure_detail = NULL);
void log_all_dependencies();
--- a/src/hotspot/share/compiler/compileBroker.cpp Wed Jul 10 05:12:23 2019 +0100
+++ b/src/hotspot/share/compiler/compileBroker.cpp Wed Jul 10 07:58:24 2019 -0400
@@ -1595,16 +1595,10 @@
// Final sanity check - the compiler object must exist
guarantee(comp != NULL, "Compiler object must exist");
- int system_dictionary_modification_counter;
- {
- MutexLocker locker(Compile_lock, thread);
- system_dictionary_modification_counter = SystemDictionary::number_of_modifications();
- }
-
{
// Must switch to native to allocate ci_env
ThreadToNativeFromVM ttn(thread);
- ciEnv ci_env(NULL, system_dictionary_modification_counter);
+ ciEnv ci_env((CompileTask*)NULL);
// Cache Jvmti state
ci_env.cache_jvmti_state();
// Cache DTrace flags
@@ -2045,12 +2039,6 @@
bool failure_reason_on_C_heap = false;
const char* retry_message = NULL;
- int system_dictionary_modification_counter;
- {
- MutexLocker locker(Compile_lock, thread);
- system_dictionary_modification_counter = SystemDictionary::number_of_modifications();
- }
-
#if INCLUDE_JVMCI
if (UseJVMCICompiler && comp != NULL && comp->is_jvmci()) {
JVMCICompiler* jvmci = (JVMCICompiler*) comp;
@@ -2064,7 +2052,7 @@
retry_message = "not retryable";
compilable = ciEnv::MethodCompilable_never;
} else {
- JVMCICompileState compile_state(task, system_dictionary_modification_counter);
+ JVMCICompileState compile_state(task);
JVMCIEnv env(thread, &compile_state, __FILE__, __LINE__);
methodHandle method(thread, target_handle);
env.runtime()->compile_method(&env, jvmci, method, osr_bci);
@@ -2090,7 +2078,7 @@
NoHandleMark nhm;
ThreadToNativeFromVM ttn(thread);
- ciEnv ci_env(task, system_dictionary_modification_counter);
+ ciEnv ci_env(task);
if (should_break) {
ci_env.set_break_at_compile(true);
}
--- a/src/hotspot/share/jvmci/jvmciEnv.cpp Wed Jul 10 05:12:23 2019 +0100
+++ b/src/hotspot/share/jvmci/jvmciEnv.cpp Wed Jul 10 07:58:24 2019 -0400
@@ -36,9 +36,8 @@
#include "jvmci/jniAccessMark.inline.hpp"
#include "jvmci/jvmciRuntime.hpp"
-JVMCICompileState::JVMCICompileState(CompileTask* task, int system_dictionary_modification_counter):
+JVMCICompileState::JVMCICompileState(CompileTask* task):
_task(task),
- _system_dictionary_modification_counter(system_dictionary_modification_counter),
_retryable(true),
_failure_reason(NULL),
_failure_reason_on_C_heap(false) {
--- a/src/hotspot/share/jvmci/jvmciEnv.hpp Wed Jul 10 05:12:23 2019 +0100
+++ b/src/hotspot/share/jvmci/jvmciEnv.hpp Wed Jul 10 07:58:24 2019 -0400
@@ -90,7 +90,6 @@
friend class JVMCIVMStructs;
private:
CompileTask* _task;
- int _system_dictionary_modification_counter;
// Cache JVMTI state. Defined as bytes so that reading them from Java
// via Unsafe is well defined (the C++ type for bool is implementation
@@ -109,11 +108,10 @@
bool _failure_reason_on_C_heap;
public:
- JVMCICompileState(CompileTask* task, int system_dictionary_modification_counter);
+ JVMCICompileState(CompileTask* task);
CompileTask* task() { return _task; }
- int system_dictionary_modification_counter() { return _system_dictionary_modification_counter; }
bool jvmti_state_changed() const;
bool jvmti_can_hotswap_or_post_breakpoint() const { return _jvmti_can_hotswap_or_post_breakpoint != 0; }
bool jvmti_can_access_local_variables() const { return _jvmti_can_access_local_variables != 0; }
--- a/src/hotspot/share/jvmci/jvmciRuntime.cpp Wed Jul 10 05:12:23 2019 +0100
+++ b/src/hotspot/share/jvmci/jvmciRuntime.cpp Wed Jul 10 07:58:24 2019 -0400
@@ -1327,14 +1327,13 @@
// Dependencies must be checked when the system dictionary changes
// or if we don't know whether it has changed (i.e., compile_state == NULL).
- bool counter_changed = compile_state == NULL || compile_state->system_dictionary_modification_counter() != SystemDictionary::number_of_modifications();
CompileTask* task = compile_state == NULL ? NULL : compile_state->task();
- Dependencies::DepType result = dependencies->validate_dependencies(task, counter_changed, failure_detail);
+ Dependencies::DepType result = dependencies->validate_dependencies(task, failure_detail);
if (result == Dependencies::end_marker) {
return JVMCI::ok;
}
- if (!Dependencies::is_klass_type(result) || counter_changed) {
+ if (!Dependencies::is_klass_type(result) || compile_state == NULL) {
return JVMCI::dependencies_failed;
}
// The dependencies were invalid at the time of installation
--- a/src/hotspot/share/oops/method.cpp Wed Jul 10 05:12:23 2019 +0100
+++ b/src/hotspot/share/oops/method.cpp Wed Jul 10 07:58:24 2019 -0400
@@ -1920,7 +1920,6 @@
Thread *thread = Thread::current();
*method->bcp_from(_bci) = Bytecodes::_breakpoint;
method->incr_number_of_breakpoints(thread);
- SystemDictionary::notice_modification();
{
// Deoptimize all dependents on this method
HandleMark hm(thread);
--- a/src/hotspot/share/opto/callGenerator.cpp Wed Jul 10 05:12:23 2019 +0100
+++ b/src/hotspot/share/opto/callGenerator.cpp Wed Jul 10 07:58:24 2019 -0400
@@ -96,13 +96,6 @@
Parse parser(jvms, method(), _expected_uses);
// Grab signature for matching/allocation
-#ifdef ASSERT
- if (parser.tf() != (parser.depth() == 1 ? C->tf() : tf())) {
- assert(C->env()->system_dictionary_modification_counter_changed(),
- "Must invalidate if TypeFuncs differ");
- }
-#endif
-
GraphKit& exits = parser.exits();
if (C->failing()) {
--- a/src/hotspot/share/opto/parse1.cpp Wed Jul 10 05:12:23 2019 +0100
+++ b/src/hotspot/share/opto/parse1.cpp Wed Jul 10 07:58:24 2019 -0400
@@ -523,10 +523,6 @@
#ifdef ASSERT
if (depth() == 1) {
assert(C->is_osr_compilation() == this->is_osr_parse(), "OSR in sync");
- if (C->tf() != tf()) {
- assert(C->env()->system_dictionary_modification_counter_changed(),
- "Must invalidate if TypeFuncs differ");
- }
} else {
assert(!this->is_osr_parse(), "no recursive OSR");
}
@@ -1040,19 +1036,12 @@
const Type* ret_type = tf()->range()->field_at(TypeFunc::Parms);
Node* ret_phi = _gvn.transform( _exits.argument(0) );
if (!_exits.control()->is_top() && _gvn.type(ret_phi)->empty()) {
- // In case of concurrent class loading, the type we set for the
- // ret_phi in build_exits() may have been too optimistic and the
- // ret_phi may be top now.
- // Otherwise, we've encountered an error and have to mark the method as
- // not compilable. Just using an assertion instead would be dangerous
- // as this could lead to an infinite compile loop in non-debug builds.
- {
- if (C->env()->system_dictionary_modification_counter_changed()) {
- C->record_failure(C2Compiler::retry_class_loading_during_parsing());
- } else {
- C->record_method_not_compilable("Can't determine return type.");
- }
- }
+ // If the type we set for the ret_phi in build_exits() is too optimistic and
+ // the ret_phi is top now, there's an extremely small chance that it may be due to class
+ // loading. It could also be due to an error, so mark this method as not compilable because
+ // otherwise this could lead to an infinite compile loop.
+ // In any case, this code path is rarely (and never in my testing) reached.
+ C->record_method_not_compilable("Can't determine return type.");
return;
}
if (ret_type->isa_int()) {
--- a/src/hotspot/share/prims/jvmtiRedefineClasses.cpp Wed Jul 10 05:12:23 2019 +0100
+++ b/src/hotspot/share/prims/jvmtiRedefineClasses.cpp Wed Jul 10 07:58:24 2019 -0400
@@ -232,9 +232,6 @@
ResolvedMethodTable::adjust_method_entries(&trace_name_printed);
}
- // Disable any dependent concurrent compilations
- SystemDictionary::notice_modification();
-
// Set flag indicating that some invariants are no longer true.
// See jvmtiExport.hpp for detailed explanation.
JvmtiExport::set_has_redefined_a_class();
--- a/src/hotspot/share/runtime/vmStructs.cpp Wed Jul 10 05:12:23 2019 +0100
+++ b/src/hotspot/share/runtime/vmStructs.cpp Wed Jul 10 07:58:24 2019 -0400
@@ -840,7 +840,6 @@
/* CI */ \
/************/ \
\
- nonstatic_field(ciEnv, _system_dictionary_modification_counter, int) \
nonstatic_field(ciEnv, _compiler_data, void*) \
nonstatic_field(ciEnv, _failure_reason, const char*) \
nonstatic_field(ciEnv, _factory, ciObjectFactory*) \
--- a/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/ci/ciEnv.java Wed Jul 10 05:12:23 2019 +0100
+++ b/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/ci/ciEnv.java Wed Jul 10 07:58:24 2019 -0400
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2011, 2013, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2011, 2019, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -50,14 +50,12 @@
factoryField = type.getAddressField("_factory");
compilerDataField = type.getAddressField("_compiler_data");
taskField = type.getAddressField("_task");
- systemDictionaryModificationCounterField = new CIntField(type.getCIntegerField("_system_dictionary_modification_counter"), 0);
}
private static AddressField dependenciesField;
private static AddressField factoryField;
private static AddressField compilerDataField;
private static AddressField taskField;
- private static CIntField systemDictionaryModificationCounterField;
public ciEnv(Address addr) {
super(addr);