RFR (S) [Graal] runtime/CommandLine/PrintTouchedMethods.java crashes with assertion "reference count underflow for symbol"

coleen.phillimore at oracle.com coleen.phillimore at oracle.com
Mon Mar 26 13:06:55 UTC 2018



On 3/26/18 1:18 AM, Ioi Lam wrote:
>
>
> 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??

It was for performance reasons, to avoid atomic increments.   I don't 
know if it actually helps performance though or how to measure that.  
Just the suspicion that more atomic operations may have bad effects.
>
> Or, maybe it's for memory saving -- allocating from arena has less 
> overhead than malloc??

Yes, that too.   Also you don't need to NMT symbols in the arena, just 
count the whole arena.

>
> 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 rule is that when you *lookup* a symbol, you increment the refcount 
(because you're using it).   Hopefully the comments around TempNewSymbol 
make sense.

I did have a prototype that used handles that had incremented and 
decremented Symbol refcounts and there were a ton more increments and 
decrements.  For the most part, Symbols come in through the constant 
pool and out through class unloading so the design takes advantage of 
that.  Plus, we were happy to finally use direct pointers to Symbols 
because it simplified the programming model (except the case of 
TempNewSymbol, so this has been debated).

>
> 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.
>

It's good to not have this exception, because of bugs like this.

Thanks,
Coleen
>
>>
>>>>
>>>> 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