On Wed, 30 Jun 2021 01:31:30 GMT, David Holmes <[email protected]> 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
