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

classic Classic list List threaded Threaded
21 messages Options
12
Reply | Threaded
Open this post in threaded view
|

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

David Holmes-2
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

-------------

Commit messages:
 - 8261127: Cleanup THREAD/TRAPS/CHECK usage in CDS code

Changes: https://git.openjdk.java.net/jdk/pull/2397/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2397&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8261127
  Stats: 86 lines in 8 files changed: 0 ins; 16 del; 70 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2397.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2397/head:pull/2397

PR: https://git.openjdk.java.net/jdk/pull/2397
Reply | Threaded
Open this post in threaded view
|

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

Ioi Lam-2
On Thu, 4 Feb 2021 05:25:15 GMT, David Holmes <[hidden email]> 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

These changes look good to me.

You probably have found out that that some of the functions you changed (e.g.,  HeapShared::archive_reachable_objects_from) are executed inside the VMThread, which is not a JavaThread. So their current use of TRAPS/CHECK will not be compatible with your upcoming changes in JDK-8252685. The PR will fix these cases (but I am not sure if there are others).

For the functions that you converted to use TRAPS, these are OK as they are called only inside a Java thread, before the CDS code switches to VM_PopulateDumpSharedSpace::doit().

***

Just a side note ... the CDS code has more misuse of THREAD. But those can be done in a separate RFE, and they probably won't affect JDK-8252685.

E.g., MetaspaceShared::preload_and_dump should probably be changed to:

void MetaspaceShared::preload_and_dump(TRAPS) {
    MetaspaceShared::preload_and_dump_impl(THREAD);
    if (HAS_PENDING_EXCEPTION) {
       log_error(cds)("CDS dumping has failed");
       vm_exit(1);
    }
}

void MetaspaceShared::preload_and_dump_impl(TRAPS) {
  .... the old MetaspaceShared::preload_and_dump()
  .... change all THREAD to CHECK;
}

-------------

