8239235: Examine SignatureStream performance after consolidation
Reviewed-by: lfoltan, coleenp
diff --git a/src/hotspot/share/classfile/stackMapFrame.cpp b/src/hotspot/share/classfile/stackMapFrame.cpp
index c102e32..3edc586 100644
--- a/src/hotspot/share/classfile/stackMapFrame.cpp
+++ b/src/hotspot/share/classfile/stackMapFrame.cpp
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2003, 2019, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2003, 2020, 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
@@ -106,7 +106,7 @@
// Create another symbol to save as signature stream unreferences
// this symbol.
Symbol *sig_copy =
- verifier()->create_temporary_symbol(sig, 0, sig->utf8_length());
+ verifier()->create_temporary_symbol(sig);
assert(sig_copy == sig, "symbols don't match");
sig = sig_copy;
}
diff --git a/src/hotspot/share/classfile/symbolTable.cpp b/src/hotspot/share/classfile/symbolTable.cpp
index 8db26e0..17dfbc9 100644
--- a/src/hotspot/share/classfile/symbolTable.cpp
+++ b/src/hotspot/share/classfile/symbolTable.cpp
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 1997, 2019, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1997, 2020, 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
@@ -56,7 +56,7 @@
inline bool symbol_equals_compact_hashtable_entry(Symbol* value, const char* key, int len) {
if (value->equals(key, len)) {
- assert(value->refcount() == PERM_REFCOUNT, "must be shared");
+ assert(value->is_permanent(), "must be shared");
return true;
} else {
return false;
@@ -139,10 +139,10 @@
static void free_node(void* memory, Value const& value) {
// We get here because #1 some threads lost a race to insert a newly created Symbol
// or #2 we're cleaning up unused symbol.
- // If #1, then the symbol can be either permanent (refcount==PERM_REFCOUNT),
+ // If #1, then the symbol can be either permanent,
// or regular newly created one (refcount==1)
// If #2, then the symbol is dead (refcount==0)
- assert((value->refcount() == PERM_REFCOUNT) || (value->refcount() == 1) || (value->refcount() == 0),
+ assert(value->is_permanent() || (value->refcount() == 1) || (value->refcount() == 0),
"refcount %d", value->refcount());
if (value->refcount() == 1) {
value->decrement_refcount();
@@ -176,7 +176,7 @@
}
void SymbolTable::delete_symbol(Symbol* sym) {
- if (sym->refcount() == PERM_REFCOUNT) {
+ if (sym->is_permanent()) {
MutexLocker ml(SymbolArena_lock, Mutex::_no_safepoint_check_flag); // Protect arena
// Deleting permanent symbol should not occur very often (insert race condition),
// so log it.
diff --git a/src/hotspot/share/classfile/verifier.cpp b/src/hotspot/share/classfile/verifier.cpp
index d1de0bc..e409ebf 100644
--- a/src/hotspot/share/classfile/verifier.cpp
+++ b/src/hotspot/share/classfile/verifier.cpp
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 1998, 2019, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1998, 2020, 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
@@ -3153,13 +3153,6 @@
// The verifier creates symbols which are substrings of Symbols.
// These are stored in the verifier until the end of verification so that
// they can be reference counted.
-Symbol* ClassVerifier::create_temporary_symbol(const Symbol *s, int begin,
- int end) {
- const char* name = (const char*)s->base() + begin;
- int length = end - begin;
- return create_temporary_symbol(name, length);
-}
-
Symbol* ClassVerifier::create_temporary_symbol(const char *name, int length) {
// Quick deduplication check
if (_previous_symbol != NULL && _previous_symbol->equals(name, length)) {
diff --git a/src/hotspot/share/classfile/verifier.hpp b/src/hotspot/share/classfile/verifier.hpp
index f6db892..e629b4f 100644
--- a/src/hotspot/share/classfile/verifier.hpp
+++ b/src/hotspot/share/classfile/verifier.hpp
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 1998, 2019, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1998, 2020, 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
@@ -456,7 +456,6 @@
// their reference counts need to be decremented when the verifier object
// goes out of scope. Since these symbols escape the scope in which they're
// created, we can't use a TempNewSymbol.
- Symbol* create_temporary_symbol(const Symbol* s, int begin, int end);
Symbol* create_temporary_symbol(const char *s, int length);
Symbol* create_temporary_symbol(Symbol* s) {
if (s == _previous_symbol) {
diff --git a/src/hotspot/share/oops/symbol.cpp b/src/hotspot/share/oops/symbol.cpp
index b701be5..356d9bd 100644
--- a/src/hotspot/share/oops/symbol.cpp
+++ b/src/hotspot/share/oops/symbol.cpp
@@ -78,34 +78,6 @@
_hash_and_refcount = pack_hash_and_refcount(extract_hash(_hash_and_refcount), PERM_REFCOUNT);
}
-
-// ------------------------------------------------------------------
-// Symbol::contains_byte_at
-//
-// Tests if the symbol contains the given byte at the given position.
-bool Symbol::contains_byte_at(int position, char code_byte) const {
- if (position < 0) return false; // can happen with ends_with
- if (position >= utf8_length()) return false;
- return code_byte == char_at(position);
-}
-
-// ------------------------------------------------------------------
-// Symbol::contains_utf8_at
-//
-// Tests if the symbol contains the given utf8 substring
-// at the given byte position.
-bool Symbol::contains_utf8_at(int position, const char* substring, int len) const {
- assert(len >= 0 && substring != NULL, "substring must be valid");
- if (len <= 1)
- return len == 0 || contains_byte_at(position, substring[0]);
- if (position < 0) return false; // can happen with ends_with
- if (position + len > utf8_length()) return false;
- if (memcmp((char*)base() + position, substring, len) == 0)
- return true;
- else
- return false;
-}
-
// ------------------------------------------------------------------
// Symbol::index_of
//
diff --git a/src/hotspot/share/oops/symbol.hpp b/src/hotspot/share/oops/symbol.hpp
index f760bf3..789214f 100644
--- a/src/hotspot/share/oops/symbol.hpp
+++ b/src/hotspot/share/oops/symbol.hpp
@@ -97,7 +97,7 @@
// Set _refcount to PERM_REFCOUNT to prevent the Symbol from being freed.
#ifndef PERM_REFCOUNT
-#define PERM_REFCOUNT ((1 << 16) - 1)
+#define PERM_REFCOUNT 0xffff
#endif
class Symbol : public MetaspaceObj {
@@ -113,7 +113,7 @@
u1 _body[2];
enum {
- max_symbol_length = (1 << 16) -1
+ max_symbol_length = 0xffff
};
static int byte_size(int length) {
@@ -165,7 +165,7 @@
bool try_increment_refcount();
void increment_refcount();
void decrement_refcount();
- bool is_permanent() {
+ bool is_permanent() const {
return (refcount() == PERM_REFCOUNT);
}
void set_permanent();
@@ -210,12 +210,24 @@
return ends_with(suffix, (int) strlen(suffix));
}
bool ends_with(int suffix_char) const {
- return contains_byte_at(utf8_length()-1, suffix_char);
+ return contains_byte_at(utf8_length() - 1, suffix_char);
}
+
// Tests if the symbol contains the given utf8 substring
- // or byte at the given byte position.
- bool contains_utf8_at(int position, const char* substring, int len) const;
- bool contains_byte_at(int position, char code_byte) const;
+ // at the given byte position.
+ bool contains_utf8_at(int position, const char* substring, int len) const {
+ assert(len >= 0 && substring != NULL, "substring must be valid");
+ if (position < 0) return false; // can happen with ends_with
+ if (position + len > utf8_length()) return false;
+ return (memcmp((char*)base() + position, substring, len) == 0);
+ }
+
+ // Tests if the symbol contains the given byte at the given position.
+ bool contains_byte_at(int position, char code_byte) const {
+ if (position < 0) return false; // can happen with ends_with
+ if (position >= utf8_length()) return false;
+ return code_byte == char_at(position);
+ }
// Tests if the symbol starts with the given prefix.
int index_of_at(int i, const char* str, int len) const;
diff --git a/src/hotspot/share/runtime/signature.cpp b/src/hotspot/share/runtime/signature.cpp
index c23cdfd..fb3beda 100644
--- a/src/hotspot/share/runtime/signature.cpp
+++ b/src/hotspot/share/runtime/signature.cpp
@@ -172,14 +172,14 @@
// Implementation of SignatureStream
-static inline int decode_signature_char(int ch) {
+static inline BasicType decode_signature_char(int ch) {
switch (ch) {
#define EACH_SIG(ch, bt, ignore) \
case ch: return bt;
SIGNATURE_TYPES_DO(EACH_SIG, ignore)
#undef EACH_SIG
}
- return 0;
+ return (BasicType)0;
}
SignatureStream::SignatureStream(const Symbol* signature,
@@ -188,28 +188,29 @@
"method signature required");
_signature = signature;
_limit = signature->utf8_length();
- int oz = (is_method ? 1 : 0);
+ int oz = (is_method ? _s_method : _s_field);
_state = oz;
- assert(_state == (int)(is_method ? _s_method : _s_field), "signature state incorrectly set");
_begin = _end = oz; // skip first '(' in method signatures
_array_prefix = 0; // just for definiteness
- _previous_name = NULL;
+
+ // assigning java/lang/Object to _previous_name means we can
+ // avoid a number of NULL checks in the parser
+ _previous_name = vmSymbols::java_lang_Object();
_names = NULL;
next();
}
SignatureStream::~SignatureStream() {
// decrement refcount for names created during signature parsing
+ _previous_name->decrement_refcount();
if (_names != NULL) {
for (int i = 0; i < _names->length(); i++) {
_names->at(i)->decrement_refcount();
}
- } else if (_previous_name != NULL && !_previous_name->is_permanent()) {
- _previous_name->decrement_refcount();
}
}
-inline int SignatureStream::scan_non_primitive(BasicType type) {
+inline int SignatureStream::scan_type(BasicType type) {
const u1* base = _signature->bytes();
int end = _end;
int limit = _limit;
@@ -217,22 +218,24 @@
switch (type) {
case T_OBJECT:
tem = (const u1*) memchr(&base[end], JVM_SIGNATURE_ENDCLASS, limit - end);
- end = (tem == NULL ? limit : tem+1 - base);
- break;
+ return (tem == NULL ? limit : tem + 1 - base);
case T_ARRAY:
while ((end < limit) && ((char)base[end] == JVM_SIGNATURE_ARRAY)) { end++; }
_array_prefix = end - _end; // number of '[' chars just skipped
- if (Signature::has_envelope(base[end++])) {
- tem = (const u1*) memchr(&base[end], JVM_SIGNATURE_ENDCLASS, limit - end);
- end = (tem == NULL ? limit : tem+1 - base);
- break;
+ if (Signature::has_envelope(base[end])) {
+ tem = (const u1 *) memchr(&base[end], JVM_SIGNATURE_ENDCLASS, limit - end);
+ return (tem == NULL ? limit : tem + 1 - base);
}
- break;
+ // Skipping over a single character for a primitive type.
+ assert(is_java_primitive(decode_signature_char(base[end])), "only primitives expected");
+ return end + 1;
- default : ShouldNotReachHere();
+ default:
+ // Skipping over a single character for a primitive type (or void).
+ assert(!is_reference_type(type), "only primitives or void expected");
+ return end + 1;
}
- return end;
}
void SignatureStream::next() {
@@ -241,50 +244,33 @@
if (_end >= len) { set_done(); return; }
_begin = _end;
int ch = sig->char_at(_begin);
- int btcode = decode_signature_char(ch);
- if (btcode == 0) {
- guarantee(ch == JVM_SIGNATURE_ENDFUNC, "bad signature char %c/%d", ch, ch);
+ if (ch == JVM_SIGNATURE_ENDFUNC) {
assert(_state == _s_method, "must be in method");
_state = _s_method_return;
_begin = ++_end;
if (_end >= len) { set_done(); return; }
ch = sig->char_at(_begin);
- btcode = decode_signature_char(ch);
}
- BasicType bt = (BasicType) btcode;
+ BasicType bt = decode_signature_char(ch);
assert(ch == type2char(bt), "bad signature char %c/%d", ch, ch);
_type = bt;
- if (!is_reference_type(bt)) {
- // Skip over a single character for a primitive type (or void).
- _end++;
- return;
- }
- _end = scan_non_primitive(bt);
+ _end = scan_type(bt);
}
-int SignatureStream::skip_array_prefix(int max_skip_length) {
- if (_type != T_ARRAY) {
- return 0;
- }
- if (_array_prefix > max_skip_length) {
- // strip some but not all levels of T_ARRAY
- _array_prefix -= max_skip_length;
- _begin += max_skip_length;
- return max_skip_length;
- }
+int SignatureStream::skip_whole_array_prefix() {
+ assert(_type == T_ARRAY, "must be");
+
// we are stripping all levels of T_ARRAY,
// so we must decode the next character
int whole_array_prefix = _array_prefix;
int new_begin = _begin + whole_array_prefix;
_begin = new_begin;
int ch = _signature->char_at(new_begin);
- int btcode = decode_signature_char(ch);
- BasicType bt = (BasicType) btcode;
+ BasicType bt = decode_signature_char(ch);
assert(ch == type2char(bt), "bad signature char %c/%d", ch, ch);
_type = bt;
assert(bt != T_VOID && bt != T_ARRAY, "bad signature type");
- // Don't bother to call scan_non_primitive, since it won't
- // change the value of _end.
+ // Don't bother to re-scan, since it won't change the value of _end.
return whole_array_prefix;
}
@@ -317,9 +303,9 @@
}
BasicType Signature::basic_type(int ch) {
- int btcode = decode_signature_char(ch);
+ BasicType btcode = decode_signature_char(ch);
if (btcode == 0) return T_ILLEGAL;
- return (BasicType) btcode;
+ return btcode;
}
static const int jl_len = 10, object_len = 6, jl_object_len = jl_len + object_len;
@@ -365,7 +351,7 @@
}
Symbol* name = _previous_name;
- if (name != NULL && name->equals(symbol_chars, len)) {
+ if (name->equals(symbol_chars, len)) {
return name;
}
@@ -375,18 +361,11 @@
// Only allocate the GrowableArray for the _names buffer if more than
// one name is being processed in the signature.
- if (_previous_name != NULL &&
- !_previous_name->is_permanent() &&
- !name->is_permanent() &&
- _names == NULL) {
- _names = new GrowableArray<Symbol*>(10);
- _names->push(_previous_name);
- }
- if (!name->is_permanent() && _previous_name != NULL) {
+ if (!_previous_name->is_permanent()) {
if (_names == NULL) {
_names = new GrowableArray<Symbol*>(10);
}
- _names->push(name); // save new symbol for decrementing later
+ _names->push(_previous_name);
}
_previous_name = name;
return name;
diff --git a/src/hotspot/share/runtime/signature.hpp b/src/hotspot/share/runtime/signature.hpp
index 4252202..59178ca 100644
--- a/src/hotspot/share/runtime/signature.hpp
+++ b/src/hotspot/share/runtime/signature.hpp
@@ -232,7 +232,6 @@
// bitfields from the fingerprint. Otherwise, the
// symbol is parsed.
template<typename T> inline void do_parameters_on(T* callback); // iterates over parameters only
- void skip_parameters(); // skips over parameters to find return type
BasicType return_type(); // computes the value on the fly if necessary
static bool fp_is_static(fingerprint_t fingerprint) {
@@ -478,8 +477,6 @@
Symbol* _previous_name; // cache the previously looked up symbol to avoid lookups
GrowableArray<Symbol*>* _names; // symbols created while parsing that need to be dereferenced
- inline int scan_non_primitive(BasicType type);
-
Symbol* find_symbol();
enum { _s_field = 0, _s_method = 1, _s_method_return = 3 };
@@ -487,9 +484,9 @@
_state |= -2; // preserve s_method bit
assert(is_done(), "Unable to set state to done");
}
+ int scan_type(BasicType bt);
public:
- bool is_method_signature() const { return (_state & (int)_s_method) != 0; }
bool at_return_type() const { return _state == (int)_s_method_return; }
bool is_done() const { return _state < 0; }
void next();
@@ -504,8 +501,6 @@
const u1* raw_bytes() const { return _signature->bytes() + _begin; }
int raw_length() const { return _end - _begin; }
- int raw_begin() const { return _begin; }
- int raw_end() const { return _end; }
int raw_symbol_begin() const { return _begin + (has_envelope() ? 1 : 0); }
int raw_symbol_end() const { return _end - (has_envelope() ? 1 : 0); }
char raw_char_at(int i) const {
@@ -541,7 +536,27 @@
// (The argument is clipped to array_prefix_length(),
// and if it ends up as zero this call is a nop.
// The default is value skips all brackets '['.)
- int skip_array_prefix(int prefix_length = 9999);
+ private:
+ int skip_whole_array_prefix();
+ public:
+ int skip_array_prefix(int max_skip_length) {
+ if (_type != T_ARRAY) {
+ return 0;
+ }
+ if (_array_prefix > max_skip_length) {
+ // strip some but not all levels of T_ARRAY
+ _array_prefix -= max_skip_length;
+ _begin += max_skip_length;
+ return max_skip_length;
+ }
+ return skip_whole_array_prefix();
+ }
+ int skip_array_prefix() {
+ if (_type != T_ARRAY) {
+ return 0;
+ }
+ return skip_whole_array_prefix();
+ }
// free-standing lookups (bring your own CL/PD pair)
enum FailureMode { ReturnNull, NCDFError, CachedOrNull };
diff --git a/test/hotspot/gtest/runtime/test_signatureStream.cpp b/test/hotspot/gtest/runtime/test_signatureStream.cpp
new file mode 100644
index 0000000..3a5fbbd
--- /dev/null
+++ b/test/hotspot/gtest/runtime/test_signatureStream.cpp
@@ -0,0 +1,88 @@
+/*
+ * Copyright (c) 2020, 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
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+#include "precompiled.hpp"
+#include "runtime/interfaceSupport.inline.hpp"
+#include "classfile/symbolTable.hpp"
+#include "runtime/signature.hpp"
+#include "threadHelper.inline.hpp"
+#include "unittest.hpp"
+
+TEST_VM(SignatureStream, check_refcount) {
+
+ JavaThread* THREAD = JavaThread::current();
+ // the thread should be in vm to use locks
+ ThreadInVMfromNative ThreadInVMfromNative(THREAD);
+ // SignatureStream::as_symbol will allocate on the resource area
+ ResourceMark rm(THREAD);
+
+ Symbol* foo = SymbolTable::new_symbol("Foo");
+ int r1 = foo->refcount();
+
+ {
+ // Trivial test: non-method signature of a non-permanent symbol
+ Symbol* methodSig = SymbolTable::new_symbol("LFoo;");
+ SignatureStream ss(methodSig, false);
+ Symbol* sym = ss.as_symbol();
+ ASSERT_EQ(sym, foo) << "found symbol should be Foo: " << sym->as_C_string();
+ // This should mean the SS looks up and increments refcount to Foo
+ ASSERT_EQ(foo->refcount(), r1 + 1) << "refcount should be incremented";
+
+ ASSERT_TRUE(!ss.is_done())
+ << "stream parsing should not be marked as done until"
+ <<" ss.next() called after the last symbol";
+
+ ss.next();
+ ASSERT_TRUE(ss.is_done()) << "stream parsing should be marked as done";
+ }
+
+ ASSERT_EQ(foo->refcount(), r1) << "refcount should have decremented";
+
+ {
+ // Ensure refcount is properly decremented when first symbol is non-permanent and second isn't
+
+ Symbol* integer = SymbolTable::new_symbol("java/lang/Integer");
+ ASSERT_TRUE(integer->is_permanent()) << "java/lang/Integer must be permanent";
+
+ Symbol* methodSig = SymbolTable::new_symbol("(LFoo;)Ljava/lang/Integer;");
+ SignatureStream ss(methodSig);
+ Symbol* sym = ss.as_symbol();
+ ASSERT_EQ(sym, foo) << "found symbol should be Foo: " << sym->as_C_string();
+ // This should mean the SS looks up and increments refcount to Foo
+ ASSERT_EQ(foo->refcount(), r1 + 1) << "refcount should be incremented";
+
+ ss.next();
+ sym = ss.as_symbol();
+ ASSERT_EQ(sym, integer) << "found symbol should be java/lang/Integer";
+
+ ASSERT_TRUE(!ss.is_done())
+ << "stream parsing should not be marked as done until"
+ <<" ss.next() called after the last symbol";
+
+ ss.next();
+ ASSERT_TRUE(ss.is_done()) << "stream parsing should be marked as done";
+ }
+
+ ASSERT_EQ(foo->refcount(), r1) << "refcount should have decremented";
+
+}
\ No newline at end of file
