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

Ioi Lam ioi.lam at oracle.com
Thu Aug 2 17:25:06 UTC 2018



On 8/2/18 10:08 AM, coleen.phillimore at oracle.com wrote:
>
>
> 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?
>

The Symbol that's eventually created stores the characters inside its 
body. It doesn't not "point to another Symbol". The buffer is used only 
during creation time.

There are several forms of SymbolTable::new_symbol() calls. Some of them 
pass in an arbitrary "char*" anyway ... so there's no need to treat 
SymbolTable::new_symbol(Symbol*, ...) specially.

Thanks
- Ioi

> thanks,
> Coleen
>>
>> Thanks
>> - Ioi
>



More information about the hotspot-runtime-dev mailing list