8222446: assert(C->env()->system_dictionary_modification_counter_changed()) failed: Must invalidate if TypeFuncs differ

Remove SystemDictionary::modification_counter optimization

Reviewed-by: dlong, eosterlund

src/hotspot/share/ci/ciEnv.cpp

@@ -98,7 +98,7 @@ static bool firstEnv = true;
 
 // ------------------------------------------------------------------
 // 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 @@ ciEnv::ciEnv(CompileTask* task, int system_dictionary_modification_counter)
   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 @@ ciEnv::ciEnv(Arena* arena) : _ciEnv_arena(mtCompiler) {
   firstEnv = false;
 #endif /* !PRODUCT */
 
-  _system_dictionary_modification_counter = 0;
   _num_inlined_bytecodes = 0;
   _task = NULL;
   _log = NULL;
@@ -919,17 +917,6 @@ bool ciEnv::is_in_vm() {
   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 @@ bool ciEnv::system_dictionary_modification_counter_changed() {
 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);
-  Dependencies::DepType result = dependencies()->validate_dependencies(_task, counter_changed);
   if (result != Dependencies::end_marker) {
     if (result == Dependencies::call_site_target_value) {
       _inc_decompile_count_on_failure = false;

src/hotspot/share/ci/ciEnv.hpp

@@ -51,7 +51,6 @@ class ciEnv : StackObj {
 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 @@ private:
   // 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 @@ public:
     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 @@ public:
   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);

src/hotspot/share/classfile/systemDictionary.cpp

@@ -98,7 +98,6 @@ ResolutionErrorTable*  SystemDictionary::_resolution_errors   = NULL;
 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 @@ InstanceKlass* SystemDictionary::parse_stream(Symbol* class_name,
       // 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::methods_do(void f(Method*)) {
 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 @@ void SystemDictionary::update_dictionary(unsigned int d_hash,
     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);

src/hotspot/share/classfile/systemDictionary.hpp

@@ -362,13 +362,6 @@ public:
   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 @@ public:
   // 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;
 

src/hotspot/share/code/dependencies.cpp

@@ -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 @@ void Dependencies::check_valid_dependency_type(DepType dept) {
   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) {
+Dependencies::DepType Dependencies::validate_dependencies(CompileTask* task, 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;
-  }
-
   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 @@ Dependencies::DepType Dependencies::validate_dependencies(CompileTask* task, boo
         }
       }
       klass_violations++;
-      if (!counter_changed) {
+      if (xtty == NULL) {
-        // 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 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 @@ Dependencies::DepType Dependencies::validate_dependencies(CompileTask* task, boo
     }
   }
 
-  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;
 }
 

src/hotspot/share/code/dependencies.hpp

@@ -476,7 +476,7 @@ class Dependencies: public ResourceObj {
 
   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();
 

src/hotspot/share/compiler/compileBroker.cpp

@@ -1595,16 +1595,10 @@ bool CompileBroker::init_compiler_runtime() {
   // 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 @@ void CompileBroker::invoke_compiler_on_method(CompileTask* task) {
   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 @@ void CompileBroker::invoke_compiler_on_method(CompileTask* task) {
       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 @@ void CompileBroker::invoke_compiler_on_method(CompileTask* task) {
     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);
     }

src/hotspot/share/jvmci/jvmciEnv.cpp

@@ -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) {

src/hotspot/share/jvmci/jvmciEnv.hpp

@@ -90,7 +90,6 @@ class JVMCICompileState : public ResourceObj {
   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 @@ class JVMCICompileState : public ResourceObj {
   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; }

src/hotspot/share/jvmci/jvmciRuntime.cpp

@@ -1327,14 +1327,13 @@ JVMCI::CodeInstallResult JVMCIRuntime::validate_compile_task_dependencies(Depend
 
   // 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

src/hotspot/share/oops/method.cpp

@@ -1920,7 +1920,6 @@ void BreakpointInfo::set(Method* method) {
   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);

src/hotspot/share/opto/callGenerator.cpp

@@ -96,13 +96,6 @@ JVMState* ParseGenerator::generate(JVMState* jvms) {
 
   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()) {

src/hotspot/share/opto/parse1.cpp

@@ -523,10 +523,6 @@ Parse::Parse(JVMState* caller, ciMethod* parse_method, float expected_uses)
 #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 @@ void Parse::do_exits() {
     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
+      // If the type we set for the ret_phi in build_exits() is too optimistic and
-      // ret_phi in build_exits() may have been too optimistic and the
+      // the ret_phi is top now, there's an extremely small chance that it may be due to class
-      // ret_phi may be top now.
+      // loading.  It could also be due to an error, so mark this method as not compilable because
-      // Otherwise, we've encountered an error and have to mark the method as
+      // otherwise this could lead to an infinite compile loop.
-      // not compilable. Just using an assertion instead would be dangerous
+      // In any case, this code path is rarely (and never in my testing) reached.
-      // as this could lead to an infinite compile loop in non-debug builds.
+      C->record_method_not_compilable("Can't determine return type.");
-      {
-        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.");
-        }
-      }
       return;
     }
     if (ret_type->isa_int()) {

src/hotspot/share/prims/jvmtiRedefineClasses.cpp

@@ -232,9 +232,6 @@ void VM_RedefineClasses::doit() {
     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();

src/hotspot/share/runtime/vmStructs.cpp

@@ -840,7 +840,6 @@ typedef PaddedEnd<ObjectMonitor>              PaddedObjectMonitor;
   /* 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*)                      \

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/ci/ciEnv.java

@@ -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 @@ public class ciEnv extends VMObject {
     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);