8239347: Refactor Symbol to make _length a standalone field again

Reviewed-by: iklam, coleenp

Author: cl4es

make/hotspot/src/native/dtrace/generateJvmOffsets.cpp

@@ -213,7 +213,7 @@ int generateJvmOffsets(GEN_variant gen_variant) {
   GEN_VALUE(AccessFlags_NATIVE, JVM_ACC_NATIVE);
   GEN_VALUE(ConstMethod_has_linenumber_table, ConstMethod::_has_linenumber_table);
   GEN_OFFS(AccessFlags, _flags);
-  GEN_OFFS(Symbol, _length_and_refcount);
+  GEN_OFFS(Symbol, _length);
   GEN_OFFS(Symbol, _body);
   printf("\n");
 

src/hotspot/os/solaris/dtrace/jhelper.d

@@ -111,7 +111,7 @@ dtrace:helper:ustack:
   copyin_offset(OFFSET_HeapBlockHeader_used);
   copyin_offset(OFFSET_oopDesc_metadata);
 
-  copyin_offset(OFFSET_Symbol_length_and_refcount);
+  copyin_offset(OFFSET_Symbol_length);
   copyin_offset(OFFSET_Symbol_body);
 
   copyin_offset(OFFSET_Method_constMethod);
@@ -463,27 +463,24 @@ dtrace:helper:ustack:
   /* The symbol is a CPSlot and has lower bit set to indicate metadata */
   this->nameSymbol &= (~1); /* remove metadata lsb */
 
-  /* Because sparc is big endian, the top half length is at the correct offset. */
   this->nameSymbolLength = copyin_uint16(this->nameSymbol +
-      OFFSET_Symbol_length_and_refcount);
+      OFFSET_Symbol_length);
 
   this->signatureSymbol = copyin_ptr(this->constantPool +
       this->signatureIndex * sizeof (pointer) + SIZE_ConstantPool);
   this->signatureSymbol &= (~1); /* remove metadata lsb */
 
-  /* Because sparc is big endian, the top half length is at the correct offset. */
   this->signatureSymbolLength = copyin_uint16(this->signatureSymbol +
-      OFFSET_Symbol_length_and_refcount);
+      OFFSET_Symbol_length);
 
   this->klassPtr = copyin_ptr(this->constantPool +
       OFFSET_ConstantPool_pool_holder);
 
   this->klassSymbol = copyin_ptr(this->klassPtr +
       OFFSET_Klass_name);
 
