RFR(s): 8209139: globalCounter bootstrap issue

[Robbin Ehn]

On 11/8/18 3:06 PM, Daniel D. Daugherty wrote:
>  > http://cr.openjdk.java.net/~rehn/8209139/v2/inc/webrev/
> 
> src/hotspot/share/runtime/threadSMR.cpp
>      No comments.
> 
> src/hotspot/share/runtime/threadSMR.hpp
>      No comments.
> 
> Thumbs up.

Thanks Dan!

/Robbin

> 
> Dan
> 
> 
> On 11/8/18 4:10 AM, Robbin Ehn wrote:
>> Hi Dan,
>>
>> Far down ->
>>
>> On 11/7/18 8:38 PM, Daniel D. Daugherty wrote:
>>> On 11/7/18 3:56 AM, 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/
>>>
>>> src/hotspot/share/runtime/threadSMR.hpp
>>>      L150:   static bool is_bootstrap_list(ThreadsList* list);
>>>          This line should be after:
>>>
>>>          L145:   static bool is_a_protected_JavaThread_with_lock(JavaThread 
>>> *thread);
>>>
>>>          and you don't need the extra blank line.
>>>
>>> src/hotspot/share/runtime/threadSMR.cpp
>>>      L77: // The bootstrap empty list
>>>          Please change the comment to:
>>>
>>>            // The bootstrap list is empty and cannot be freed.
>>>
>>>      L80: // The ThreadsList
>>>          Please change the comment to:
>>>
>>>            // This is the VM's current "threads list" and it contains all of
>>>            // the JavaThreads the VM considers to be alive at this moment in
>>>            // time. The other ThreadsList objects in the VM contain past
>>>            // snapshots of the "threads list". _java_thread_list is initially
>>>            // set to _bootstrap_list so that we can detect when we have a very
>>>            // early use of a ThreadsListHandle.
>>>
>>>      L170: bool ThreadsSMRSupport::is_bootstrap_list(ThreadsList* list) {
>>>      L171:   return list == &_bootstrap_list;
>>>      L172: }
>>>          Consider moving this function definition to before:
>>>
>>>              L146: inline void 
>>> ThreadsSMRSupport::update_deleted_thread_time_max(uint new_value) {
>>>
>>>          Also maybe it should be inline?
>>>
>>>      L524:   // Handle bootstrap
>>>      L525:   if (ThreadsSMRSupport::is_bootstrap_list(_list)) {
>>>          Please move the comment to after L525 and maybe change it to:
>>>
>>>                // We are early in VM bootstrapping so nothing to do here.
>>>
>>>      L782:     // The bootstrap list is on BSS and cannot be freed.
>>>      L783:     // The list is empty so it do not need to be scanned.
>>>          Please change the comment lines to:
>>>
>>>                // The bootstrap list cannot be freed and is empty so
>>>                // it does not need to be scanned. Nothing to do here.
>>>
>>>          If you want to mention "BSS" somewhere, then it would be better
>>>          above on L77 (the comment for the _bootstrap_list definition.
>>>
>>>      L784:     log_debug(thread, smr)("tid=" UINTX_FORMAT ": 
>>> ThreadsSMRSupport::free_list: bootstrap ThreadsList=" INTPTR_FORMAT " no 
>>> longer in use.", os::current_thread_id(), p2i(threads));
>>>          Typo: "no longer in..." -> "is no longer in..."
>>>
>>>
>>> src/hotspot/share/utilities/globalCounter.cpp
>>>      No comments.
>>>
>>
>> Updated accordingly:
>> Inc:
>> http://cr.openjdk.java.net/~rehn/8209139/v2/inc/webrev/
>> Full:
>> http://cr.openjdk.java.net/~rehn/8209139/v2/full/webrev/
>>
>>>
>>> Thumbs up!
>>
>> Thanks Dan!
>>
>>>
>>> I prefer this solution to the "Threads::number_of_threads() == 0"
>>> check. Your fix is done internally to ThreadsSMRSupport and it is
>>> clear (internal to that class) that we're solving a bootstrapping
>>> issue.
>>>
>>> The "Threads::number_of_threads() == 0" check is relying on a side
>>> effect that having no JavaThreads means that we are early in VM
>>> bring up. While it is true, it is not the most clear of mechanisms.
>>
>> Yes, this was my thinking also.
>>
>> /Robbin
>>
>>>
>>> Dan
>>>
>>>
>>>>
>>>> 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
>>>>
>>>
>