RFR (M) 8207359: Make SymbolTable increment_refcount disallow zero

[Ioi Lam]

Hi Coleen,

The changes look good! The new operations on _length_and_refcount are 
much cleaner than my old ATOMIC_SHORT_PAIR hack.

symbolTable.cpp:

  SymbolTable::lookup_dynamic() {
  ...
  214       Symbol* sym = e->literal();
  215       if (sym->equals(name, len) && sym->try_increment_refcount()) {
  216         // something is referencing this symbol now.
  217         return sym;
  218       }


symbol.cpp:

  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");
  228     }
  229     NOT_PRODUCT(Atomic::inc(&_total_count);)
  230   }
  231 }

  246 // Atomically increment while checking for zero, zero is bad.
  247 bool Symbol::try_increment_refcount() {
  248   uint32_t old_value = _length_and_refcount;  // fetch once
  249   int refc = extract_refcount(old_value);
  250
  251   if (refc == PERM_REFCOUNT) {
  252     return true;
  253   } else if (refc == 0) {
  254     return false; // effectively dead, can't revive
  255   }
  256
  257   uint32_t now;
  258   while ((now = Atomic::cmpxchg(old_value + 1, 
&_length_and_refcount, old_value)) != old_value) {
  259     // failed to increment, check refcount again.
  260     refc = extract_refcount(now);
  261     if (refc == 0) {
  262       return false; // just died
  263     } else if (refc == PERM_REFCOUNT) {
  264       return true; // just became permanent
  265     }
  266     old_value = now; // refcount changed, try again
  267   }
  268   return true;
  269 }


So is it valid for Symbol::try_increment_refcount() to return false? 
SymbolTable::lookup_dynamic() seems to suggest YES, but 
Symbol::increment_refcount() seems to suggest NO.

If it's always an invalid condition, I think the fatal() should be moved 
inside try_increment_refcount.

Otherwise, I think you need to add comments in all 3 places, to say when 
it's possible to get a 0 refcount, and when it's not. And, it might be 
worth expanding on why "zero is bad" :-)

My guess is:
+ if you're doing a lookup, you might be seeing Symbols that have 
already been marked for deletion, which is indicated by a 0 refcount. 
You want to skip such Symbols.

+ if you're incrementing the refcount, that means you're holding a valid 
Symbol, which means this Symbol should have never been marked for deletion.

Is this correct?

Thanks
- Ioi


On 7/17/18 2:08 PM, coleen.phillimore at oracle.com wrote:
>
> Gerard, thank you for the code review.
>
> On 7/17/18 4:13 PM, Gerard Ziemski wrote:
>> 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
>
> Yes, I'll update with my commit.
>>
>> #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?
>
> Yes, it's so that we can store the length of the symbol into 16 bits.
>
> How I change the comment above max_symbol_length from:
>
>     // max_symbol_length is constrained by type of _length
>
> to
>
>     // max_symbol_length must fit into the top 16 bits of 
> _length_and_refcount
>
>>
>> #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
>>
> I can change PERM_REFCOUNT to ((1 << 16)) -1) to be consistent.
>
>> #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?
>>
>
> I was thought that looked strange too.  I'll move the #endif from 226 
> to after 227.
>
> Thank you for reviewing the code!
> Coleen
>
>> 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
>