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

[Ioi Lam]

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

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