RFR(s): 8209139: globalCounter bootstrap issue

[David Holmes]

Hi Robbin,

I wrote two responses to this last night before deciding to "sleep on 
it". :)

Thanks for the extended explanations - I understand what you are doing 
more clearly now. I see Dan has made a few minor comments and I don't 
have anything additional to add. So Reviewed.

My personal preference is to treat the early initialization in the 
initial thread as a special case - using "hacks" like the 
"Threads::number_of_threads() == 0" test. Allowing any thread to be able 
to access a ThreadsList (and by extension GlobalCounter) before it has 
properly initialized may turn out to be useful one day, but it may also 
mask bugs, as other threads should be following strict initialization 
protocols that don't permit non-trivial actions prior to being fully 
initialized.

Thanks,
David

On 7/11/2018 9:53 PM, Robbin Ehn wrote:
> Hi David,
> 
> On 2018-11-07 12:10, David Holmes wrote:
>> Hi Robbin,
>>
>> On 7/11/2018 8:01 PM, Robbin Ehn wrote:
>>> Hi David,
>>>
>>> On 2018-11-07 10:40, David Holmes wrote:
>>>> Hi Robbin,
>>>>
>>>> Can't you just use an _initialized flag ?? 
>>>
>>> I can, but I need to changes many places since the ThreadsList is not 
>>> expected
>>> to be null. And I do not see how checking that flag instead checking 
>>> if the list
>>> is the bootstrap would be better.
>>
>>  From the bug there seemed to be only once place that needed this 
>> check. Where are all these other places?
> 
> All potential cases that uses a ThreadsList before init.
> These are very had to track down, so I don't know of any others.
> 
> I'm adding that exception, so you may have a ThreadsList if you are 
> _not_ part
> of the SMR protocol if that list is the bootstrap list.
> 
>>
>>> Or statically initialize
>>>> _java_thread_list to a zero-length list? 
>>>
>>> It's statically initialized to _bootstrap_list. Since I'm going to 
>>> need to check
>>> for a bootstrap list in a non-SMR-thread I can't follow the pointer 
>>> because we
>>> can crash. So I need to know which address to check for, this is the
>>> _bootstrap_list. And since it's static I can't free it.
>>
>> Not following. Can't you initialize _java_thread_list directly to a 
>> zero-length list and that will "just work" because you'll iterate zero 
>> items? There will only ever be one zero-length threads-list right?
> 
> The iteration is not the problem, it is the protocol verification.
> Right know you may only have a ThreadsList if you are part of SMR-protocol,
> which means the VM must have done it's init. With the above exception to 
> the
> protocol the validation works for all cases we have today and all cases 
> we might
> have in the future that I can think of.
> 
> With this you can use ThreadsListHandle before init and you don't need 
> to pay
> attention to if your code could ever possibly be run before init or in 
> the init
> code. So I'm trying to create robustness because as SymbolTable issue 
> could pass t1-5 without that code path being executed.
> 
> That why I tested with a ThreadsListHandle in the init code to make sure 
> we actually do create at least one there.
> 
> /Robbin
> 
>>
>> Thanks,
>> David
>> -----
>>
>>>> This seems overly elaborate.
>>>
>>> If we get a SMR bug doing it different could give us wired crashes 
>>> instead of
>>> clean assert failures. That is why you think this is 'overly 
>>> elaborate' I guess.
>>>
>>>>
>>>> I'm not even concerned about the existing "hack" - checking for 
>>>> "Threads::number_of_threads() == 0" has always been a simple way to 
>>>> see if you are very early in the init sequence.
>>>
>>> Well it only solves the problem for the GlobalCounter.
>>> And in theory we could have added non-JavaThreads to the RCU 
>>> thread-set they
>>> should be checked.
>>>
>>> Thanks, Robbin
>>>
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>> On 7/11/2018 6:56 PM, Robbin Ehn wrote:
>>>>> Hi all, please review.
>>>>>
>>>>> When changing the hashtable for SymbolTable we did a hack to handle 
>>>>> the case
>>>>> when symbols gets inserted before the VM is proper initialized, by 
>>>>> doing nothing
>>>>> in the GlobalCounter if there is no threads. The problem is that 
>>>>> the thread
>>>>> creating the VM is not yet added to the VM (non-SMR-thread) and thus
>>>>> ThreadsListHandle do not yet work for that thread. Trying to move 
>>>>> the addition
>>>>> of the creating thread earlier in the bootstrap is troublesome.
>>>>>
>>>>> This removes that hack and do a more generic solution for 
>>>>> ThreadsList instead.
>>>>> To make it safe to verify that we are talking about the bootstrap 
>>>>> ThreadsList from a non-SMR-thread we cannot follow that pointer. 
>>>>> Using null was intrusive to the code I therefore choose to do a 
>>>>> static bootstrap ThreadsList.
>>>>>
>>>>> CR: https://bugs.openjdk.java.net/browse/JDK-8209139
>>>>> Webrev: http://cr.openjdk.java.net/~rehn/8209139/webrev/
>>>>>
>>>>> Tested with use of ThreadsListHandle and GlobalCounter in the 
>>>>> problematic init method, passed t1-2 with use of ThreadsListHandle 
>>>>> in that init method.
>>>>>
>>>>> Thanks, Robbin