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

[David Holmes]

Hi Coleen,

Thanks for taking a look at this.

On 4/02/2021 11:41 pm, Coleen Phillimore wrote:
> On Thu, 4 Feb 2021 05:25:15 GMT, David Holmes <dholmes 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
> 
> I think using CATCH is preferable to passing THREAD with a comment, which is unenforceable in the code. Thanks for cleaning up excess TRAPS.

To be honest I never even considered CATCH because I thought it "caught" 
exceptions by simply clearing; but it also aborts! So what it really 
means is: "I don't expect any exceptions to escape this call so if I 
find one I'm going to abort"! Which is fine for debug code, but for 
product code it just adds redundant calls to HAS_PENDING_EXCEPTION. 
(CATCH should really be named something more indicative.)

> src/hotspot/share/classfile/javaClasses.hpp line 282:
> 
>> 280:   static void archive_basic_type_mirrors() NOT_CDS_JAVA_HEAP_RETURN;
>> 281:   static oop  archive_mirror(Klass* k) NOT_CDS_JAVA_HEAP_RETURN_(NULL);
>> 282:   static oop  process_archived_mirror(Klass* k, oop mirror, oop archived_mirro)
> 
> missing an 'r'.

Well spotted -thanks!

> 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.

Ignored. resolve_classes_for_subgraphs calls 
resolve_classes_for_subgraph_of, which does:

  if (HAS_PENDING_EXCEPTION) {
    CLEAR_PENDING_EXCEPTION;
  }

so no exceptions can percolate up.

> src/hotspot/share/memory/heapShared.cpp line 700:
> 
>> 698:     InstanceKlass* k = SystemDictionaryShared::find_builtin_class(klass_name);
>> 699:     assert(k != NULL && k->is_shared_boot_class(), "sanity");
>> 700:     resolve_classes_for_subgraph_of(k, THREAD /* exceptions are ignored */);
> 
> They're not ignored though, they're passed to the caller of this function.  Since this function passes TRAPS, the caller should have a CHECK/CATCH or handle the pending exception.

They are ignored as I just explained.

> 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 CATCH then.

All I was trying to capture by the comment was why this is not a CHECK 
usage. Using CATCH does not convey that.

CATCH really is the wrong word for the actual semantics here. :(

And I really don't want to add useless checks in product mode. :(

Thanks,
David

> -------------
> 
> Changes requested by coleenp (Reviewer).
> 
> PR: https://git.openjdk.java.net/jdk/pull/2397
>