--- old/src/hotspot/share/classfile/javaClasses.cpp 2017-11-30 14:07:07.372262772 +0100 +++ new/src/hotspot/share/classfile/javaClasses.cpp 2017-11-30 14:07:07.016262784 +0100 @@ -619,12 +619,12 @@ bool java_lang_String::equals(oop java_string, jchar* chars, int len) { assert(java_string->klass() == SystemDictionary::String_klass(), "must be java_string"); - typeArrayOop value = java_lang_String::value(java_string); - int length = java_lang_String::length(java_string); + typeArrayOop value = java_lang_String::value_no_keepalive(java_string); + int length = java_lang_String::length(java_string); if (length != len) { return false; } - bool is_latin1 = java_lang_String::is_latin1(java_string); + bool is_latin1 = java_lang_String::is_latin1(java_string); if (!is_latin1) { for (int i = 0; i < len; i++) { if (value->char_at(i) != chars[i]) { @@ -646,12 +646,12 @@ "must be java String"); assert(str2->klass() == SystemDictionary::String_klass(), "must be java String"); - typeArrayOop value1 = java_lang_String::value(str1); - int length1 = java_lang_String::length(str1); - bool is_latin1 = java_lang_String::is_latin1(str1); - typeArrayOop value2 = java_lang_String::value(str2); - int length2 = java_lang_String::length(str2); - bool is_latin2 = java_lang_String::is_latin1(str2); + typeArrayOop value1 = java_lang_String::value_no_keepalive(str1); + int length1 = java_lang_String::length(value1); + bool is_latin1 = java_lang_String::is_latin1(str1); + typeArrayOop value2 = java_lang_String::value_no_keepalive(str2); + int length2 = java_lang_String::length(value2); + bool is_latin2 = java_lang_String::is_latin1(str2); if ((length1 != length2) || (is_latin1 != is_latin2)) { // Strings of different size or with different @@ -659,7 +659,7 @@ return false; } int blength1 = value1->length(); - for (int i = 0; i < value1->length(); i++) { + for (int i = 0; i < blength1; i++) { if (value1->byte_at(i) != value2->byte_at(i)) { return false; } @@ -669,7 +669,7 @@ void java_lang_String::print(oop java_string, outputStream* st) { assert(java_string->klass() == SystemDictionary::String_klass(), "must be java_string"); - typeArrayOop value = java_lang_String::value(java_string); + typeArrayOop value = java_lang_String::value_no_keepalive(java_string); if (value == NULL) { // This can happen if, e.g., printing a String --- old/src/hotspot/share/classfile/javaClasses.hpp 2017-11-30 14:07:08.520262732 +0100 +++ new/src/hotspot/share/classfile/javaClasses.hpp 2017-11-30 14:07:08.168262744 +0100 @@ -102,6 +102,7 @@ // Accessors static inline typeArrayOop value(oop java_string); + static inline typeArrayOop value_no_keepalive(oop java_string); static inline unsigned int hash(oop java_string); static inline bool is_latin1(oop java_string); static inline int length(oop java_string); --- old/src/hotspot/share/classfile/javaClasses.inline.hpp 2017-11-30 14:07:09.648262692 +0100 +++ new/src/hotspot/share/classfile/javaClasses.inline.hpp 2017-11-30 14:07:09.288262705 +0100 @@ -26,6 +26,7 @@ #define SHARE_VM_CLASSFILE_JAVACLASSES_INLINE_HPP #include "classfile/javaClasses.hpp" +#include "oops/access.inline.hpp" #include "oops/oop.inline.hpp" #include "oops/oopsHierarchy.hpp" @@ -53,6 +54,11 @@ assert(is_instance(java_string), "must be java_string"); return (typeArrayOop) java_string->obj_field(value_offset); } +typeArrayOop java_lang_String::value_no_keepalive(oop java_string) { + assert(initialized && (value_offset > 0), "Must be initialized"); + assert(is_instance(java_string), "must be java_string"); + return (typeArrayOop) java_string->obj_field_special(value_offset); +} unsigned int java_lang_String::hash(oop java_string) { assert(initialized && (hash_offset > 0), "Must be initialized"); assert(is_instance(java_string), "must be java_string"); @@ -68,11 +74,11 @@ int java_lang_String::length(oop java_string) { assert(initialized, "Must be initialized"); assert(is_instance(java_string), "must be java_string"); - typeArrayOop value_array = ((typeArrayOop)java_string->obj_field(value_offset)); - if (value_array == NULL) { + typeArrayOop value = java_lang_String::value_no_keepalive(java_string); + if (value == NULL) { return 0; } - int arr_length = value_array->length(); + int arr_length = value->length(); if (!is_latin1(java_string)) { assert((arr_length & 1) == 0, "should be even for UTF16 string"); arr_length >>= 1; // convert number of bytes to number of elements --- old/src/hotspot/share/classfile/stringTable.cpp 2017-11-30 14:07:10.776262653 +0100 +++ new/src/hotspot/share/classfile/stringTable.cpp 2017-11-30 14:07:10.420262666 +0100 @@ -35,6 +35,7 @@ #include "memory/filemap.hpp" #include "memory/metaspaceShared.hpp" #include "memory/resourceArea.hpp" +#include "oops/access.inline.hpp" #include "oops/oop.inline.hpp" #include "runtime/atomic.hpp" #include "runtime/mutexLocker.hpp" @@ -43,7 +44,6 @@ #include "utilities/macros.hpp" #if INCLUDE_ALL_GCS #include "gc/g1/g1CollectedHeap.hpp" -#include "gc/g1/g1SATBCardTableModRefBS.hpp" #include "gc/g1/g1StringDedup.hpp" #endif @@ -124,6 +124,22 @@ } } +oop StringTable::string_object(HashtableEntry* entry) { + return RootAccess::oop_load(entry->literal_addr()); +} + +oop StringTable::string_object_no_keepalive(HashtableEntry* entry) { + // The AS_NO_KEEPALIVE peeks at the oop without keeping it alive. + // This is *very dangerous* in general but is okay in this specific + // case. The subsequent oop_load keeps the oop alive if it it matched + // the jchar* string. + return RootAccess::oop_load(entry->literal_addr()); +} + +void StringTable::set_string_object(HashtableEntry* entry, oop string) { + RootAccess::oop_store(entry->literal_addr(), string); +} + oop StringTable::lookup_shared(jchar* name, int len, unsigned int hash) { assert(hash == java_lang_String::hash_code(name, len), "hash must be computed using java_lang_String::hash_code"); @@ -131,13 +147,16 @@ } oop StringTable::lookup_in_main_table(int index, jchar* name, - int len, unsigned int hash) { + int len, unsigned int hash) { int count = 0; for (HashtableEntry* l = bucket(index); l != NULL; l = l->next()) { count++; if (l->hash() == hash) { - if (java_lang_String::equals(l->literal(), name, len)) { - return l->literal(); + if (java_lang_String::equals(string_object_no_keepalive(l), name, len)) { + // We must perform a new load with string_object() that keeps the string + // alive as we must expose the oop as strongly reachable when exiting + // this context, in case the oop gets published. + return string_object(l); } } } @@ -192,18 +211,6 @@ return lookup(chars, length); } -// Tell the GC that this string was looked up in the StringTable. -static void ensure_string_alive(oop string) { - // A lookup in the StringTable could return an object that was previously - // considered dead. The SATB part of G1 needs to get notified about this - // potential resurrection, otherwise the marking might not find the object. -#if INCLUDE_ALL_GCS - if (UseG1GC && string != NULL) { - G1SATBCardTableModRefBS::enqueue(string); - } -#endif -} - oop StringTable::lookup(jchar* name, int len) { // shared table always uses java_lang_String::hash_code unsigned int hash = java_lang_String::hash_code(name, len); @@ -217,8 +224,6 @@ int index = the_table()->hash_to_index(hash); string = the_table()->lookup_in_main_table(index, name, len, hash); - ensure_string_alive(string); - return string; } @@ -238,9 +243,6 @@ // Found if (found_string != NULL) { - if (found_string != string_or_null()) { - ensure_string_alive(found_string); - } return found_string; } @@ -276,10 +278,6 @@ hashValue, CHECK_NULL); } - if (added_or_found != string()) { - ensure_string_alive(added_or_found); - } - return added_or_found; } @@ -388,9 +386,9 @@ while (entry != NULL) { assert(!entry->is_shared(), "CDS not used for the StringTable"); - if (is_alive->do_object_b(entry->literal())) { + if (is_alive->do_object_b(string_object_no_keepalive(entry))) { if (f != NULL) { - f->do_oop((oop*)entry->literal_addr()); + f->do_oop(entry->literal_addr()); } p = entry->next_addr(); } else { @@ -429,7 +427,7 @@ for (int i = 0; i < the_table()->table_size(); ++i) { HashtableEntry* p = the_table()->bucket(i); for ( ; p != NULL; p = p->next()) { - oop s = p->literal(); + oop s = string_object_no_keepalive(p); guarantee(s != NULL, "interned string is NULL"); unsigned int h = hash_string(s); guarantee(p->hash() == h, "broken hash in string table entry"); @@ -448,10 +446,10 @@ for (int i = 0; i < the_table()->table_size(); ++i) { HashtableEntry* p = the_table()->bucket(i); for ( ; p != NULL; p = p->next()) { - oop s = p->literal(); - typeArrayOop value = java_lang_String::value(s); - int length = java_lang_String::length(s); - bool is_latin1 = java_lang_String::is_latin1(s); + oop s = string_object_no_keepalive(p); + typeArrayOop value = java_lang_String::value_no_keepalive(s); + int length = java_lang_String::length(s); + bool is_latin1 = java_lang_String::is_latin1(s); if (length <= 0) { st->print("%d: ", length); @@ -484,8 +482,8 @@ HashtableEntry* e_ptr2) { // These entries are sanity checked by verify_and_compare_entries() // before this function is called. - oop str1 = e_ptr1->literal(); - oop str2 = e_ptr2->literal(); + oop str1 = string_object_no_keepalive(e_ptr1); + oop str2 = string_object_no_keepalive(e_ptr2); if (str1 == str2) { tty->print_cr("ERROR: identical oop values (0x" PTR_FORMAT ") " @@ -505,12 +503,12 @@ } StringTable::VerifyRetTypes StringTable::verify_entry(int bkt, int e_cnt, - HashtableEntry* e_ptr, - StringTable::VerifyMesgModes mesg_mode) { + HashtableEntry* e_ptr, + StringTable::VerifyMesgModes mesg_mode) { VerifyRetTypes ret = _verify_pass; // be optimistic - oop str = e_ptr->literal(); + oop str = string_object_no_keepalive(e_ptr); if (str == NULL) { if (mesg_mode == _verify_with_mesgs) { tty->print_cr("ERROR: NULL oop value in entry @ bucket[%d][%d]", bkt, @@ -684,7 +682,7 @@ assert(DumpSharedSpaces, "this function is only used with -Xshare:dump"); oop new_s = NULL; - typeArrayOop v = java_lang_String::value(s); + typeArrayOop v = java_lang_String::value_no_keepalive(s); typeArrayOop new_v = (typeArrayOop)MetaspaceShared::archive_heap_object(v, THREAD); if (new_v == NULL) { return NULL; @@ -708,7 +706,7 @@ for (int i = 0; i < the_table()->table_size(); ++i) { HashtableEntry* bucket = the_table()->bucket(i); for ( ; bucket != NULL; bucket = bucket->next()) { - oop s = bucket->literal(); + oop s = string_object_no_keepalive(bucket); unsigned int hash = java_lang_String::hash_code(s); if (hash == 0) { continue; @@ -721,7 +719,7 @@ } // set the archived string in bucket - bucket->set_literal(new_s); + set_string_object(bucket, new_s); // add to the compact table writer->add(hash, new_s); @@ -763,4 +761,3 @@ _shared_table.oops_do(f); } #endif //INCLUDE_CDS_JAVA_HEAP - --- old/src/hotspot/share/classfile/stringTable.hpp 2017-11-30 14:07:11.904262614 +0100 +++ new/src/hotspot/share/classfile/stringTable.hpp 2017-11-30 14:07:11.544262626 +0100 @@ -76,6 +76,13 @@ static unsigned int hash_string(oop string); static unsigned int alt_hash_string(const jchar* s, int len); + // Accessors for the string roots in the hashtable entries. + // Use string_object_no_keepalive() only when the value is not returned + // outside of a scope where a thread transition is possible. + static oop string_object(HashtableEntry* entry); + static oop string_object_no_keepalive(HashtableEntry* entry); + static void set_string_object(HashtableEntry* entry, oop string); + StringTable() : RehashableHashtable((int)StringTableSize, sizeof (HashtableEntry)) {} --- old/src/hotspot/share/oops/oop.hpp 2017-11-30 14:07:13.012262575 +0100 +++ new/src/hotspot/share/oops/oop.hpp 2017-11-30 14:07:12.668262587 +0100 @@ -28,6 +28,7 @@ #include "gc/shared/specialized_oop_closures.hpp" #include "memory/iterator.hpp" #include "memory/memRegion.hpp" +#include "oops/access.hpp" #include "oops/metadata.hpp" #include "utilities/macros.hpp" @@ -178,6 +179,8 @@ static inline void encode_store_heap_oop(oop* p, oop v); // Access to fields in a instanceOop through these methods. + template + oop obj_field_special(int offset) const; oop obj_field(int offset) const; void obj_field_put(int offset, oop value); void obj_field_put_raw(int offset, oop value); --- old/src/hotspot/share/oops/oop.inline.hpp 2017-11-30 14:07:14.224262533 +0100 +++ new/src/hotspot/share/oops/oop.inline.hpp 2017-11-30 14:07:13.864262546 +0100 @@ -326,7 +326,10 @@ *p = encode_heap_oop(v); } +template +inline oop oopDesc::obj_field_special(int offset) const { return HeapAccess::oop_load_at(as_oop(), offset); } inline oop oopDesc::obj_field(int offset) const { return HeapAccess<>::oop_load_at(as_oop(), offset); } + inline void oopDesc::obj_field_put(int offset, oop value) { HeapAccess<>::oop_store_at(as_oop(), offset, value); } inline jbyte oopDesc::byte_field(int offset) const { return HeapAccess<>::load_at(as_oop(), offset); }