8192003: Refactor weak references in StringTable to use the Access API
authoreosterlund
Mon, 08 Jan 2018 16:21:23 +0100
changeset 48618 688e5cbd0b91
parent 48617 01b07229a6ad
child 48620 7f97d35fac6e
8192003: Refactor weak references in StringTable to use the Access API Reviewed-by: pliden, dholmes, coleenp
src/hotspot/share/classfile/javaClasses.cpp
src/hotspot/share/classfile/javaClasses.hpp
src/hotspot/share/classfile/javaClasses.inline.hpp
src/hotspot/share/classfile/stringTable.cpp
src/hotspot/share/classfile/stringTable.hpp
src/hotspot/share/oops/oop.hpp
src/hotspot/share/oops/oop.inline.hpp
--- a/src/hotspot/share/classfile/javaClasses.cpp	Mon Jan 08 09:58:38 2018 -0500
+++ b/src/hotspot/share/classfile/javaClasses.cpp	Mon Jan 08 16:21:23 2018 +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
--- a/src/hotspot/share/classfile/javaClasses.hpp	Mon Jan 08 09:58:38 2018 -0500
+++ b/src/hotspot/share/classfile/javaClasses.hpp	Mon Jan 08 16:21:23 2018 +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);
--- a/src/hotspot/share/classfile/javaClasses.inline.hpp	Mon Jan 08 09:58:38 2018 -0500
+++ b/src/hotspot/share/classfile/javaClasses.inline.hpp	Mon Jan 08 16:21:23 2018 +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_access<AS_NO_KEEPALIVE>(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
--- a/src/hotspot/share/classfile/stringTable.cpp	Mon Jan 08 09:58:38 2018 -0500
+++ b/src/hotspot/share/classfile/stringTable.cpp	Mon Jan 08 16:21:23 2018 +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<oop, mtSymbol>* entry) {
+  return RootAccess<ON_PHANTOM_OOP_REF>::oop_load(entry->literal_addr());
+}
+
+oop StringTable::string_object_no_keepalive(HashtableEntry<oop, mtSymbol>* 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<ON_PHANTOM_OOP_REF | AS_NO_KEEPALIVE>::oop_load(entry->literal_addr());
+}
+
+void StringTable::set_string_object(HashtableEntry<oop, mtSymbol>* entry, oop string) {
+  RootAccess<ON_PHANTOM_OOP_REF>::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<oop, mtSymbol>* 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<oop, mtSymbol>* 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<oop, mtSymbol>* 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<oop, mtSymbol>* 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<oop, mtSymbol>* e_ptr,
-                                      StringTable::VerifyMesgModes mesg_mode) {
+                                                      HashtableEntry<oop, mtSymbol>* 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<oop, mtSymbol>* 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
-
--- a/src/hotspot/share/classfile/stringTable.hpp	Mon Jan 08 09:58:38 2018 -0500
+++ b/src/hotspot/share/classfile/stringTable.hpp	Mon Jan 08 16:21:23 2018 +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<oop, mtSymbol>* entry);
+  static oop string_object_no_keepalive(HashtableEntry<oop, mtSymbol>* entry);
+  static void set_string_object(HashtableEntry<oop, mtSymbol>* entry, oop string);
+
   StringTable() : RehashableHashtable<oop, mtSymbol>((int)StringTableSize,
                               sizeof (HashtableEntry<oop, mtSymbol>)) {}
 
--- a/src/hotspot/share/oops/oop.hpp	Mon Jan 08 09:58:38 2018 -0500
+++ b/src/hotspot/share/oops/oop.hpp	Mon Jan 08 16:21:23 2018 +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 <DecoratorSet decorator>
+  oop obj_field_access(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);
--- a/src/hotspot/share/oops/oop.inline.hpp	Mon Jan 08 09:58:38 2018 -0500
+++ b/src/hotspot/share/oops/oop.inline.hpp	Mon Jan 08 16:21:23 2018 +0100
@@ -326,7 +326,10 @@
   *p = encode_heap_oop(v);
 }
 
+template <DecoratorSet decorators>
+inline oop  oopDesc::obj_field_access(int offset) const             { return HeapAccess<decorators>::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);  }