-  /* Because sparc is big endian, the top half length is at the correct offset. */
   this->klassSymbolLength = copyin_uint16(this->klassSymbol +
-      OFFSET_Symbol_length_and_refcount);
+      OFFSET_Symbol_length);
 
   /*
    * Enough for three strings, plus the '.', plus the trailing '\0'.

src/hotspot/share/oops/symbol.cpp

@@ -37,21 +37,18 @@
 #include "runtime/os.hpp"
 #include "utilities/utf8.hpp"
 
-uint32_t Symbol::pack_length_and_refcount(int length, int refcount) {
-  STATIC_ASSERT(max_symbol_length == ((1 << 16) - 1));
+uint32_t Symbol::pack_hash_and_refcount(short hash, int refcount) {
   STATIC_ASSERT(PERM_REFCOUNT == ((1 << 16) - 1));
-  assert(length >= 0, "negative length");
-  assert(length <= max_symbol_length, "too long symbol");
   assert(refcount >= 0, "negative refcount");
   assert(refcount <= PERM_REFCOUNT, "invalid refcount");
-  uint32_t hi = length;
+  uint32_t hi = hash;
   uint32_t lo = refcount;
   return (hi << 16) | lo;
 }
 
 Symbol::Symbol(const u1* name, int length, int refcount) {
-  _length_and_refcount =  pack_length_and_refcount(length, refcount);
-  _identity_hash = (short)os::random();
+  _hash_and_refcount =  pack_hash_and_refcount((short)os::random(), refcount);
+  _length = length;
   _body[0] = 0;  // in case length == 0
   for (int i = 0; i < length; i++) {
     byte_at_put(i, name[i]);
@@ -78,7 +75,7 @@ void Symbol::operator delete(void *p) {
 void Symbol::set_permanent() {
   // This is called at a safepoint during dumping of a dynamic CDS archive.
   assert(SafepointSynchronize::is_at_safepoint(), "must be at a safepoint");
-  _length_and_refcount =  pack_length_and_refcount(length(), PERM_REFCOUNT);
+  _hash_and_refcount =  pack_hash_and_refcount(extract_hash(_hash_and_refcount), PERM_REFCOUNT);
 }
 
 
@@ -282,7 +279,7 @@ void Symbol::print_as_signature_external_parameters(outputStream *os) {
 // a thread could be concurrently removing the Symbol.  This is used during SymbolTable
 // lookup to avoid reviving a dead Symbol.
 bool Symbol::try_increment_refcount() {
-  uint32_t found = _length_and_refcount;
+  uint32_t found = _hash_and_refcount;
   while (true) {
     uint32_t old_value = found;
     int refc = extract_refcount(old_value);
@@ -291,7 +288,7 @@ bool Symbol::try_increment_refcount() {
     } else if (refc == 0) {
       return false; // dead, can't revive.
     } else {
-      found = Atomic::cmpxchg(&_length_and_refcount, old_value, old_value + 1);
+      found = Atomic::cmpxchg(&_hash_and_refcount, old_value, old_value + 1);
       if (found == old_value) {
         return true; // successfully updated.
       }
@@ -321,7 +318,7 @@ void Symbol::increment_refcount() {
 // to check the value after attempting to decrement so that if another
 // thread increments to PERM_REFCOUNT the value is not decremented.
 void Symbol::decrement_refcount() {
-  uint32_t found = _length_and_refcount;
+  uint32_t found = _hash_and_refcount;
   while (true) {
     uint32_t old_value = found;
     int refc = extract_refcount(old_value);
@@ -334,7 +331,7 @@ void Symbol::decrement_refcount() {
 #endif
       return;
     } else {
-      found = Atomic::cmpxchg(&_length_and_refcount, old_value, old_value - 1);
+      found = Atomic::cmpxchg(&_hash_and_refcount, old_value, old_value - 1);
       if (found == old_value) {
         return;  // successfully updated.
       }
@@ -344,7 +341,7 @@ void Symbol::decrement_refcount() {
 }
 
 void Symbol::make_permanent() {
-  uint32_t found = _length_and_refcount;
+  uint32_t found = _hash_and_refcount;
   while (true) {
     uint32_t old_value = found;
     int refc = extract_refcount(old_value);
@@ -357,8 +354,8 @@ void Symbol::make_permanent() {
 #endif
       return;
     } else {
-      int len = extract_length(old_value);
-      found = Atomic::cmpxchg(&_length_and_refcount, old_value, pack_length_and_refcount(len, PERM_REFCOUNT));
+      int hash = extract_hash(old_value);
+      found = Atomic::cmpxchg(&_hash_and_refcount, old_value, pack_hash_and_refcount(hash, PERM_REFCOUNT));
       if (found == old_value) {
         return;  // successfully updated.
       }

src/hotspot/share/oops/symbol.hpp

@@ -106,14 +106,13 @@ class Symbol : public MetaspaceObj {
 
  private:
 
-  // This is an int because it needs atomic operation on the refcount.  Mask length
+  // This is an int because it needs atomic operation on the refcount.  Mask hash
   // in high half word. length is the number of UTF8 characters in the symbol
-  volatile uint32_t _length_and_refcount;
-  short _identity_hash;
+  volatile uint32_t _hash_and_refcount;
+  u2 _length;
   u1 _body[2];
 
   enum {
-    // max_symbol_length must fit into the top 16 bits of _length_and_refcount
     max_symbol_length = (1 << 16) -1
   };
 
@@ -137,11 +136,11 @@ class Symbol : public MetaspaceObj {
 
   void  operator delete(void* p);
 
-  static int extract_length(uint32_t value)   { return value >> 16; }
+  static short extract_hash(uint32_t value)   { return (short)(value >> 16); }
   static int extract_refcount(uint32_t value) { return value & 0xffff; }
-  static uint32_t pack_length_and_refcount(int length, int refcount);
+  static uint32_t pack_hash_and_refcount(short hash, int refcount);
 
-  int length() const   { return extract_length(_length_and_refcount); }
+  int length() const   { return _length; }
 
  public:
   // Low-level access (used with care, since not GC-safe)
@@ -157,12 +156,12 @@ class Symbol : public MetaspaceObj {
   static int max_length() { return max_symbol_length; }
   unsigned identity_hash() const {
     unsigned addr_bits = (unsigned)((uintptr_t)this >> (LogMinObjAlignmentInBytes + 3));
-    return ((unsigned)_identity_hash & 0xffff) |
+    return ((unsigned)extract_hash(_hash_and_refcount) & 0xffff) |
            ((addr_bits ^ (length() << 8) ^ (( _body[0] << 8) | _body[1])) << 16);
   }
 
   // Reference counting.  See comments above this class for when to use.
-  int refcount() const { return extract_refcount(_length_and_refcount); }
+  int refcount() const { return extract_refcount(_hash_and_refcount); }
   bool try_increment_refcount();
   void increment_refcount();
   void decrement_refcount();

src/hotspot/share/runtime/vmStructs.cpp

@@ -327,8 +327,8 @@ typedef HashtableEntry<InstanceKlass*, mtClass>  KlassHashtableEntry;
   nonstatic_field(ConstMethod,                 _size_of_parameters,                           u2)                                    \
   nonstatic_field(ObjArrayKlass,               _element_klass,                                Klass*)                                \
   nonstatic_field(ObjArrayKlass,               _bottom_klass,                                 Klass*)                                \
-  volatile_nonstatic_field(Symbol,             _length_and_refcount,                          unsigned int)                          \
-  nonstatic_field(Symbol,                      _identity_hash,                                short)                                 \
+  volatile_nonstatic_field(Symbol,             _hash_and_refcount,                            unsigned int)                          \
+  nonstatic_field(Symbol,                      _length,                                       u2)                                    \
   unchecked_nonstatic_field(Symbol,            _body,                                         sizeof(u1)) /* NOTE: no type */        \
   nonstatic_field(Symbol,                      _body[0],                                      u1)                                    \
   nonstatic_field(TypeArrayKlass,              _max_length,                                   jint)                                  \

