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

[Ioi Lam]

BTW, SymbolTable::add seems to be just a alias of 
SymbolTable::new_symbols. Maybe we should just remove the add function 
and move the implementation to new_symbols? "add" is such a commonly 
used name so it's hard to look up anyway.

On 2/21/19 4:55 PM, Ioi Lam 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);
>
> 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);
>>    }
>