8220366: Optimize Symbol handling in ClassVerifier and SignatureStream

Reviewed-by: hseigel, coleenp, lfoltan

src/hotspot/share/classfile/stackMapFrame.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2003, 2016, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2003, 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
@@ -103,13 +103,16 @@ VerificationType StackMapFrame::set_locals_from_arg(
     case T_ARRAY:
     {
       Symbol* sig = ss.as_symbol(CHECK_(VerificationType::bogus_type()));
+      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");
-      return VerificationType::reference_type(sig_copy);
+        sig = sig_copy;
+      }
+      return VerificationType::reference_type(sig);
     }
     case T_INT:     return VerificationType::integer_type();
     case T_BYTE:    return VerificationType::byte_type();

src/hotspot/share/classfile/symbolTable.cpp

@@ -487,8 +487,8 @@ Symbol* SymbolTable::new_permanent_symbol(const char* name, TRAPS) {
   if (sym == NULL) {
     sym = SymbolTable::the_table()->do_add_if_needed(name, len, hash, false, CHECK_NULL);
   }
-  if (sym->refcount() != PERM_REFCOUNT) {
+  if (!sym->is_permanent()) {
-    sym->increment_refcount();
+    sym->make_permanent();
     log_trace_symboltable_helper(sym, "Asked for a permanent symbol, but got a regular one");
   }
   return sym;

src/hotspot/share/classfile/verifier.cpp

@@ -164,17 +164,15 @@ bool Verifier::verify(InstanceKlass* klass, bool should_verify_class, TRAPS) {
   // 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* message_buffer = NULL;
-  char* exception_message = message_buffer;
+  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 @@ bool Verifier::verify(InstanceKlass* klass, bool should_verify_class, TRAPS) {
     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(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", klassName);
+      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 @@ bool Verifier::verify(InstanceKlass* klass, bool should_verify_class, TRAPS) {
       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 @@ bool Verifier::verify(InstanceKlass* klass, bool should_verify_class, TRAPS) {
   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 @@ bool Verifier::verify(InstanceKlass* klass, bool should_verify_class, TRAPS) {
       }
       kls = kls->super();
     }
+    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,19 +573,20 @@ void ErrorContext::stackmap_details(outputStream* ss, const Method* method) cons
 
 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<Symbol*>(100, 0, NULL);
 }
 
 ClassVerifier::~ClassVerifier() {
   // Decrement the reference count for any symbols created.
+  if (_symbols != NULL) {
     for (int i = 0; i < _symbols->length(); i++) {
       Symbol* s = _symbols->at(i);
       s->decrement_refcount();
     }
   }
+}
 
 VerificationType ClassVerifier::object_type() const {
   return VerificationType::reference_type(vmSymbols::java_lang_Object());
@@ -3092,13 +3097,23 @@ void ClassVerifier::verify_return_value(
 // 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);
+  const char* name = (const char*)s->base() + begin;
-  _symbols->push(sym);
+  int length = end - begin;
-  return sym;
+  return create_temporary_symbol(name, length, CHECK_NULL);
 }
 
-Symbol* ClassVerifier::create_temporary_symbol(const char *s, int length, TRAPS) {
+Symbol* ClassVerifier::create_temporary_symbol(const char *name, int length, TRAPS) {
-  Symbol* sym = SymbolTable::new_symbol(s, length, CHECK_NULL);
+  // 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<Symbol*>(50, 0, NULL);
+    }
     _symbols->push(sym);
+  }
+  _previous_symbol = sym;
   return sym;
 }

src/hotspot/share/classfile/verifier.hpp

@@ -250,6 +250,8 @@ class ErrorContext {
 class ClassVerifier : public StackObj {
  private:
   Thread* _thread;
+
+  Symbol* _previous_symbol;          // cache of the previously looked up symbol
   GrowableArray<Symbol*>* _symbols;  // keep a list of symbols created
 
   Symbol* _exception_type;
@@ -411,12 +413,18 @@ class ClassVerifier : public StackObj {
   // 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
+    if (s == _previous_symbol) {
-    // dereferenced later.
+      return s;
+    }
+    if (!s->is_permanent()) {
       s->increment_refcount();
+      if (_symbols == NULL) {
+        _symbols = new GrowableArray<Symbol*>(50, 0, NULL);
+      }
       _symbols->push(s);
+    }
+    _previous_symbol = s;
     return s;
   }
 

src/hotspot/share/oops/symbol.cpp

@@ -264,6 +264,30 @@ void Symbol::decrement_refcount() {
   }
 }
 
+void Symbol::make_permanent() {
+  uint32_t found = _length_and_refcount;
+  while (true) {
+    uint32_t old_value = found;
+    int refc = extract_refcount(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 {
+      int len = extract_length(old_value);
+      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());

src/hotspot/share/oops/symbol.hpp

@@ -170,6 +170,7 @@ class Symbol : public MetaspaceObj {
   bool is_permanent() {
     return (refcount() == PERM_REFCOUNT);
   }
+  void make_permanent();
 
   // Function char_at() returns the Symbol's selected u1 byte as a char type.
   //

src/hotspot/share/runtime/signature.cpp

@@ -123,15 +123,6 @@ void SignatureIterator::check_signature_end() {
 }
 
 
-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 @@ void SignatureIterator::iterate_parameters( uint64_t fingerprint ) {
         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 @@ void SignatureIterator::iterate_parameters( uint64_t fingerprint ) {
     }
     fingerprint >>= parameter_feature_size;
   }
-  _parameter_index = 0;
 }
 
 
@@ -239,10 +228,7 @@ void SignatureIterator::iterate_returntype() {
         break;
       case '[':
         {
-          int begin = ++_index;
+          while (sig->char_at(++_index) == '[') ;
-          while (sig->char_at(_index) == '[') {
-            _index++;
-          }
           if (sig->char_at(_index) == 'L') {
             while (sig->char_at(_index++) != ';') ;
           } else {
@@ -281,18 +267,19 @@ void SignatureIterator::iterate() {
 
 // 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<Symbol*>(10);
   next();
 }
 
 SignatureStream::~SignatureStream() {
   // decrement refcount for names created during signature parsing
+  if (_names != NULL) {
     for (int i = 0; i < _names->length(); i++) {
       _names->at(i)->decrement_refcount();
     }
   }
+}
 
 bool SignatureStream::is_done() const {
   return _end > _signature->utf8_length();
@@ -359,10 +346,35 @@ Symbol* SignatureStream::as_symbol(TRAPS) {
     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);
+  name = SymbolTable::new_symbol(symbol_chars, len, CHECK_NULL);
+  if (!name->is_permanent()) {
+    if (_names == NULL) {
+      _names = new GrowableArray<Symbol*>(10);
+    }
     _names->push(name);  // save new symbol for decrementing later
+  }
+  _previous_name = name;
   return name;
 }
 
@@ -467,11 +479,12 @@ ssize_t SignatureVerifier::is_valid_type(const char* type, ssize_t limit) {
     case 'L':
       for (index = index + 1; index < limit; ++index) {
         char c = type[index];
-        if (c == ';') {
+        switch (c) {
+          case ';':
             return index + 1;
-        }
+          case '\0': case '.': case '[':
-        if (invalid_name_char(c)) {
             return -1;
+          default: ; // fall through
         }
       }
       // fall through
@@ -479,13 +492,4 @@ ssize_t SignatureVerifier::is_valid_type(const char* type, ssize_t limit) {
   }
   return -1;
 }
-
-bool SignatureVerifier::invalid_name_char(char c) {
-  switch (c) {
-    case '\0': case '.': case ';': case '[':
-      return true;
-    default:
-      return false;
-  }
-}
 #endif // ASSERT

src/hotspot/share/runtime/signature.hpp

@@ -90,7 +90,6 @@ class SignatureIterator: public ResourceObj {
   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 @@ class SignatureStream : public StackObj {
   int          _end;
   BasicType    _type;
   bool         _at_return_type;
-  GrowableArray<Symbol*>* _names;  // symbols created while parsing signature
+  Symbol*      _previous_name;     // cache the previously looked up symbol to avoid lookups
-
+  GrowableArray<Symbol*>* _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 @@ class SignatureStream : public StackObj {
   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; }
@@ -421,9 +420,7 @@ class SignatureVerifier : public StackObj {
     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
 #endif // SHARE_RUNTIME_SIGNATURE_HPP