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

[Mikael Vidstedt]

> 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)?

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);
>>>>    }
>