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

[Coleen Phillimore]

On 2/4/21 7:00 PM, David Holmes wrote:
> 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).

To me TRAPS means that you're going to return with an exception pending, 
and the caller has to deal with it.  That is, if you pass THREAD as the 
last parameter to a function with TRAPS, I hope that the next few lines 
in the caller will explicitly check for HAS_PENDING_EXCEPTION.  So this 
change, even with comments, was a bit jarring. I don't trust or believe 
comments in the long term.

I think separately we should make CATCH not fail in production.

I'm glad you're trying to change TRAPS to be JavaThread*, that's a 
worthy goal since non-java threads shouldn't be throwing Java exceptions 
(or we want to know if they are).  I started this change myself, but it 
got unwieldy.

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

I'm sensitive to that.
>
>>> 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?

The trouble with Thread* thread as a parameter is that one expects 
THREAD to be the current thread, and there are a few Thread* thread 
functions where that's not true of 'thread'.

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

Yes thanks.  See what you can do, I'm not going to insist on these 
changes.  Most are great, I was just jarred by these few THREAD 
parameters with comments.

Coleen

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