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