Marked as reviewed by iklam (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2397
Reply | Threaded
Open this post in threaded view
|

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

Coleen Phillimore-3
In reply to this post by David Holmes-2
On Thu, 4 Feb 2021 05:25:15 GMT, David Holmes <[hidden email]> 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.

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

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.

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.

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.

-------------

Changes requested by coleenp (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2397
Reply | Threaded
Open this post in threaded view
|

Re: 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 <[hidden email]> 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
>
Reply | Threaded
Open this post in threaded view
|

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

David Holmes
In reply to this post by Ioi Lam-2
Hi Ioi,

Thanks for taking a look.

On 4/02/2021 4:09 pm, Ioi Lam wrote:

> On Thu, 4 Feb 2021 05:25:15 GMT, David Holmes <[hidden email]> 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
>
> These changes look good to me.
>
> You probably have found out that that some of the functions you changed (e.g.,  HeapShared::archive_reachable_objects_from) are executed inside the VMThread, which is not a JavaThread. So their current use of TRAPS/CHECK will not be compatible with your upcoming changes in JDK-8252685. The PR will fix these cases (but I am not sure if there are others).

Having TRAPS on such functions is plain wrong - if there were actually
exceptions involved then the VMThread can't/shouldn't be executing the
code. Yes I may find other cases where this problem exists.

> For the functions that you converted to use TRAPS, these are OK as they are called only inside a Java thread, before the CDS code switches to VM_PopulateDumpSharedSpace::doit().

Given their use of exceptions it is imperative they are only executed by
JavaThreads. But thanks for confirming.

> ***
>
> Just a side note ... the CDS code has more misuse of THREAD. But those can be done in a separate RFE, and they probably won't affect JDK-8252685.
>
> E.g., MetaspaceShared::preload_and_dump should probably be changed to:
>
> void MetaspaceShared::preload_and_dump(TRAPS) {
>      MetaspaceShared::preload_and_dump_impl(THREAD);
>      if (HAS_PENDING_EXCEPTION) {
>         log_error(cds)("CDS dumping has failed");
>         vm_exit(1);
>      }
> }
>
> void MetaspaceShared::preload_and_dump_impl(TRAPS) {
>    .... the old MetaspaceShared::preload_and_dump()
>    .... change all THREAD to CHECK;
> }

Right - I didn't try to do a full audit, I simply found a particular
case (that seemed big enough to warrant the effort) and pulled on that
thread as far as I could. There may be other followup cleanups as I find
more cases. :)

Thanks,
David

> -------------
>
> Marked as reviewed by iklam (Reviewer).
>
> PR: https://git.openjdk.java.net/jdk/pull/2397
>
Reply | Threaded
Open this post in threaded view
|

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

Coleen Phillimore-3
In reply to this post by Coleen Phillimore-3
On Thu, 4 Feb 2021 13:36:19 GMT, Coleen Phillimore <[hidden email]> 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 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.

The are passed to the callers of this function as THREAD->_pending_exception will have an something in it.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2397
Reply | Threaded
Open this post in threaded view
|

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

David Holmes
On 5/02/2021 9:19 am, Coleen Phillimore wrote:

> On Thu, 4 Feb 2021 13:36:19 GMT, Coleen Phillimore <[hidden email]> 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 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.
>
> The are passed to the callers of this function as THREAD->_pending_exception will have an something in it.

The code does:

void HeapShared::resolve_classes_for_subgraph_of(Klass* k, TRAPS) {
  const ArchivedKlassSubGraphInfoRecord* record =
resolve_or_init_classes_for_subgraph_of(k, /*do_init=*/false, THREAD);
  if (HAS_PENDING_EXCEPTION) {
    CLEAR_PENDING_EXCEPTION;
  }
  if (record == NULL) {
    clear_archived_roots_of(k);
  }
}

so we clear any pending exception that might arise.

David
-----

> -------------
>
> PR: https://git.openjdk.java.net/jdk/pull/2397
>
Reply | Threaded
Open this post in threaded view
|

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

Coleen Phillimore-3
In reply to this post by David Holmes-2
On Thu, 4 Feb 2021 05:25:15 GMT, David Holmes <[hidden email]> 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

Changes requested by coleenp (Reviewer).

-------------

PR: https://git.openjdk.java.net/jdk/pull/2397
Reply | Threaded
Open this post in threaded view
|

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

Coleen Phillimore-3
In reply to this post by Coleen Phillimore-3
On Thu, 4 Feb 2021 13:35:11 GMT, Coleen Phillimore <[hidden email]> 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();

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

-------------

PR: https://git.openjdk.java.net/jdk/pull/2397
Reply | Threaded
Open this post in threaded view
|

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

Coleen Phillimore-3
In reply to this post by Coleen Phillimore-3
On Thu, 4 Feb 2021 23:17:20 GMT, Coleen Phillimore <[hidden email]> wrote:

>> 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.
>
> These _are_ passed to the callers of this function as THREAD->_pending_exception will have an something in it.

Exceptions aren't ignored, they are cleared by the callees, so you don't expect any exceptions, which is CATCH.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2397
Reply | Threaded
Open this post in threaded view
|

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

David Holmes
In reply to this post by Coleen Phillimore-3
On 5/02/2021 9:42 am, Coleen Phillimore wrote:

> On Thu, 4 Feb 2021 13:35:11 GMT, Coleen Phillimore <[hidden email]> 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
>
Reply | Threaded
Open this post in threaded view
|

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

Coleen Phillimore-2


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
>> <[hidden email]> 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
>>

Reply | Threaded
Open this post in threaded view
|

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

Ioi Lam-2
In reply to this post by Coleen Phillimore-3
On Thu, 4 Feb 2021 23:39:51 GMT, Coleen Phillimore <[hidden email]> 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
>
> Changes requested by coleenp (Reviewer).

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

I agree we should change the check to an assert.

It's not clear what `CATCH` means. Luckily we have very few use of it now.

In the Monty VM, we had a similar construct. Instead of CATCH, it's call MUST_SUCCEED (if I remember correctly). Like:

void test(TRAPS) {
  Foo bar = some_func(x, y, z, MUST_SUCCEED);
  ...
}

-------------

PR: https://git.openjdk.java.net/jdk/pull/2397
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261127: Cleanup THREAD/TRAPS/CHECK usage in CDS code [v2]

David Holmes-2
In reply to this post by David Holmes-2
> 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

David Holmes has updated the pull request incrementally with two additional commits since the last revision:

 - Reverted changes so TRAPS becomes Thread* THREAD again; and removed comment
 - Fix typo

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2397/files
  - new: https://git.openjdk.java.net/jdk/pull/2397/files/6ef59b90..72f196fd

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2397&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2397&range=00-01

  Stats: 25 lines in 3 files changed: 1 ins; 0 del; 24 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2397.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2397/head:pull/2397

PR: https://git.openjdk.java.net/jdk/pull/2397
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261127: Cleanup THREAD/TRAPS/CHECK usage in CDS code [v3]

David Holmes-2
In reply to this post by David Holmes-2
> 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

David Holmes has updated the pull request incrementally with one additional commit since the last revision:

  Missed updating caller of resolve_classes() - which never lets exceptions propagate

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2397/files
  - new: https://git.openjdk.java.net/jdk/pull/2397/files/72f196fd..302c0977

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2397&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2397&range=01-02

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2397.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2397/head:pull/2397

PR: https://git.openjdk.java.net/jdk/pull/2397
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261127: Cleanup THREAD/TRAPS/CHECK usage in CDS code [v3]

Ioi Lam-2
On Fri, 5 Feb 2021 05:14:03 GMT, David Holmes <[hidden email]> 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
>
> David Holmes has updated the pull request incrementally with one additional commit since the last revision:
>
>   Missed updating caller of resolve_classes() - which never lets exceptions propagate

Still good.

-------------

Marked as reviewed by iklam (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2397
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261127: Cleanup THREAD/TRAPS/CHECK usage in CDS code [v3]

David Holmes
On 5/02/2021 3:57 pm, Ioi Lam wrote:

> On Fri, 5 Feb 2021 05:14:03 GMT, David Holmes <[hidden email]> 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
>>
>> David Holmes has updated the pull request incrementally with one additional commit since the last revision:
>>
>>    Missed updating caller of resolve_classes() - which never lets exceptions propagate
>
> Still good.

Thanks Ioi!

David

> -------------
>
> Marked as reviewed by iklam (Reviewer).
>
> PR: https://git.openjdk.java.net/jdk/pull/2397
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261127: Cleanup THREAD/TRAPS/CHECK usage in CDS code [v3]

Coleen Phillimore-3
In reply to this post by David Holmes-2
On Fri, 5 Feb 2021 05:14:03 GMT, David Holmes <[hidden email]> 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
>
> David Holmes has updated the pull request incrementally with one additional commit since the last revision:
>
>   Missed updating caller of resolve_classes() - which never lets exceptions propagate

Looks good!  I don't know why replying in email appended a ? in many of my sentences. Thank you for these changes.

-------------

Marked as reviewed by coleenp (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2397
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261127: Cleanup THREAD/TRAPS/CHECK usage in CDS code [v4]

David Holmes-2
In reply to this post by David Holmes-2
> 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

David Holmes has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains five commits:

 - merge
 - Missed updating caller of resolve_classes() - which never lets exceptions propagate
 - Reverted changes so TRAPS becomes Thread* THREAD again; and removed comment
 - Fix typo
 - 8261127: Cleanup THREAD/TRAPS/CHECK usage in CDS code

-------------

Changes: https://git.openjdk.java.net/jdk/pull/2397/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2397&range=03
  Stats: 85 lines in 9 files changed: 1 ins; 16 del; 68 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2397.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2397/head:pull/2397

PR: https://git.openjdk.java.net/jdk/pull/2397
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261127: Cleanup THREAD/TRAPS/CHECK usage in CDS code [v3]

David Holmes-2
In reply to this post by Coleen Phillimore-3
On Mon, 8 Feb 2021 13:31:05 GMT, Coleen Phillimore <[hidden email]> wrote:

>> David Holmes has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Missed updating caller of resolve_classes() - which never lets exceptions propagate
>
> Looks good!  I don't know why replying in email appended a ? in many of my sentences. Thank you for these changes.

Thanks Coleen and Ioi!

-------------

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