8207359: Make SymbolTable increment_refcount disallow zero

Use cmpxchg for non permanent symbol refcounting, and pack refcount and length into an int.

Reviewed-by: gziemski, kbarrett, iklam

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

@@ -212,7 +212,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);
+  GEN_OFFS(Symbol, _length_and_refcount);
   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);
+  copyin_offset(OFFSET_Symbol_length_and_refcount);
   copyin_offset(OFFSET_Symbol_body);
 
   copyin_offset(OFFSET_Method_constMethod);
@@ -463,15 +463,17 @@ 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);
+      OFFSET_Symbol_length_and_refcount);
 
   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);
+      OFFSET_Symbol_length_and_refcount);
 
   this->klassPtr = copyin_ptr(this->constantPool +
       OFFSET_ConstantPool_pool_holder);
@@ -479,8 +481,9 @@ dtrace:helper:ustack:
   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);
+      OFFSET_Symbol_length_and_refcount);
 
   /*
    * Enough for three strings, plus the '.', plus the trailing '\0'.

src/hotspot/share/classfile/compactHashtable.inline.hpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2015, 2016, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2015, 2018, 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
@@ -36,7 +36,7 @@ inline Symbol* CompactHashtable<T, N>::decode_entry(CompactHashtable<Symbol*, ch
                                                     u4 offset, const char* name, int len) {
   Symbol* sym = (Symbol*)(_base_address + offset);
   if (sym->equals(name, len)) {
-    assert(sym->refcount() == -1, "must be shared");
+    assert(sym->refcount() == PERM_REFCOUNT, "must be shared");
     return sym;
   }
 

src/hotspot/share/classfile/symbolTable.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1997, 2017, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1997, 2018, 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
@@ -190,6 +190,7 @@ void SymbolTable::rehash_table() {
   assert(SafepointSynchronize::is_at_safepoint(), "must be at safepoint");
   // This should never happen with -Xshare:dump but it might in testing mode.
   if (DumpSharedSpaces) return;
+
   // Create a new symbol table
   SymbolTable* new_table = new SymbolTable();
 
@@ -212,10 +213,16 @@ Symbol* SymbolTable::lookup_dynamic(int index, const char* name,
     count++;  // count all entries in this bucket, not just ones with same hash
     if (e->hash() == hash) {
       Symbol* sym = e->literal();
-      if (sym->equals(name, len)) {
-        // something is referencing this symbol now.
-        sym->increment_refcount();
-        return sym;
+      // Skip checking already dead symbols in the bucket.
+      if (sym->refcount() == 0) {
+        count--;   // Don't count this symbol towards rehashing.
+      } else if (sym->equals(name, len)) {
+        if (sym->try_increment_refcount()) {
+          // something is referencing this symbol now.
+          return sym;
+        } else {
+          count--;   // don't count this symbol.
+        }
       }
     }
   }

src/hotspot/share/classfile/systemDictionary.cpp

@@ -2226,12 +2226,14 @@ bool SystemDictionary::add_loader_constraint(Symbol* class_name,
   ClassLoaderData* loader_data2 = class_loader_data(class_loader2);
 
   Symbol* constraint_name = NULL;
+  // Needs to be in same scope as constraint_name in case a Symbol is created and
+  // assigned to constraint_name.
+  FieldArrayInfo fd;
   if (!FieldType::is_array(class_name)) {
     constraint_name = class_name;
   } else {
     // For array classes, their Klass*s are not kept in the
     // constraint table. The element classes are.
-    FieldArrayInfo fd;
     BasicType t = FieldType::get_array_info(class_name, fd, CHECK_(false));
     // primitive types always pass
     if (t != T_OBJECT) {

src/hotspot/share/oops/symbol.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1997, 2017, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1997, 2018, 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
@@ -34,11 +34,22 @@
 #include "runtime/atomic.hpp"
 #include "runtime/os.hpp"
 
+uint32_t Symbol::pack_length_and_refcount(int length, int refcount) {
+  STATIC_ASSERT(max_symbol_length == ((1 << 16) - 1));
+  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 lo = refcount;
+  return (hi << 16) | lo;
+}
+
 Symbol::Symbol(const u1* name, int length, int refcount) {
-  _refcount = refcount;
-  _length = length;
+  _length_and_refcount =  pack_length_and_refcount(length, refcount);
   _identity_hash = (short)os::random();
-  for (int i = 0; i < _length; i++) {
+  for (int i = 0; i < length; i++) {
     byte_at_put(i, name[i]);
   }
 }
@@ -207,26 +218,68 @@ unsigned int Symbol::new_hash(juint seed) {
   return AltHashing::murmur3_32(seed, (const jbyte*)as_C_string(), utf8_length());
 }
 
-void Symbol::increment_refcount() {
-  // Only increment the refcount if non-negative.  If negative either
-  // overflow has occurred or it is a permanent symbol in a read only
-  // shared archive.
-  if (_refcount >= 0) { // not a permanent symbol
-    Atomic::inc(&_refcount);
-    NOT_PRODUCT(Atomic::inc(&_total_count);)
+// Increment refcount while checking for zero.  If the Symbol's refcount becomes zero
+// 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;
+  while (true) {
+    uint32_t old_value = found;
+    int refc = extract_refcount(old_value);
+    if (refc == PERM_REFCOUNT) {
+      return true;  // sticky max or created permanent
+    } else if (refc == 0) {
+      return false; // dead, can't revive.
+    } else {
+      found = Atomic::cmpxchg(old_value + 1, &_length_and_refcount, old_value);
+      if (found == old_value) {
+        return true; // successfully updated.
+      }
+      // refcount changed, try again.
+    }
   }
 }
 
-void Symbol::decrement_refcount() {
-  if (_refcount >= 0) { // not a permanent symbol
-    short new_value = Atomic::add(short(-1), &_refcount);
+// The increment_refcount() is called when not doing lookup. It is assumed that you
+// have a symbol with a non-zero refcount and it can't become zero while referenced by
+// this caller.
+void Symbol::increment_refcount() {
+  if (!try_increment_refcount()) {
 #ifdef ASSERT
-    if (new_value == -1) { // we have transitioned from 0 -> -1
-      print();
-      assert(false, "reference count underflow for symbol");
-    }
+    print();
+    fatal("refcount has gone to zero");
 #endif
-    (void)new_value;
+  }
+#ifndef PRODUCT
+  if (refcount() != PERM_REFCOUNT) { // not a permanent symbol
+    NOT_PRODUCT(Atomic::inc(&_total_count);)
+  }
+#endif
+}
+
+// Decrement refcount potentially while racing increment, so we need
+// 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;
+  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 {
+      found = Atomic::cmpxchg(old_value - 1, &_length_and_refcount, old_value);
+      if (found == old_value) {
+        return;  // successfully updated.
+      }
+      // refcount changed, try again.
+    }
   }
 }
 

src/hotspot/share/oops/symbol.hpp

@@ -96,9 +96,9 @@
 // type without virtual functions.
 class ClassLoaderData;
 
-// Set _refcount to PERM_REFCOUNT to prevent the Symbol from being GC'ed.
+// Set _refcount to PERM_REFCOUNT to prevent the Symbol from being freed.
 #ifndef PERM_REFCOUNT
-#define PERM_REFCOUNT -1
+#define PERM_REFCOUNT ((1 << 16) - 1)
 #endif
 
 class Symbol : public MetaspaceObj {
@@ -107,15 +107,15 @@ class Symbol : public MetaspaceObj {
   friend class MoveSymbols;
 
  private:
-  ATOMIC_SHORT_PAIR(
-    volatile short _refcount,  // needs atomic operation
-    unsigned short _length     // number of UTF8 characters in the symbol (does not need atomic op)
-  );
+
+  // This is an int because it needs atomic operation on the refcount.  Mask length
+  // in high half word. length is the number of UTF8 characters in the symbol
+  volatile uint32_t _length_and_refcount;
   short _identity_hash;
   jbyte _body[2];
 
   enum {
-    // max_symbol_length is constrained by type of _length
+    // max_symbol_length must fit into the top 16 bits of _length_and_refcount
     max_symbol_length = (1 << 16) -1
   };
 
@@ -129,7 +129,7 @@ class Symbol : public MetaspaceObj {
   }
 
   void byte_at_put(int index, int value) {
-    assert(index >=0 && index < _length, "symbol index overflow");
+    assert(index >=0 && index < length(), "symbol index overflow");
     _body[index] = value;
   }
 
@@ -140,6 +140,12 @@ class Symbol : public MetaspaceObj {
 
   void  operator delete(void* p);
 
+  static int extract_length(uint32_t value)   { return value >> 16; }
+  static int extract_refcount(uint32_t value) { return value & 0xffff; }
+  static uint32_t pack_length_and_refcount(int length, int refcount);
+
+  int length() const   { return extract_length(_length_and_refcount); }
+
  public:
   // Low-level access (used with care, since not GC-safe)
   const jbyte* base() const { return &_body[0]; }
@@ -155,28 +161,29 @@ class Symbol : public MetaspaceObj {
   unsigned identity_hash() const {
     unsigned addr_bits = (unsigned)((uintptr_t)this >> (LogMinObjAlignmentInBytes + 3));
     return ((unsigned)_identity_hash & 0xffff) |
-           ((addr_bits ^ (_length << 8) ^ (( _body[0] << 8) | _body[1])) << 16);
+           ((addr_bits ^ (length() << 8) ^ (( _body[0] << 8) | _body[1])) << 16);
   }
 
   // For symbol table alternate hashing
   unsigned int new_hash(juint seed);
 
   // Reference counting.  See comments above this class for when to use.
-  int refcount() const      { return _refcount; }
+  int refcount() const { return extract_refcount(_length_and_refcount); }
+  bool try_increment_refcount();
   void increment_refcount();
   void decrement_refcount();
   bool is_permanent() {
-    return (_refcount == PERM_REFCOUNT);
+    return (refcount() == PERM_REFCOUNT);
   }
 
   int byte_at(int index) const {
-    assert(index >=0 && index < _length, "symbol index overflow");
+    assert(index >=0 && index < length(), "symbol index overflow");
     return base()[index];
   }
 
   const jbyte* bytes() const { return base(); }
 
-  int utf8_length() const { return _length; }
+  int utf8_length() const { return length(); }
 
   // Compares the symbol with a string.
   bool equals(const char* str, int len) const {

src/hotspot/share/runtime/vmStructs.cpp

@@ -330,9 +330,8 @@ typedef PaddedEnd<ObjectMonitor>              PaddedObjectMonitor;
   nonstatic_field(ConstMethod,                 _size_of_parameters,                           u2)                                    \
   nonstatic_field(ObjArrayKlass,               _element_klass,                                Klass*)                                \
   nonstatic_field(ObjArrayKlass,               _bottom_klass,                                 Klass*)                                \
-  volatile_nonstatic_field(Symbol,             _refcount,                                     short)                                 \
+  volatile_nonstatic_field(Symbol,             _length_and_refcount,                          unsigned int)                          \
   nonstatic_field(Symbol,                      _identity_hash,                                short)                                 \
-  nonstatic_field(Symbol,                      _length,                                       unsigned short)                        \
   unchecked_nonstatic_field(Symbol,            _body,                                         sizeof(jbyte)) /* NOTE: no type */     \
   nonstatic_field(Symbol,                      _body[0],                                      jbyte)                                 \
   nonstatic_field(TypeArrayKlass,              _max_length,                                   jint)                                  \

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

