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

[Ioi Lam]

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