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

[David Holmes]

On 7/08/2018 5:33 PM, Robbin Ehn wrote:
> Hi, another 2c,
> 
> On 08/07/2018 07:29 AM, Kim Barrett wrote:
>>> On Aug 7, 2018, at 12:26 AM, David Holmes <david.holmes at oracle.com> 
>>> wrote:
>>>
>>> Just adding my 2c in one place as I'm not looking at this in detail ...
>>>
>>> On 7/08/2018 12:18 PM, Kim Barrett wrote:
>>>>> On Aug 6, 2018, at 12:39 PM, Gerard Ziemski 
>>>>> <gerard.ziemski at oracle.com> wrote:
>>>>>> src/hotspot/share/utilities/globalCounter.cpp
>>>>>> 64   // Handle bootstrap
>>>>>> 65   if (Threads::number_of_threads() == 0) {
>>>>>> 66     return;
>>>>>> 67   }
>>>>>>
>>>>>> What is this about?  number_of_threads only refers to Java threads,
>>>>>> but the iteration should deal with there being none, right?  Or does
>>>>>> this get called so early (because of SymbolTable changes) that even
>>>>>> that iteration setup doesn't work?
>>>>>>
>>>>>> If not that, then maybe we're calling before the VMThread exists, and
>>>>>> that's the real problem?  In which case a different test, differently
>>>>>> located, seems necessary.
> 
> We use "Threads::number_of_threads() == 0" as bootstrap check in the VM, 
> with stuff like:
>        // cache can grow so we have to be more careful
>        if (Threads::number_of_threads() == 0 ||
>            SafepointSynchronize::is_at_safepoint()) {
>          // we're single threaded or at a safepoint - no locking needed
> 
> I do not like that, but it's what we got. It is actually the only valid 
> use of number_of_threads, except as an historical number.
> 
> Personally I would really like number_of_threads go away.
> 
>>>>>
>>>>> We needed it when I was bringing up the code and I tried removing 
>>>>> it to see whether we still need it and it seems that we do (details 
>>>>> on the crash in the bug itself). Maybe Robbin can shed more light 
>>>>> here?
>>>> The problem here is that we're using the StringTable (and therefore
>>>> GlobalCounter) before the main thread has been registered.  That
>>>> registration happens in create_vm() (Threads::add(main_thread), line
>>>> 3729), which follows the call to init_globals() (where the symbol
>>>> table usage is occurring) in create_vm().
>>>> Maybe moving the Threads::add earlier would fix the problem?  But this
>>>> initialization code is very ordering sensitive, so I don't know if
>>>> that would work.
>>>
>>> I'd suggest not attempting that. It might appear to work only for the 
>>> bugs to become apparent much later.
> 
> I tried doing a proper fix by splitting up init methods in to parts and 
> do it in a correct sequence, problem was, as you say, that I can't 
> easily test it to be true. With small amount of time I deemed it not 
> possible to ship a proper fix.
> 
>>>
>>> I think this early VM usage needs to be pushed down to the SMR code 
>>> to handle correctly, rather than hacking an initialization check into 
>>> the GlobalCounter code.
> 
> Sorry I do not understand, please elaborate what you have in mind? 

Do the "number_of_threads==0" hack in the SMR code instead of the code 
that wants to use SMR.

Cheers,
David

> (second part, yes it's a hack)
> 
> Thanks, Robbin
> 
>>>
>>> David
>>> -----
>>
>> I agree with David on both counts.
>>