RFR (S) [Graal] runtime/CommandLine/PrintTouchedMethods.java crashes with assertion "reference count underflow for symbol"
Ioi Lam
ioi.lam at oracle.com
Mon Mar 26 05:18:34 UTC 2018
On 3/25/18 4:28 PM, David Holmes wrote:
> On 26/03/2018 1:23 AM, coleen.phillimore at oracle.com wrote:
>> On 3/25/18 9:08 AM, David Holmes wrote:
>>> Hi Ioi,
>>>
>>> On 24/03/2018 7:38 AM, Ioi Lam wrote:
>>>> https://bugs.openjdk.java.net/browse/JDK-8199793
>>>> http://cr.openjdk.java.net/~iklam/jdk11/8199793-PrintTouchedMethods-crash.v01/
>>>>
>>>>
>>>>
>>>> ANALYSIS:
>>>>
>>>> The crash is in
>>>>
>>>> V [libjvm.so+0x16fe226] Symbol::decrement_refcount()+0xe6
>>>> V [libjvm.so+0x1026e0b] JVM_FindLoadedClass+0x20b
>>>>
>>>> and the log file says "Symbol: 'java/lang/invoke/LambdaForm$BMH'
>>>> count -1".
>>>>
>>>> This seems to be a race condition between Symbol::decrement_refcount()
>>>> vs Symbol::set_permanent(). The former uses an atomic increment and
>>>> is called by
>>>> JVM_FindLoadedClass. The latter does a simple store of a short
>>>> value of -1, and is
>>>> called only by Method::log_touched() when -XX:+LogTouchedMethods is
>>>> enabled.
>>>>
>>>> Apparently we have a Symbol whose refcount started with a positive
>>>> value.
>>>> While one thread is calling Symbol::decrement_refcount() and a second
>>>> thread calls Symbol::set_permanent() at the same time, the
>>>> unexpected value -1
>>>> could be returned to the first thread.
>>>>
>>>> FIX:
>>>>
>>>> I changed Method::log_touched() to use Symbol::increment_refcount
>>>> instead.
>>>> I can no longer reproduce the crash after this change.
>>>>
>>>> Also, because the behavior of Symbol::set_permanent is not well
>>>> understood
>>>> and has shown to be racy, I removed this function altogether.
>>>
>>> So when will a Symbol now ever be "permanent"? Isn't all the code
>>> related to is_permanent() and PERM_REFCOUNT dead now? And the checks
>>> for _refcount >= 0 ?
>>
>> When we create a symbol, it can have a permanent refcount.
>
> Okay so now there are two types of "permanent" symbols - which seems
> confusing. Those set at creation time, and those previously set
> through set_permanent which now simply bump the refcount. And you can
> no longer identify the second set as being permanent.
After my patch, there will be only one type of permanent symbol -- when
you create a symbol, you know it will be alive forever (e.g., a symbol
requested by the null class loader).
I am not quite sure about this history of why this is necessary --
except for symbols in the CDS archive. Those are marked as permanent so
you won't change its refcount (because it's read only).
Perhaps it's for performance reasons? There are a lot of Symbols created
by the boot loader and are referenced by many classes. So if we don't
need to atomically modify their _refcounts then it pays off??
Or, maybe it's for memory saving -- allocating from arena has less
overhead than malloc??
Anyway, regardless of whether a symbol is permanent or not, the usual
rule is -- when you store a symbol (e.g., as an UTF8 into a
ConstantPool), you increment its refcount. When you release a symbol
(e.g., when said ConstantPool is deallocated), you decrement its refcount.
The only exception to this rule was Method::log_touched(), which used
set_permanent instead. It stores a symbol (in its hashtable) but never
releases it (the hashtable persists for the entire VM run time). So in
this case, it's perfectly OK to call increment_reference instead.
>
>>>
>>> The other way to avoid the race is to implement inc/dec using CAS
>>> instead of raw atomic add/sub.
>>
>> That seems expensive.
>
> If you wanted to keep the existing notion of permanent and how it is
> recorded, then it is a way to do that.
>
I think removing set_permanent will make the code cleaner and easier to
understand.
Thanks
- Ioi
> David
>
>> Coleen
>>>
>>> Thanks,
>>> David
>>>
>>>> Thanks
>>>> - Ioi
>>
More information about the hotspot-runtime-dev
mailing list