@@ -552,7 +552,8 @@ 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
-  err = ps_pread(J->P, nameSymbol + OFFSET_Symbol_length, &nameSymbolLength, 2);
+  // The length is in the top half of the word.
+  err = ps_pread(J->P, nameSymbol + OFFSET_Symbol_length_and_refcount, &nameSymbolLength, 2);
   CHECK_FAIL(err);
   nameString = (char*)calloc(nameSymbolLength + 1, 1);
   err = ps_pread(J->P, nameSymbol + OFFSET_Symbol_body, nameString, nameSymbolLength);
@@ -564,7 +565,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, &signatureSymbolLength, 2);
+  err = ps_pread(J->P, signatureSymbol + OFFSET_Symbol_length_and_refcount, &signatureSymbolLength, 2);
   CHECK_FAIL(err);
   signatureString = (char*)calloc(signatureSymbolLength + 1, 1);
   err = ps_pread(J->P, signatureSymbol + OFFSET_Symbol_body, signatureString, signatureSymbolLength);
@@ -575,7 +576,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, &klassSymbolLength, 2);
+  err = ps_pread(J->P, klassSymbol + OFFSET_Symbol_length_and_refcount, &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,17 +45,11 @@ public class Symbol extends VMObject {
 
   private static synchronized void initialize(TypeDataBase db) throws WrongTypeException {
     Type type  = db.lookupType("Symbol");
-    length     = type.getCIntegerField("_length");
+    lengthAndRefcount = type.getCIntegerField("_length_and_refcount");
     baseOffset = type.getField("_body").getOffset();
     idHash = type.getCIntegerField("_identity_hash");
   }
 
-  // Format:
-  //   [header]
-  //   [klass ]
-  //   [length] byte size of uft8 string
-  //   ..body..
-
   public static Symbol create(Address addr) {
     if (addr == null) {
       return null;
@@ -72,10 +66,13 @@ public class Symbol extends VMObject {
   private static long baseOffset; // tells where the array part starts
 
   // Fields
-  private static CIntegerField length;
+  private static CIntegerField lengthAndRefcount;
 
   // Accessors for declared fields
-  public long   getLength() { return          length.getValue(this.addr); }
+  public long getLength() {
+    long i = lengthAndRefcount.getValue(this.addr);
+    return (i >> 16) & 0xffff;
+  }
 
   public byte getByteAt(long index) {
     return addr.getJByteAt(baseOffset + index);

test/hotspot/gtest/classfile/test_symbolTable.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2016, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2016, 2018, 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
@@ -24,6 +24,7 @@
 #include "precompiled.hpp"
 #include "runtime/interfaceSupport.inline.hpp"
 #include "classfile/symbolTable.hpp"
+#include "threadHelper.inline.hpp"
 #include "unittest.hpp"
 
 TEST_VM(SymbolTable, temp_new_symbol) {
@@ -74,4 +75,76 @@ TEST_VM(SymbolTable, temp_new_symbol) {
   }
   ASSERT_EQ(xyz->refcount(), xyzcount - 1)
           << "Should have been decremented by dtor in inner scope";
+
+  // Test overflowing refcount making symbol permanent
+  Symbol* bigsym = SymbolTable::new_symbol("bigsym", CATCH);
+  for (int i = 0; i < PERM_REFCOUNT + 100; i++) {
+    bigsym->increment_refcount();
+  }
+  ASSERT_EQ(bigsym->refcount(), PERM_REFCOUNT) << "should not have overflowed";
+
+  // Test that PERM_REFCOUNT is sticky
+  for (int i = 0; i < 10; i++) {
+    bigsym->decrement_refcount();
+  }
+  ASSERT_EQ(bigsym->refcount(), PERM_REFCOUNT) << "should be sticky";
+}
+
+// TODO: Make two threads one decrementing the refcount and the other trying to increment.
+// try_increment_refcount should return false
+
+#define SYM_NAME_LENGTH 30
+static char symbol_name[SYM_NAME_LENGTH];
+
+class SymbolThread : public JavaTestThread {
+  public:
+  SymbolThread(Semaphore* post) : JavaTestThread(post) {}
+  virtual ~SymbolThread() {}
+  void main_run() {
+    Thread* THREAD = Thread::current();
+    for (int i = 0; i < 1000; i++) {
+      TempNewSymbol sym = SymbolTable::new_symbol(symbol_name, CATCH);
+      // Create and destroy new symbol
+      EXPECT_TRUE(sym->refcount() != 0) << "Symbol refcount unexpectedly zeroed";
+    }
+  }
+};
+
+#define SYM_TEST_THREAD_COUNT 5
+
+class DriverSymbolThread : public JavaTestThread {
+public:
+  Semaphore _done;
+  DriverSymbolThread(Semaphore* post) : JavaTestThread(post) { };
+  virtual ~DriverSymbolThread(){}
+
+  void main_run() {
+    Semaphore done(0);
+
+    Thread* THREAD = Thread::current();
+
+    // Find a symbol where there will probably be only one instance.
+    for (int i = 0; i < 100; i++) {
+       snprintf(symbol_name, SYM_NAME_LENGTH, "some_symbol%d", i);
+       TempNewSymbol ts = SymbolTable::new_symbol(symbol_name, CATCH);
+       if (ts->refcount() == 1) {
+         EXPECT_TRUE(ts->refcount() == 1) << "Symbol is just created";
+         break;  // found a unique symbol
+       }
+    }
+
+    SymbolThread* st[SYM_TEST_THREAD_COUNT];
+    for (int i = 0; i < SYM_TEST_THREAD_COUNT; i++) {
+      st[i] = new SymbolThread(&done);
+      st[i]->doit();
+    }
+
+    for (int i = 0; i < 4; i++) {
+      done.wait();
+    }
+  }
+};
+
+TEST_VM(SymbolTable, test_symbol_refcount_parallel) {
+  mt_test_doer<DriverSymbolThread>();
 }

test/hotspot/gtest/threadHelper.inline.hpp

@@ -21,8 +21,8 @@
  * questions.
  */
 
-#ifndef GTEST_UTILITIES_HELPER_INLINE_HPP
-#define GTEST_UTILITIES_HELPER_INLINE_HPP
+#ifndef GTEST_THREADHELPER_INLINE_HPP
+#define GTEST_THREADHELPER_INLINE_HPP
 
 #include "runtime/mutex.hpp"
 #include "runtime/semaphore.hpp"

test/hotspot/gtest/utilities/test_concurrentHashtable.cpp

@@ -29,7 +29,7 @@
 #include "runtime/vm_operations.hpp"
 #include "utilities/concurrentHashTable.inline.hpp"
 #include "utilities/concurrentHashTableTasks.inline.hpp"
-#include "utilitiesHelper.inline.hpp"
+#include "threadHelper.inline.hpp"
 #include "unittest.hpp"
 
 // NOTE: On win32 gtest asserts are not mt-safe.

test/hotspot/gtest/utilities/test_globalCounter.cpp

@@ -27,7 +27,7 @@
 #include "runtime/os.hpp"
 #include "utilities/globalCounter.hpp"
 #include "utilities/globalCounter.inline.hpp"
-#include "utilitiesHelper.inline.hpp"
+#include "threadHelper.inline.hpp"
 
 #define GOOD_VALUE 1337
 #define BAD_VALUE  4711