8261127: Cleanup THREAD/TRAPS/CHECK usage in CDS code #2397

dholmes-ora · 2021-02-04T05:25:15Z

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

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

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

Reviewers

Ioi Lam (@iklam - Reviewer) ⚠️ Review applies to 302c097
Coleen Phillimore (@coleenp - Reviewer) ⚠️ Review applies to 302c097

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2397/head:pull/2397
$ git checkout pull/2397

bridgekeeper · 2021-02-04T05:25:40Z

👋 Welcome back dholmes! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2021-02-04T05:27:19Z

@dholmes-ora The following label will be automatically applied to this pull request:

hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

dholmes-ora · 2021-02-04T05:29:32Z

/label remove hotspot
/label add hotspot-runtime

openjdk · 2021-02-04T05:29:54Z

@dholmes-ora
The hotspot label was successfully removed.

openjdk · 2021-02-04T05:30:25Z

@dholmes-ora
The hotspot-runtime label was successfully added.

mlbridge · 2021-02-04T05:33:54Z

Webrevs

iklam

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;
}

openjdk · 2021-02-04T06:07:07Z

@dholmes-ora This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

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

Reviewed-by: iklam, coleenp

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

coleenp

I think using CATCH is preferable to passing THREAD with a comment, which is unenforceable in the code. Thanks for cleaning up excess TRAPS.

coleenp · 2021-02-04T13:31:22Z

src/hotspot/share/classfile/javaClasses.hpp

-  static oop  process_archived_mirror(Klass* k, oop mirror, oop archived_mirror, Thread *THREAD)
+  static void archive_basic_type_mirrors() NOT_CDS_JAVA_HEAP_RETURN;
+  static oop  archive_mirror(Klass* k) NOT_CDS_JAVA_HEAP_RETURN_(NULL);
+  static oop  process_archived_mirror(Klass* k, oop mirror, oop archived_mirro)


missing an 'r'.

coleenp · 2021-02-04T13:35:11Z

src/hotspot/share/memory/heapShared.cpp

  resolve_classes_for_subgraphs(fmg_open_archive_subgraph_entry_fields,
                                num_fmg_open_archive_subgraph_entry_fields,
-                                CHECK);
+                                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();

coleenp · 2021-02-04T13:36:19Z

src/hotspot/share/memory/heapShared.cpp

@@ -701,7 +697,7 @@ void HeapShared::resolve_classes_for_subgraphs(ArchivableStaticFieldInfo fields[
    TempNewSymbol klass_name = SymbolTable::new_symbol(info->klass_name);
    InstanceKlass* k = SystemDictionaryShared::find_builtin_class(klass_name);
    assert(k != NULL && k->is_shared_boot_class(), "sanity");
-    resolve_classes_for_subgraph_of(k, CHECK);
+    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.

coleenp · 2021-02-04T13:37:28Z

src/hotspot/share/memory/heapShared.cpp

  assert(is_heap_object_archiving_allowed(), "Sanity check");
  _dump_time_subgraph_info_table = new (ResourceObj::C_HEAP, mtClass)DumpTimeKlassSubGraphInfoTable();
  init_subgraph_entry_fields(closed_archive_subgraph_entry_fields,
                             num_closed_archive_subgraph_entry_fields,
-                             THREAD);
+                             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.

mlbridge · 2021-02-04T22:53:43Z

Mailing list message from David Holmes on hotspot-runtime-dev:

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

mlbridge · 2021-02-04T23:01:45Z

Mailing list message from David Holmes on hotspot-runtime-dev:

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

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

coleenp · 2021-02-04T23:30:28Z

src/hotspot/share/memory/heapShared.cpp

  resolve_classes_for_subgraphs(fmg_open_archive_subgraph_entry_fields,
                                num_fmg_open_archive_subgraph_entry_fields,
-                                CHECK);
+                                THREAD /* exceptions are ignored */);


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();

coleenp · 2021-02-04T23:32:19Z

src/hotspot/share/memory/heapShared.cpp

@@ -701,7 +697,7 @@ void HeapShared::resolve_classes_for_subgraphs(ArchivableStaticFieldInfo fields[
    TempNewSymbol klass_name = SymbolTable::new_symbol(info->klass_name);
    InstanceKlass* k = SystemDictionaryShared::find_builtin_class(klass_name);
    assert(k != NULL && k->is_shared_boot_class(), "sanity");
-    resolve_classes_for_subgraph_of(k, CHECK);
+    resolve_classes_for_subgraph_of(k, THREAD /* exceptions are ignored */);


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

coleenp · 2021-02-04T23:37:52Z

src/hotspot/share/memory/heapShared.cpp

  assert(is_heap_object_archiving_allowed(), "Sanity check");
  _dump_time_subgraph_info_table = new (ResourceObj::C_HEAP, mtClass)DumpTimeKlassSubGraphInfoTable();
  init_subgraph_entry_fields(closed_archive_subgraph_entry_fields,
                             num_closed_archive_subgraph_entry_fields,
-                             THREAD);
+                             THREAD /* aborts on exception */);


This code makes me wonder if Thread* THREAD doesn't make the intent of this code clearer.

mlbridge · 2021-02-04T23:42:43Z

Mailing list message from David Holmes on hotspot-runtime-dev:

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

On Thu, 4 Feb 2021 13:36:19 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 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
-----

mlbridge · 2021-02-05T00:02:43Z

Mailing list message from David Holmes on hotspot-runtime-dev:

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

mlbridge · 2021-02-05T01:27:42Z

Mailing list message from Coleen Phillimore on hotspot-runtime-dev:

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

iklam · 2021-02-05T03:14:42Z

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);
  ...
}

