# HG changeset patch # User coleenp # Date 1562759904 14400 # Node ID 0fb70c9118ce3a295f89c9cf79f27cf92e574b02 # Parent 0f1e29c77e50c7da11d83df410026392c4d1a28c 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 diff -r 0f1e29c77e50 -r 0fb70c9118ce src/hotspot/share/ci/ciEnv.cpp --- 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; diff -r 0f1e29c77e50 -r 0fb70c9118ce src/hotspot/share/ci/ciEnv.hpp --- 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); diff -r 0f1e29c77e50 -r 0fb70c9118ce src/hotspot/share/classfile/systemDictionary.cpp --- 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); diff -r 0f1e29c77e50 -r 0fb70c9118ce src/hotspot/share/classfile/systemDictionary.hpp --- 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; diff -r 0f1e29c77e50 -r 0fb70c9118ce src/hotspot/share/code/dependencies.cpp --- 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; } diff -r 0f1e29c77e50 -r 0fb70c9118ce src/hotspot/share/code/dependencies.hpp --- 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(); diff -r 0f1e29c77e50 -r 0fb70c9118ce src/hotspot/share/compiler/compileBroker.cpp --- 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); } diff -r 0f1e29c77e50 -r 0fb70c9118ce src/hotspot/share/jvmci/jvmciEnv.cpp --- 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) { diff -r 0f1e29c77e50 -r 0fb70c9118ce src/hotspot/share/jvmci/jvmciEnv.hpp --- 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; } diff -r 0f1e29c77e50 -r 0fb70c9118ce src/hotspot/share/jvmci/jvmciRuntime.cpp --- 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 diff -r 0f1e29c77e50 -r 0fb70c9118ce src/hotspot/share/oops/method.cpp --- 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); diff -r 0f1e29c77e50 -r 0fb70c9118ce src/hotspot/share/opto/callGenerator.cpp --- 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()) { diff -r 0f1e29c77e50 -r 0fb70c9118ce src/hotspot/share/opto/parse1.cpp --- 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()) { diff -r 0f1e29c77e50 -r 0fb70c9118ce src/hotspot/share/prims/jvmtiRedefineClasses.cpp --- 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(); diff -r 0f1e29c77e50 -r 0fb70c9118ce src/hotspot/share/runtime/vmStructs.cpp --- 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*) \ diff -r 0f1e29c77e50 -r 0fb70c9118ce src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/ci/ciEnv.java --- 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);