RFR(s): 8209139: globalCounter bootstrap issue

[Daniel D. Daugherty]

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.


Thumbs up!

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.

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
>