RFR (M) 8207359: Make SymbolTable increment_refcount disallow zero

coleen.phillimore at oracle.com coleen.phillimore at oracle.com
Tue Jul 17 21:08:40 UTC 2018


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



More information about the hotspot-runtime-dev mailing list