src/java.base/solaris/native/libjvm_db/libjvm_db.c

@@ -552,8 +552,7 @@ name_for_methodPtr(jvm_agent_t* J, uint64_t methodPtr, char * result, size_t siz
   CHECK_FAIL(err);
   // The symbol is a CPSlot and has lower bit set to indicate metadata
   nameSymbol &= (~1); // remove metadata lsb
-  // The length is in the top half of the word.
-  err = ps_pread(J->P, nameSymbol + OFFSET_Symbol_length_and_refcount, &nameSymbolLength, 2);
+  err = ps_pread(J->P, nameSymbol + OFFSET_Symbol_length, &nameSymbolLength, 2);
   CHECK_FAIL(err);
   nameString = (char*)calloc(nameSymbolLength + 1, 1);
   err = ps_pread(J->P, nameSymbol + OFFSET_Symbol_body, nameString, nameSymbolLength);
@@ -565,7 +564,7 @@ name_for_methodPtr(jvm_agent_t* J, uint64_t methodPtr, char * result, size_t siz
   err = read_pointer(J, constantPool + signatureIndex * POINTER_SIZE + SIZE_ConstantPool, &signatureSymbol);
   CHECK_FAIL(err);
   signatureSymbol &= (~1);  // remove metadata lsb
-  err = ps_pread(J->P, signatureSymbol + OFFSET_Symbol_length_and_refcount, &signatureSymbolLength, 2);
+  err = ps_pread(J->P, signatureSymbol + OFFSET_Symbol_length, &signatureSymbolLength, 2);
   CHECK_FAIL(err);
   signatureString = (char*)calloc(signatureSymbolLength + 1, 1);
   err = ps_pread(J->P, signatureSymbol + OFFSET_Symbol_body, signatureString, signatureSymbolLength);
@@ -576,7 +575,7 @@ name_for_methodPtr(jvm_agent_t* J, uint64_t methodPtr, char * result, size_t siz
   CHECK_FAIL(err);
   err = read_pointer(J, klassPtr + OFFSET_Klass_name, &klassSymbol);
   CHECK_FAIL(err);
-  err = ps_pread(J->P, klassSymbol + OFFSET_Symbol_length_and_refcount, &klassSymbolLength, 2);
+  err = ps_pread(J->P, klassSymbol + OFFSET_Symbol_length, &klassSymbolLength, 2);
   CHECK_FAIL(err);
   klassString = (char*)calloc(klassSymbolLength + 1, 1);
   err = ps_pread(J->P, klassSymbol + OFFSET_Symbol_body, klassString, klassSymbolLength);

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/Symbol.java

@@ -45,9 +45,9 @@ public void update(Observable o, Object data) {
 
   private static synchronized void initialize(TypeDataBase db) throws WrongTypeException {
     Type type  = db.lookupType("Symbol");
-    lengthAndRefcount = type.getCIntegerField("_length_and_refcount");
+    lengthField = type.getCIntegerField("_length");
     baseOffset = type.getField("_body").getOffset();
-    idHash = type.getCIntegerField("_identity_hash");
+    idHashAndRefcount = type.getCIntegerField("_hash_and_refcount");
   }
 
   public static Symbol create(Address addr) {
@@ -66,19 +66,18 @@ public static Symbol create(Address addr) {
   private static long baseOffset; // tells where the array part starts
 
   // Fields
-  private static CIntegerField lengthAndRefcount;
+  private static CIntegerField lengthField;
+  // idHash is a short packed into the high bits of a 32-bit integer with refcount
+  private static CIntegerField idHashAndRefcount;
 
   // Accessors for declared fields
   public long getLength() {
-    long i = lengthAndRefcount.getValue(this.addr);
-    return (i >> 16) & 0xffff;
+    return lengthField.getValue(this.addr);
   }
 
   public byte getByteAt(long index) {
     return addr.getJByteAt(baseOffset + index);
   }
-  // _identity_hash is a short
-  private static CIntegerField idHash;
 
   public long identityHash() {
     long addr_value = getAddress().asLongValue();
@@ -87,7 +86,8 @@ public long identityHash() {
     int  length = (int)getLength();
     int  byte0 = getByteAt(0);
     int  byte1 = getByteAt(1);
-    long id_hash = 0xffffL & (long)idHash.getValue(this.addr);
+    long id_hash = (long)idHashAndRefcount.getValue(this.addr);
+    id_hash = (id_hash >> 16) & 0xffff;
     return (id_hash |
       ((addr_bits ^ (length << 8) ^ ((byte0 << 8) | byte1)) << 16)) & 0xffffffffL;
   }