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

[David Holmes]

Hi Coleen,

Thanks for the review.

On 1/07/2021 4:56 am, Coleen Phillimore 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
> 
> 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.

Right.

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

I've deleted the NULL check, but you can apply delete to NULL in any case.

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

Fixed.

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

The java.lang.Thread creation code will be fixed by JDK-8269652.
> 
> 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.

Fixed. I also updated the comments there.

Thanks,
David
-----

> -------------
> 
> Changes requested by coleenp (Reviewer).
> 
> PR: https://git.openjdk.java.net/jdk/pull/4629
>