RFR(T): 8219554: Redundant lookup_common in SymbolTable::add

[Mikael Vidstedt]

Ooooh, *that* kind of shared, that makes sense. Thanks for the explanation!

Cheers,
Mikael

> On Feb 21, 2019, at 5:40 PM, Ioi Lam <ioi.lam at oracle.com> wrote:
> 
> 
> 
> On 2/21/19 5:27 PM, Mikael Vidstedt wrote:
>> 
>>> On Feb 21, 2019, at 5:17 PM, Ioi Lam <ioi.lam at oracle.com> wrote:
>>> 
>>> 
>>> 
>>> On 2/21/19 5:12 PM, Mikael Vidstedt wrote:
>>>>> On Feb 21, 2019, at 4:55 PM, Ioi Lam <ioi.lam at oracle.com> wrote:
>>>>> 
>>>>> Hi Claes,
>>>>> 
>>>>> Before your change, there's no API requirement that you must have called SymbolTable::lookup_common before calling SymbolTable::add, even though that is the behavior of the classFileParser.
>>>>> 
>>>>> So, in order to codify this new requirement, I think you should you should add an assert:
>>>>> 
>>>>> 
>>>>> +   assert(lookup_shared(name, len, hash) == NULL, "must have checked already");
>>>>>     Symbol* sym = SymbolTable::the_table()->do_add_if_needed(name, len, hash, c_heap, CHECK);
>>>> I’m sure this is a stupid(?) question, but I’m curious: What ensures the symbol isn’t removed/unloaded from the table between the earlier lookup and the SymbolTable::add call?
>>> A symbol could be removed from the "dynamic" table (not sure if that requires a safe point ...), but it would be added back again by do_add_if_needed.
>>> 
>>> Symbols in the shared table will never be removed, that's why I am asserting with only lookup_shared().
>> What prevents the symbol from being added to the shared table “just before” the assert (presumably by another thread)?
> 
> The shared table is part of the CDS archive. It is created when the CDS archive was dumped (-Xshare:dump) and cannot be modified at run time :-)
> 
> Thanks
> - Ioi
>> Cheers,
>> Mikael
>> 
>>> Thanks
>>> - Ioi
>>> 
>>>> Cheers,
>>>> Mikael
>>>> 
>>>>> Thanks
>>>>> - Ioi
>>>>> 
>>>>> 
>>>>> On 2/21/19 2:06 PM, Claes Redestad wrote:
>>>>>> Hi,
>>>>>> 
>>>>>> the arguments passed to SymbolTable::add has already been through a call
>>>>>> to lookup_common, so a small optimization is to remove the redundant
>>>>>> call inside the add method:
>>>>>> 
>>>>>> - the lookup_common already done in the class file parser ensures none
>>>>>> of the symbols lives in the shared archive.
>>>>>> - do_add_if_needed will do a quick get into the dynamic hash table anyhow.
>>>>>> 
>>>>>> Bug:   https://bugs.openjdk.java.net/browse/JDK-8219554
>>>>>> Patch: [1]
>>>>>> 
>>>>>> Testing: tier1+2
>>>>>> 
>>>>>> Thanks!
>>>>>> 
>>>>>> /Claes
>>>>>> 
>>>>>> [1]
>>>>>> hg diff -r cc9359f8c563 src/hotspot/share/classfile/symbolTable.cpp
>>>>>> --- a/src/hotspot/share/classfile/symbolTable.cpp    Thu Feb 21 14:16:44 2019 +0100
>>>>>> +++ b/src/hotspot/share/classfile/symbolTable.cpp    Thu Feb 21 23:12:29 2019 +0100
>>>>>> @@ -444,10 +444,7 @@
>>>>>>      const char *name = names[i];
>>>>>>      int len = lengths[i];
>>>>>>      unsigned int hash = hashValues[i];
>>>>>> -    Symbol* sym = SymbolTable::the_table()->lookup_common(name, len, hash);
>>>>>> -    if (sym == NULL) {
>>>>>> -      sym = SymbolTable::the_table()->do_add_if_needed(name, len, hash, c_heap, CHECK);
>>>>>> -    }
>>>>>> +    Symbol* sym = SymbolTable::the_table()->do_add_if_needed(name, len, hash, c_heap, CHECK);
>>>>>>      assert(sym->refcount() != 0, "lookup should have incremented the count");
>>>>>>      cp->symbol_at_put(cp_indices[i], sym);
>>>>>>    }