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

[Coleen Phillimore]

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

Some small change requests.

src/hotspot/share/jfr/recorder/service/jfrRecorderThread.cpp line 49:

> 47:   assert(proc != NULL, "invariant");
> 48: 
> 49:   JavaThread* new_thread = new JavaThread(proc);

if new_thread allocation fails, the operator new will call vm_exit_out_of_memory and exit. It won't return NULL.

src/hotspot/share/jfr/recorder/service/jfrRecorderThread.cpp line 54:

> 52:   // osthread was created for the JavaThread due to lack of memory.
> 53:   if (new_thread == NULL || new_thread->osthread() == NULL) {
> 54:     delete new_thread;

Since new_thread can't be null, this delete is going to be ok, but it would better not to check for null.

src/hotspot/share/prims/jvmtiEnv.cpp line 1334:

> 1332:   // At this point it may be possible that no osthread was created for the
> 1333:   // JavaThread due to lack of memory.
> 1334:   if (new_thread == NULL || new_thread->osthread() == NULL) {

Same here, unless there's some operator new overloading that doesn't use the nothrow operator new.

src/hotspot/share/runtime/notificationThread.cpp line 61:

> 59:                           vmSymbols::thread_void_signature(),
> 60:                           thread_oop,
> 61:                           THREAD);

This was all the code that I thought was unnecessarily duplicated.

src/hotspot/share/runtime/thread.cpp line 3935:

> 3933:   // in that case. However, since this must work and we do not allow
> 3934:   // exceptions anyway, check and abort if this fails.
> 3935:   if (thread == nullptr || thread->osthread() == nullptr) {

thread shouldn't be NULL here if you haven't used a nothrow version of new to allocate the thread.

-------------

Changes requested by coleenp (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/4629