On Thu, 1 Jul 2021 04:18:28 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 three additional
> commits since the last revision:
>
> - Rename vm_exit_on_thread_allocation_failure to vm_exit_on_osthread_failure
> - Adjust comment
> - Comments from PR review:
> - remove unnecessary new_thread NULL checks
> - adjust some comments
> - fix whitespace
Few comments for compileBroker.cpp changes.
src/hotspot/share/compiler/compileBroker.cpp line 909:
> 907: // JavaThread due to lack of resources. We will handle that failure
> below.
> 908:
> 909: if (new_thread->osthread() != NULL) {
I think you do need to check `new_thread != NULL` here to please Parfait for
`default:` case.
src/hotspot/share/compiler/compileBroker.cpp line 939:
> 937: && comp->num_compiler_threads() > 0) {
> 938: // The new thread is not known to Thread-SMR yet so we can just
> delete.
> 939: delete new_thread;
Need `new_thread != NULL` check if you do as I suggested in previous comment.
-------------
PR: https://git.openjdk.java.net/jdk/pull/4629
