RFR(s): 8209139: globalCounter bootstrap issue

[Daniel D. Daugherty]

 > 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.

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
>>>
>>