RFR: 8269466: Factor out the common code for initializing and starting internal VM JavaThreads [v2]

[David Holmes]

Hi Dan,

Thanks for the review.

On 1/07/2021 3:08 am, Daniel D.Daugherty wrote:
> On Wed, 30 Jun 2021 01:31:30 GMT, David Holmes <dholmes at openjdk.org> wrote:
> 
>>> Please see the JBS issue for more details, but basically we have 8 different kinds of internal VM JavaThreads (grouping the three types of CompilerThread together) that all basically duplicated the logic for initializing (preparing is the term we use for user-defined JavaThreads) and starting the new thread. This common code can be factored out into static helpers in JavaThread.
>>>
>>> This change does not look at the way the java.lang.Thread instance is created - that will be handled by a separate RFE.
>>>
>>> The semantics of the changes are not identical, but I don't believe there is any observable change in behaviour. The scope of holding the Threads_lock has been reduced, and we now create the JavaThread instances ("new XXXThread(...)") outside of the lock. As far as I can see nothing in the construction process needs to happen under the Threads_lock.
>>>
>>> A few of the threads use a static `_instance` field to hold a reference to the create JavaThread. This proved very difficult to handle, as logically the field would need to be updated in the middle of the new factored-out method: after setting all the fields but before releasing the newly started thread. I eventually realized that in all but one case those `_instance` fields are never used and so could be deleted. The one case remaining does not need to be set as I just described, but can be set after the thread has started, as the new thread does not examine it (arguably its existence is unnecessary).
>>>
>>> The trickiest changes related to the CompilerThreads, so they need particular scrutiny.
>>>
>>> Testing: tiers 1-3
>>>
>>> Thanks,
>>> David
>>
>> David Holmes has updated the pull request incrementally with one additional commit since the last revision:
>>
>>    Fixed copyright years in hpp files
> 
> Finished my re-review without the whitespace noise. I have to say
> that really reveals how nice this cleanup is! Thanks for doing it.
> 
> I only added some nits. Feel free to fix or not.
> 
> src/hotspot/share/compiler/compileBroker.cpp line 938:
> 
>> 936:     if (UseDynamicNumberOfCompilerThreads && type == compiler_t
>> 937:         && comp->num_compiler_threads() > 0) {
>> 938:       // the new thread is not known to Thread-SMR yet so we can just delete
> 
> nit: s/the/The/  and add a period to the end of the sentence.

Okay

> src/hotspot/share/prims/jvmtiEnv.cpp line 1335:
> 
>> 1333:   // JavaThread due to lack of memory.
>> 1334:   if (new_thread == NULL || new_thread->osthread() == NULL) {
>> 1335:     // the new thread is not known to Thread-SMR yet so we can just delete
> 
> nit: s/the/The/ and add a period to the end of the sentence.

Okay

> src/hotspot/share/runtime/monitorDeflationThread.cpp line 51:
> 
>> 49:                           CHECK);
>> 50:
>> 51:   MonitorDeflationThread* thread =  new MonitorDeflationThread(&monitor_deflation_thread_entry);
> 
> nit: s/=  new/= new/
> 
> (Not your typo, but please fix it while you're in here.)

Fixed

> src/hotspot/share/runtime/notificationThread.cpp line 63:
> 
>> 61:                           THREAD);
>> 62:
>> 63:    NotificationThread* thread =  new NotificationThread(&notification_thread_entry);
> 
> nit: s/= new/= new/
> 
> (Not your typo, but please fix it while you're in here.)

Fixed (also ServiceThread)

> src/hotspot/share/runtime/thread.cpp line 3911:
> 
>> 3909:   MutexLocker mu(current, Threads_lock);
>> 3910:
>> 3911:   // Initialize the fields of the thread_oop first
> 
> nit: please add a period to the end of the sentence.

Ok

> src/hotspot/share/runtime/thread.cpp line 3923:
> 
>> 3921:   java_lang_Thread::set_daemon(thread_oop());
>> 3922:
>> 3923:   // Now bind the thread_oop to the target JavaThread
> 
> nit: please add a period to the end of the sentence.

Ok.

I generally don't consider that single line comments need to be 
grammatical sentences.

> src/hotspot/share/services/attachListener.cpp line 490:
> 
>> 488:   JavaThread::vm_exit_on_thread_allocation_failure(thread);
>> 489:
>> 490:   JavaThread::start_internal_daemon(THREAD, thread, thread_oop, NoPriority);
> 
> I wonder if the lack of a specific priority for the attach listener is a
> contributing factor to some of the timeouts in attach that we observe.
> @plummercj and @sspitsyn - You might be interested here...

Given that we don't use thread native priorities by default except on 
Windows, that seems unlikely to be a source of problems. But who can say 
for sure ...

Thanks,
David

> -------------
> 
> Marked as reviewed by dcubed (Reviewer).
> 
> PR: https://git.openjdk.java.net/jdk/pull/4629
>