Re: 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 <dhol...@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