RFR: 8261127: Cleanup THREAD/TRAPS/CHECK usage in CDS code

[David Holmes]

On 5/02/2021 9:42 am, Coleen Phillimore wrote:
> On Thu, 4 Feb 2021 13:35:11 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:
> 
>>> While examining changes related to the TRAPS macro I noticed that a number of CDS related methods were passed a thread parameter that ended up never being used. Fixing this percolates up the call chain resulting in the ability to remove the parameter from other calls. In some places the thread is needed and has to be manifested via Thread::current(), but these are few and non-critical, and the cleanup of the API (including the benefit to the TRAPS work - see JDK-8252685) makes this worthwhile.
>>>
>>> I also changed `Thread* THREAD` to TRAPS where it relates to exception processing (even if CDS diverts to an abort path).
>>>
>>> Some uses of CHECK were removed that were only passing THREAD and the called code never triggers any exceptions.
>>>
>>> Added commentary where it is not clear why we don't use CHECK.
>>>
>>> Testing: local build and CI tiers 1-3
>>>
>>> Thanks,
>>> David
>>
>> src/hotspot/share/memory/heapShared.cpp line 690:
>>
>>> 688:   resolve_classes_for_subgraphs(fmg_open_archive_subgraph_entry_fields,
>>> 689:                                 num_fmg_open_archive_subgraph_entry_fields,
>>> 690:                                 THREAD /* exceptions are ignored */);
>>
>> Are exceptions ignored or unexpected? This pattern doesn't exist anywhere else as far as I know, otherwise maybe we should have a CLEAR macro in addition to CHECK/CATCH that clears pending exceptions? I don't really like that there may be exceptions lurking through multiple calls.
> 
> Ok, so I'm reading this in context. This still looks unusual and possibly problematic without deeper inspection.  It seems like removing TRAPS here also and the callees, and adding this before the functions that clear pending exceptions would be better.
> JavaThread* THREAD = JavaThread::current();

I guess it depends on what we each think TRAPS should indicate. I like 
to consider it as a flag that there is a code path that will execute 
Java code and/or potentially raise exceptions. If you think it means 
specifically that this method can return with an exception pending, then 
yes using TRAPS on these methods is not appropriate. I like the more 
broader interpretation when reasoning about whether a call path can ever 
be taken by a non-JavaThread - TRAPS says "only JavaThreads should call 
this" (hence my work to change the definition of TRAPS to JavaThread* 
THREAD).

Passing through "Thread* thread" is still reasonable from a performance 
perspective so we don't unnecessarily manifest Thread::current() too 
many times.

>> src/hotspot/share/memory/heapShared.cpp line 1275:
>>
>>> 1273:   init_subgraph_entry_fields(closed_archive_subgraph_entry_fields,
>>> 1274:                              num_closed_archive_subgraph_entry_fields,
>>> 1275:                              THREAD /* aborts on exception */);
>>
>> So these should really be debug version of CATCH then.
> 
> This code makes me wonder if Thread* THREAD doesn't make the intent of this code clearer.

"Thread* thread" - perhaps. To me "Thread* THREAD" is a sign that the 
code is confused - it wants a variable called THREAD which is only 
needed to interact with exception macros, but it's not a TRAPS method?

I'll rework the changes in this area to avoid using TRAPS and THREAD.

Thanks,
David

> -------------
> 
> PR: https://git.openjdk.java.net/jdk/pull/2397
>