RFR(s): 8209139: globalCounter bootstrap issue

[Robbin Ehn]

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