RFR (L) JDK-8195100: Use a low latency hashtable for SymbolTable

[coleen.phillimore at oracle.com]

On 8/2/18 12:26 PM, Ioi Lam wrote:
>
>
> On 8/2/18 8:44 AM, Gerard Ziemski wrote:
>>> On Aug 2, 2018, at 10:40 AM, Ioi Lam <ioi.lam at oracle.com> wrote:
>>>
>>>
>>>
>>> On 8/2/18 8:26 AM, Gerard Ziemski wrote:
>>>>> symbolTable.cpp:
>>>>>
>>>>>   342   // Otherwise, add the symbol to table. Copy to a C string 
>>>>> first.
>>>>>   343   char stack_buf[ON_STACK_BUFFER_LENGTH];
>>>>>   344   ResourceMark rm(THREAD);
>>>>>   345   if (len <= ON_STACK_BUFFER_LENGTH) {
>>>>>   346     buffer = stack_buf;
>>>>>   347   } else {
>>>>>   348     buffer = NEW_RESOURCE_ARRAY_IN_THREAD(THREAD, char, len);
>>>>>   349   }
>>>>>   350   for (int i=0; i<len; i++) {
>>>>>   351     buffer[i] = name[i];
>>>>>   352   }
>>>>>   353   // Make sure there is no safepoint in the code above since 
>>>>> name can't move.
>>>>>   354   // We can't include the code in NoSafepointVerifier 
>>>>> because of the
>>>>>   355   // ResourceMark.
>>>>>
>>>>> The above is dead code and should be removed.
>>>> That’s not dead code, but in fact you did find an issue here - we 
>>>> should be using the “buffer”, not “name” when we add to the table. 
>>>> Thanks!
>>>>
>>>>
>>> Why do you need to make a copy here? buffer and name seems to have 
>>> the exact same contents.
>> I didn’t write that code, but I assume the name comes in some temp 
>> memory that can go away at any time, so we need a solid copy.
>>
>>
>> cheers
>>
> This function is called only from two places. In both cases, the input 
> Symbol won't go away
>
> Symbol* SignatureStream::as_symbol(TRAPS) {
>   // Create a symbol from for string _begin _end
>   int begin = _begin;
>   int end   = _end;
>
>   if (   _signature->byte_at(_begin) == 'L'
>       && _signature->byte_at(_end-1) == ';') {
>     begin++;
>     end--;
>   }
>
>   // Save names for cleaning up reference count at the end of
>   // SignatureStream scope.
>   Symbol* name = SymbolTable::new_symbol(_signature, begin, end, 
> CHECK_NULL);
>   _names->push(name);  // save new symbol for decrementing later
>   return name;
> }
>
> Symbol* ClassVerifier::create_temporary_symbol(const Symbol *s, int 
> begin,
>                                                int end, TRAPS) {
>   Symbol* sym = SymbolTable::new_symbol(s, begin, end, CHECK_NULL);
>   _symbols->push(sym);
>   return sym;
> }
>
> Also, the "name" was used just a few lines above in the lookup, so if 
> this was ever a issue, that lookup would fail as well.
>
> Mercurial history shows that this function has not had any meaningful 
> change since it was imported from SCCS in 2007, so I think it's just a 
> case of bit rot and the dead code can be safely removed.

I don't like the concept of one Symbol pointing into a substring of 
another Symbol.  If the latter goes away, then the former will point to 
deallocated memory.   Can we examine this outside this change to see if 
the lifetime of the substring Symbol is completely contained in the 
superstring Symbol?

thanks,
Coleen
>
> Thanks
> - Ioi