# HG changeset patch # User redestad # Date 1552060779 -3600 # Fri Mar 08 16:59:39 2019 +0100 # Node ID a7c9b525082d4e240b605e11ec17be8cbf200bcd # Parent 6347ffe2c3c7ef750ec92781d89950607bf4b73d 8220366: Optimize Symbol handling in ClassVerifier and SignatureStream Reviewed-by: TBD diff --git a/src/hotspot/share/classfile/stackMapFrame.cpp b/src/hotspot/share/classfile/stackMapFrame.cpp --- a/src/hotspot/share/classfile/stackMapFrame.cpp +++ b/src/hotspot/share/classfile/stackMapFrame.cpp @@ -103,13 +103,16 @@ case T_ARRAY: { Symbol* sig = ss.as_symbol(CHECK_(VerificationType::bogus_type())); - // Create another symbol to save as signature stream unreferences - // this symbol. - Symbol* sig_copy = - verifier()->create_temporary_symbol(sig, 0, sig->utf8_length(), - CHECK_(VerificationType::bogus_type())); - assert(sig_copy == sig, "symbols don't match"); - return VerificationType::reference_type(sig_copy); + if (!sig->is_permanent()) { + // Create another symbol to save as signature stream unreferences + // this symbol. + Symbol *sig_copy = + verifier()->create_temporary_symbol(sig, 0, sig->utf8_length(), + CHECK_(VerificationType::bogus_type())); + assert(sig_copy == sig, "symbols don't match"); + sig = sig_copy; + } + return VerificationType::reference_type(sig); } case T_INT: return VerificationType::integer_type(); case T_BYTE: return VerificationType::byte_type(); diff --git a/src/hotspot/share/classfile/symbolTable.cpp b/src/hotspot/share/classfile/symbolTable.cpp --- a/src/hotspot/share/classfile/symbolTable.cpp +++ b/src/hotspot/share/classfile/symbolTable.cpp @@ -487,8 +487,8 @@ if (sym == NULL) { sym = SymbolTable::the_table()->do_add_if_needed(name, len, hash, false, CHECK_NULL); } - if (sym->refcount() != PERM_REFCOUNT) { - sym->increment_refcount(); + if (!sym->is_permanent()) { + sym->make_permanent(); log_trace_symboltable_helper(sym, "Asked for a permanent symbol, but got a regular one"); } return sym; diff --git a/src/hotspot/share/classfile/verifier.cpp b/src/hotspot/share/classfile/verifier.cpp --- a/src/hotspot/share/classfile/verifier.cpp +++ b/src/hotspot/share/classfile/verifier.cpp @@ -164,17 +164,15 @@ // If the class should be verified, first see if we can use the split // verifier. If not, or if verification fails and FailOverToOldVerifier // is set, then call the inference verifier. - Symbol* exception_name = NULL; const size_t message_buffer_len = klass->name()->utf8_length() + 1024; - char* message_buffer = NEW_RESOURCE_ARRAY(char, message_buffer_len); - char* exception_message = message_buffer; + char* message_buffer = NULL; + char* exception_message = NULL; - const char* klassName = klass->external_name(); bool can_failover = FailOverToOldVerifier && klass->major_version() < NOFAILOVER_MAJOR_VERSION; - log_info(class, init)("Start class verification for: %s", klassName); + log_info(class, init)("Start class verification for: %s", klass->external_name()); if (klass->major_version() >= STACKMAP_ATTRIBUTE_MAJOR_VERSION) { ClassVerifier split_verifier(klass, THREAD); split_verifier.verify_class(THREAD); @@ -182,8 +180,10 @@ if (can_failover && !HAS_PENDING_EXCEPTION && (exception_name == vmSymbols::java_lang_VerifyError() || exception_name == vmSymbols::java_lang_ClassFormatError())) { - log_info(verification)("Fail over class verification to old verifier for: %s", klassName); - log_info(class, init)("Fail over class verification to old verifier for: %s", klassName); + log_info(verification)("Fail over class verification to old verifier for: %s", klass->external_name()); + log_info(class, init)("Fail over class verification to old verifier for: %s", klass->external_name()); + message_buffer = NEW_RESOURCE_ARRAY(char, message_buffer_len); + exception_message = message_buffer; exception_name = inference_verify( klass, message_buffer, message_buffer_len, THREAD); } @@ -191,6 +191,8 @@ exception_message = split_verifier.exception_message(); } } else { + message_buffer = NEW_RESOURCE_ARRAY(char, message_buffer_len); + exception_message = message_buffer; exception_name = inference_verify( klass, message_buffer, message_buffer_len, THREAD); } @@ -198,20 +200,19 @@ LogTarget(Info, class, init) lt1; if (lt1.is_enabled()) { LogStream ls(lt1); - log_end_verification(&ls, klassName, exception_name, THREAD); + log_end_verification(&ls, klass->external_name(), exception_name, THREAD); } LogTarget(Info, verification) lt2; if (lt2.is_enabled()) { LogStream ls(lt2); - log_end_verification(&ls, klassName, exception_name, THREAD); + log_end_verification(&ls, klass->external_name(), exception_name, THREAD); } if (HAS_PENDING_EXCEPTION) { return false; // use the existing exception } else if (exception_name == NULL) { - return true; // verifcation succeeded + return true; // verification succeeded } else { // VerifyError or ClassFormatError to be created and thrown - ResourceMark rm(THREAD); Klass* kls = SystemDictionary::resolve_or_fail(exception_name, true, CHECK_false); if (log_is_enabled(Debug, class, resolve)) { @@ -228,7 +229,10 @@ } kls = kls->super(); } - message_buffer[message_buffer_len - 1] = '\0'; // just to be sure + if (message_buffer != NULL) { + message_buffer[message_buffer_len - 1] = '\0'; // just to be sure + } + assert(exception_message != NULL, ""); THROW_MSG_(exception_name, exception_message, false); } } @@ -569,17 +573,18 @@ ClassVerifier::ClassVerifier( InstanceKlass* klass, TRAPS) - : _thread(THREAD), _exception_type(NULL), _message(NULL), _klass(klass) { + : _thread(THREAD), _previous_symbol(NULL), _symbols(NULL), _exception_type(NULL), + _message(NULL), _klass(klass) { _this_type = VerificationType::reference_type(klass->name()); - // Create list to hold symbols in reference area. - _symbols = new GrowableArray(100, 0, NULL); } ClassVerifier::~ClassVerifier() { // Decrement the reference count for any symbols created. - for (int i = 0; i < _symbols->length(); i++) { - Symbol* s = _symbols->at(i); - s->decrement_refcount(); + if (_symbols != NULL) { + for (int i = 0; i < _symbols->length(); i++) { + Symbol* s = _symbols->at(i); + s->decrement_refcount(); + } } } @@ -3102,13 +3107,36 @@ // they can be reference counted. Symbol* ClassVerifier::create_temporary_symbol(const Symbol *s, int begin, int end, TRAPS) { - Symbol* sym = SymbolTable::new_symbol(s, begin, end, CHECK_NULL); - _symbols->push(sym); + const char* name = (const char*)s->base() + begin; + int length = end - begin; + + // Quick deduplication check + if (_previous_symbol != NULL && _previous_symbol->equals(name, length)) { + return _previous_symbol; + } + Symbol* sym = SymbolTable::new_symbol(name, length, CHECK_NULL); + if (!sym->is_permanent()) { + if (_symbols == NULL) { + _symbols = new GrowableArray(50, 0, NULL); + } + _symbols->push(sym); + } + _previous_symbol = sym; return sym; } -Symbol* ClassVerifier::create_temporary_symbol(const char *s, int length, TRAPS) { - Symbol* sym = SymbolTable::new_symbol(s, length, CHECK_NULL); - _symbols->push(sym); +Symbol* ClassVerifier::create_temporary_symbol(const char *name, int length, TRAPS) { + // Quick deduplication check + if (_previous_symbol != NULL && _previous_symbol->equals(name, length)) { + return _previous_symbol; + } + Symbol* sym = SymbolTable::new_symbol(name, length, CHECK_NULL); + if (!sym->is_permanent()) { + if (_symbols == NULL) { + _symbols = new GrowableArray(50, 0, NULL); + } + _symbols->push(sym); + } + _previous_symbol = sym; return sym; } diff --git a/src/hotspot/share/classfile/verifier.hpp b/src/hotspot/share/classfile/verifier.hpp --- a/src/hotspot/share/classfile/verifier.hpp +++ b/src/hotspot/share/classfile/verifier.hpp @@ -250,6 +250,8 @@ class ClassVerifier : public StackObj { private: Thread* _thread; + + Symbol* _previous_symbol; // cache of the previously looked up symbol GrowableArray* _symbols; // keep a list of symbols created Symbol* _exception_type; @@ -411,12 +413,18 @@ // created, we can't use a TempNewSymbol. Symbol* create_temporary_symbol(const Symbol* s, int begin, int end, TRAPS); Symbol* create_temporary_symbol(const char *s, int length, TRAPS); - Symbol* create_temporary_symbol(Symbol* s) { - // This version just updates the reference count and saves the symbol to be - // dereferenced later. - s->increment_refcount(); - _symbols->push(s); + if (s == _previous_symbol) { + return s; + } + if (!s->is_permanent()) { + s->increment_refcount(); + if (_symbols == NULL) { + _symbols = new GrowableArray(50, 0, NULL); + } + _symbols->push(s); + } + _previous_symbol = s; return s; } diff --git a/src/hotspot/share/oops/symbol.cpp b/src/hotspot/share/oops/symbol.cpp --- a/src/hotspot/share/oops/symbol.cpp +++ b/src/hotspot/share/oops/symbol.cpp @@ -264,6 +264,30 @@ } } +void Symbol::make_permanent() { + uint32_t found = _length_and_refcount; + while (true) { + uint32_t old_value = found; + int refc = extract_refcount(old_value); + int len = extract_length(old_value); + if (refc == PERM_REFCOUNT) { + return; // refcount is permanent, permanent is sticky + } else if (refc == 0) { +#ifdef ASSERT + print(); + fatal("refcount underflow"); +#endif + return; + } else { + found = Atomic::cmpxchg(pack_length_and_refcount(len, PERM_REFCOUNT), &_length_and_refcount, old_value); + if (found == old_value) { + return; // successfully updated. + } + // refcount changed, try again. + } + } +} + void Symbol::metaspace_pointers_do(MetaspaceClosure* it) { if (log_is_enabled(Trace, cds)) { LogStream trace_stream(Log(cds)::trace()); diff --git a/src/hotspot/share/oops/symbol.hpp b/src/hotspot/share/oops/symbol.hpp --- a/src/hotspot/share/oops/symbol.hpp +++ b/src/hotspot/share/oops/symbol.hpp @@ -170,6 +170,7 @@ bool is_permanent() { return (refcount() == PERM_REFCOUNT); } + void make_permanent(); // Function char_at() returns the Symbol's selected u1 byte as a char type. // diff --git a/src/hotspot/share/runtime/signature.cpp b/src/hotspot/share/runtime/signature.cpp --- a/src/hotspot/share/runtime/signature.cpp +++ b/src/hotspot/share/runtime/signature.cpp @@ -123,15 +123,6 @@ } -void SignatureIterator::dispatch_field() { - // no '(', just one (field) type - _index = 0; - _parameter_index = 0; - parse_type(); - check_signature_end(); -} - - void SignatureIterator::iterate_parameters() { // Parse parameters _index = 0; @@ -196,7 +187,6 @@ break; case done_parm: return; - break; default: tty->print_cr("*** parameter is " UINT64_FORMAT, fingerprint & parameter_feature_mask); tty->print_cr("*** fingerprint is " PTR64_FORMAT, saved_fingerprint); @@ -205,7 +195,6 @@ } fingerprint >>= parameter_feature_size; } - _parameter_index = 0; } @@ -239,10 +228,7 @@ break; case '[': { - int begin = ++_index; - while (sig->char_at(_index) == '[') { - _index++; - } + while (sig->char_at(++_index) == '[') ; if (sig->char_at(_index) == 'L') { while (sig->char_at(_index++) != ';') ; } else { @@ -281,16 +267,17 @@ // Implementation of SignatureStream SignatureStream::SignatureStream(Symbol* signature, bool is_method) : - _signature(signature), _at_return_type(false) { + _signature(signature), _at_return_type(false), _previous_name(NULL), _names(NULL) { _begin = _end = (is_method ? 1 : 0); // skip first '(' in method signatures - _names = new GrowableArray(10); next(); } SignatureStream::~SignatureStream() { // decrement refcount for names created during signature parsing - for (int i = 0; i < _names->length(); i++) { - _names->at(i)->decrement_refcount(); + if (_names != NULL) { + for (int i = 0; i < _names->length(); i++) { + _names->at(i)->decrement_refcount(); + } } } @@ -359,10 +346,35 @@ end--; } + const char* symbol_chars = (const char*)_signature->base() + begin; + int len = end - begin; + + // Quick check for common symbols in signatures + assert((vmSymbols::java_lang_String()->utf8_length() == 16 && vmSymbols::java_lang_Object()->utf8_length() == 16), "sanity"); + if (len == 16 && + strncmp(symbol_chars, "java/lang/", 10) == 0) { + if (strncmp("String", symbol_chars + 10, 6) == 0) { + return vmSymbols::java_lang_String(); + } else if (strncmp("Object", symbol_chars + 10, 6) == 0) { + return vmSymbols::java_lang_Object(); + } + } + + Symbol* name = _previous_name; + if (name != NULL && name->equals(symbol_chars, len)) { + return name; + } + // Save names for cleaning up reference count at the end of // SignatureStream scope. - Symbol* name = SymbolTable::new_symbol(_signature, begin, end, CHECK_NULL); - _names->push(name); // save new symbol for decrementing later + name = SymbolTable::new_symbol(symbol_chars, len, CHECK_NULL); + if (!name->is_permanent()) { + if (_names == NULL) { + _names = new GrowableArray(10); + } + _names->push(name); // save new symbol for decrementing later + } + _previous_name = name; return name; } @@ -481,8 +493,10 @@ if (c == ';') { return index + 1; } - if (invalid_name_char(c)) { - return -1; + switch (c) { + case '\0': case '.': case '[': + return -1; + default: ; // fall through } } // fall through @@ -490,12 +504,3 @@ } return -1; } - -bool SignatureVerifier::invalid_name_char(char c) { - switch (c) { - case '\0': case '.': case ';': case '[': - return true; - default: - return false; - } -} diff --git a/src/hotspot/share/runtime/signature.hpp b/src/hotspot/share/runtime/signature.hpp --- a/src/hotspot/share/runtime/signature.hpp +++ b/src/hotspot/share/runtime/signature.hpp @@ -90,7 +90,6 @@ SignatureIterator(Symbol* signature); // Iteration - void dispatch_field(); // dispatches once for field signatures void iterate_parameters(); // iterates over parameters only void iterate_parameters( uint64_t fingerprint ); void iterate_returntype(); // iterates over returntype only @@ -363,8 +362,8 @@ int _end; BasicType _type; bool _at_return_type; - GrowableArray* _names; // symbols created while parsing signature - + Symbol* _previous_name; // cache the previously looked up symbol to avoid lookups + GrowableArray* _names; // symbols created while parsing that need to be dereferenced public: bool at_return_type() const { return _at_return_type; } bool is_done() const; @@ -402,7 +401,7 @@ bool is_array() const; // True if this argument is an array BasicType type() const { return _type; } Symbol* as_symbol(TRAPS); - enum FailureMode { ReturnNull, CNFException, NCDFError }; + enum FailureMode { ReturnNull, NCDFError }; Klass* as_klass(Handle class_loader, Handle protection_domain, FailureMode failure_mode, TRAPS); oop as_java_mirror(Handle class_loader, Handle protection_domain, FailureMode failure_mode, TRAPS); const u1* raw_bytes() { return _signature->bytes() + _begin; } @@ -423,9 +422,7 @@ static bool is_valid_method_signature(Symbol* sig); static bool is_valid_type_signature(Symbol* sig); private: - static ssize_t is_valid_type(const char*, ssize_t); - static bool invalid_name_char(char); }; #endif // SHARE_RUNTIME_SIGNATURE_HPP