On Sat, 8 May 2021 13:06:01 GMT, Coleen Phillimore <[email protected]> wrote:
>> Our code is littered with API's that take, or manifest, a Thread* and then
>> assert/guarantee that it must be a JavaThread, rather than
>> taking/manifesting a JavaThread in the first place. The main reason for this
>> is that the TRAPS macro, used in relation to exception generation and
>> processing, is declared as "Thread* THREAD". In practice only JavaThreads
>> that can execute Java code can generate arbitrary exceptions, because it
>> requires executing Java code. In other places we can get away with other
>> kinds of threads "throwing" exceptions because they are only pre-allocated
>> instances that require no Java code execution (e.g. OOME), or when executed
>> by a non-JavaThread the code actually by-passes the logic would throw an
>> exception. Such code also eventually clears the exception and reports the
>> OOM by some other means. We have been progressively untangling these code
>> paths and modifying TRAPS/CHECK usage so that only JavaThreads will call
>> TRAPS methods and throw exceptions. Having done t
hat pre-work we are now ready to convert TRAPS to be "JavaThread* THREAD" and
that is what this change does. The resulting changes are largely mechanical:
>>
>> - declarations of "Thread* THREAD" become "JavaThread* THREAD" (where THREAD
>> is needed for exceptions, otherwise THREAD is renamed to current)
>> - methods that took a Thread* parameter that was always THREAD, now take a
>> JavaThread* param
>> - Manifestations of Thread::current() become JavaThread::current()
>> - THREAD->as_Java_thread() becomes just THREAD
>> - THREAD->is_Java_thread() is reworked so that THREAD is "current"
>>
>> There are still places where a CompilerThread (which is a JavaThread but may
>> not be able to execute Java code) calls a TRAPS function (primarily where
>> OOME is possible). Fixing that would be a future RFE and may not be possible
>> without reworking a lot of the allocation code paths.
>>
>> Testing:
>> - tiers 1-8 on Linux-x64
>> - all builds in tiers 1-4
>> - tiers 1-3 on all platforms
>>
>> Thanks,
>> David
>
> src/hotspot/share/cds/archiveBuilder.cpp line 903:
>
>> 901:
>> 902: static void write_klass(Klass* k, address runtime_dest, const char*
>> type_name, int bytes, Thread* THREAD) {
>> 903: ResourceMark rm(THREAD);
>
> This change seems like it could be an independent check in because all you
> did was change the name of the parameter here, ie doesn't depend on on THREAD
> being a JavaThread.
Under these changes THREAD is always a JavaThread so I either need to change
the type here (which would be wrong) or else change the name, because it
doesn't actually relate to TRAPS/exceptions. As this change touches a lot of
files keeping it merged it time consuming so I'd prefer not to have to split
out anything further at this stage. Thanks.
> src/hotspot/share/cds/dynamicArchive.cpp line 172:
>
>> 170: _header = mapinfo->dynamic_header();
>> 171:
>> 172: Thread* THREAD = Thread::current();
>
> Unused? Looks like you could also remove these in a trivial pre-commit.
Yes unused. Again trying to avoid further delays. Also I think this code just
went through an undo and will soon be redone, in which case if it gets
integrated first I'll try to get this cleaned up. Thanks.
> src/hotspot/share/runtime/sharedRuntime.cpp line 1197:
>
>> 1195:
>> 1196: methodHandle SharedRuntime::find_callee_method(TRAPS) {
>> 1197: JavaThread* current = THREAD;
>
> I think these look strange, especially the ones
> JavaThread* thread = THREAD;
> but they can be fixed later to just use THREAD when THREAD is available. And
> it's not really important.
They are the inverse of the
JavaThread* THREAD = current; // For exception macro use.
due to the general trend to not use THREAD for non-exception related code
except in some trivial cases.
-------------
PR: https://git.openjdk.java.net/jdk/pull/3877