…omment

…ions propagate

iklam

Still good.

mlbridge · 2021-02-05T08:39:45Z

Mailing list message from David Holmes on hotspot-runtime-dev:

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

On Fri, 5 Feb 2021 05:14:03 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

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

coleenp

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

openjdk · 2021-02-08T13:31:51Z

@dholmes-ora this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout 8261127
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

dholmes-ora · 2021-02-08T23:16:26Z

/integrate

dholmes-ora · 2021-02-08T23:16:37Z

Thanks Coleen and Ioi!

openjdk · 2021-02-08T23:17:09Z

@dholmes-ora Pushed as commit f03e839.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

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

6ef59b9

openjdk bot added the rfr Pull request is ready for review label Feb 4, 2021

openjdk bot added the hotspot hotspot-dev@openjdk.org label Feb 4, 2021

openjdk bot removed the hotspot hotspot-dev@openjdk.org label Feb 4, 2021

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Feb 4, 2021

iklam approved these changes Feb 4, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Feb 4, 2021

coleenp suggested changes Feb 4, 2021

View reviewed changes

Fix typo

6bf99f7

dholmes-ora added 2 commits February 4, 2021 23:49

Reverted changes so TRAPS becomes Thread* THREAD again; and removed c…

72f196f

…omment

Missed updating caller of resolve_classes() - which never lets except…

302c097

…ions propagate

iklam approved these changes Feb 5, 2021

View reviewed changes

coleenp approved these changes Feb 8, 2021

View reviewed changes

openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Feb 8, 2021

openjdk bot removed the ready Pull request is ready to be integrated label Feb 8, 2021

merge

4261a37

openjdk bot added ready Pull request is ready to be integrated and removed merge-conflict Pull request has merge conflict with target branch labels Feb 8, 2021

openjdk bot closed this Feb 8, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Feb 8, 2021

dholmes-ora deleted the 8261127 branch February 8, 2021 23:30

8261127: Cleanup THREAD/TRAPS/CHECK usage in CDS code #2397

8261127: Cleanup THREAD/TRAPS/CHECK usage in CDS code #2397

dholmes-ora commented Feb 4, 2021 •

edited by openjdk bot

bridgekeeper bot commented Feb 4, 2021

openjdk bot commented Feb 4, 2021

dholmes-ora commented Feb 4, 2021

openjdk bot commented Feb 4, 2021

openjdk bot commented Feb 4, 2021

mlbridge bot commented Feb 4, 2021 •

edited

iklam left a comment

openjdk bot commented Feb 4, 2021 •

edited

coleenp left a comment

coleenp Feb 4, 2021

coleenp Feb 4, 2021

coleenp Feb 4, 2021

coleenp Feb 4, 2021

coleenp Feb 4, 2021 •

edited

coleenp Feb 4, 2021

coleenp Feb 4, 2021 •

edited

coleenp Feb 4, 2021

mlbridge bot commented Feb 4, 2021

mlbridge bot commented Feb 4, 2021

coleenp Feb 4, 2021

coleenp Feb 4, 2021

coleenp Feb 4, 2021

mlbridge bot commented Feb 4, 2021

mlbridge bot commented Feb 5, 2021

mlbridge bot commented Feb 5, 2021

iklam commented Feb 5, 2021

iklam left a comment

mlbridge bot commented Feb 5, 2021

coleenp left a comment

openjdk bot commented Feb 8, 2021

dholmes-ora commented Feb 8, 2021

dholmes-ora commented Feb 8, 2021

openjdk bot commented Feb 8, 2021

8261127: Cleanup THREAD/TRAPS/CHECK usage in CDS code #2397

8261127: Cleanup THREAD/TRAPS/CHECK usage in CDS code #2397

Conversation

dholmes-ora commented Feb 4, 2021 • edited by openjdk bot

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Feb 4, 2021

openjdk bot commented Feb 4, 2021

dholmes-ora commented Feb 4, 2021

openjdk bot commented Feb 4, 2021

openjdk bot commented Feb 4, 2021

mlbridge bot commented Feb 4, 2021 • edited

Webrevs

iklam left a comment

Choose a reason for hiding this comment

openjdk bot commented Feb 4, 2021 • edited

coleenp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coleenp Feb 4, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coleenp Feb 4, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlbridge bot commented Feb 4, 2021

mlbridge bot commented Feb 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlbridge bot commented Feb 4, 2021

mlbridge bot commented Feb 5, 2021

mlbridge bot commented Feb 5, 2021

iklam commented Feb 5, 2021

iklam left a comment

Choose a reason for hiding this comment

mlbridge bot commented Feb 5, 2021

coleenp left a comment

Choose a reason for hiding this comment

openjdk bot commented Feb 8, 2021

dholmes-ora commented Feb 8, 2021

dholmes-ora commented Feb 8, 2021

openjdk bot commented Feb 8, 2021

dholmes-ora commented Feb 4, 2021 •

edited by openjdk bot

mlbridge bot commented Feb 4, 2021 •

edited

openjdk bot commented Feb 4, 2021 •

edited

coleenp Feb 4, 2021 •

edited

coleenp Feb 4, 2021 •

edited