RFR (M) 8207359: Make SymbolTable increment_refcount disallow zero

[Gerard Ziemski]

Thank you Coleen (and Kim)!

#1 Need copyright year updates:

src/hotspot/share/oops/symbol.cpp
src/hotspot/share/classfile/symbolTable.cpp
src/hotspot/share/classfile/compactHashtable.inline.hpp


#2 What’s the purpose of this code in src/hotspot/share/oops/symbol.cpp 

  38   STATIC_ASSERT(max_symbol_length == ((1 << 16) - 1));

when we have:

 117   enum {
 118     // max_symbol_length is constrained by type of _length
 119     max_symbol_length = (1 << 16) -1
 120   };

Wouldn’t that always be true?  Is it to make sure that nobody changes max_symbol_length, because the implementation needs it to be that? If so, should we add comment to:

 119     max_symbol_length = (1 << 16) -1

with a big warning of some sorts?


#3 If we have:

  39   STATIC_ASSERT(PERM_REFCOUNT == ((1 << 16) - 1));

then why not  

 101 #define PERM_REFCOUNT ((1 << 16) - 1)) // 0xffff

or 
  39   STATIC_ASSERT(PERM_REFCOUNT == 0xffff;
 101 #define PERM_REFCOUNT 0xffff


#4 We have:

 221 void Symbol::increment_refcount() {
 222   if (refcount() != PERM_REFCOUNT) { // not a permanent symbol
 223     if (!try_increment_refcount()) {
 224 #ifdef ASSERT
 225       print();
 226 #endif
 227       fatal("refcount has gone to zero");

but

 233 void Symbol::decrement_refcount() {
 234   if (refcount() != PERM_REFCOUNT) { // not a permanent symbol
 235     int new_value = Atomic::sub((uint32_t)1, &_length_and_refcount);
 236 #ifdef ASSERT
 237     // Check if we have transitioned to 0xffff
 238     if (extract_refcount(new_value) == PERM_REFCOUNT) {
 239       print();
 240       fatal("refcount underflow");
 241     }
 242 #endif

Where the line:

 240       fatal("refcount underflow”);

is inside #ifdef ASSERT, but:

227       fatal("refcount has gone to zero”);

is outside. Shouldn't “fatal" be consistent in both?


cheers


> On Jul 17, 2018, at 10:51 AM, coleen.phillimore at oracle.com wrote:
> 
> Summary: Use cmpxchg for non permanent symbol refcounting, and pack refcount and length into an int.
> 
> This is a precurser change to the concurrent SymbolTable change. Zeroed refcounted entries can be deleted at anytime so they cannot be allowed to be zero in runtime code.  Thanks to Kim for writing the packing function and helping me avoid undefined behavior.
> 
> open webrev at http://cr.openjdk.java.net/~coleenp/8207359.01/webrev
> bug link https://bugs.openjdk.java.net/browse/JDK-8207359
> 
> Tested with solaris ptrace helper, mach5 tier1-5 including solaris. Added multithreaded gtest which exercises the code.
> 
> Thanks,
> Coleen