8257700: Add logging for sealed classes in JVM_GetPermittedSubclasses #1780

hseigel · 2020-12-15T15:45:25Z

Please review this small fix for JDK-8257700 to add logging to JVM_GetPermittedSubclasses(). The fix was tested with Mach5 tiers 1 and 2 on Linux, MacOS, and WIndows, tiers 3-5 on Linux x64, and by running the hotspot runtime sealed tests with -Xlog:class+sealed=trace and looking at the output.

Thanks, Harold

Progress

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

Issue

JDK-8257700: Add logging for sealed classes in JVM_GetPermittedSubclasses

Reviewers

Lois Foltan (@lfoltan - Reviewer) ⚠️ Review applies to 4cb2719
Coleen Phillimore (@coleenp - Reviewer)
David Holmes (@dholmes-ora - Reviewer) ⚠️ Review applies to 4cb2719

Download

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

bridgekeeper · 2020-12-15T15:46:17Z

👋 Welcome back hseigel! 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 · 2020-12-15T15:47:06Z

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

mlbridge · 2020-12-15T15:50:04Z

Webrevs

lfoltan

Looks good, one minor comment.
Lois

lfoltan · 2020-12-15T17:29:28Z

src/hotspot/share/prims/jvm.cpp

@@ -2160,6 +2176,7 @@ JVM_ENTRY(jobjectArray, JVM_GetPermittedSubclasses(JNIEnv* env, jclass current))
    }
    return (jobjectArray)JNIHandles::make_local(THREAD, result());
  } else {
+    log_trace(class, sealed)("Type %s is not sealed", ik->external_name());


I would remove this line in favor of adding adding an indication of sealed or not sealed to line #2129. See my suggested change above.

lfoltan · 2020-12-15T17:33:18Z

src/hotspot/share/prims/jvm.cpp

@@ -2127,10 +2126,14 @@ JVM_ENTRY(jobjectArray, JVM_GetPermittedSubclasses(JNIEnv* env, jclass current))
  Klass* c = java_lang_Class::as_Klass(mirror);
  assert(c->is_instance_klass(), "must be");
  InstanceKlass* ik = InstanceKlass::cast(c);
+  log_trace(class, sealed)("Calling GetPermittedSubclasses for type %s", ik->external_name());


Consider adding a sealed indication to this log_trace which would allow you to delete line #2179. Something like:
("Calling GetPermittedSubclasses for %s type %s", ik->external_name(), (ik->is_sealed() ? "sealed" : "non-sealed"))

openjdk · 2020-12-15T17:34:09Z

@hseigel 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:

8257700: Add logging for sealed classes in JVM_GetPermittedSubclasses

Reviewed-by: lfoltan, coleenp, dholmes

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 16 new commits pushed to the master branch:

6eca296: 8258420: Move URL configuration from Docs.gmk to conf dir
3c66485: 8257906: JFR: RecordingStream leaks memory
0c8cc2c: 8258058: improve description of OutOfMemoryError relevant flags
cdb5342: 8258252: Move PtrQueue enqueue to PtrQueueSet subclasses
17ace83: 8258074: Move some flags related to compiler to compiler_globals.hpp
47ba652: 8258455: problem list tools/jdeprscan/tests/jdk/jdeprscan/TestRelease.java
1e77896: 8236225: Remove expired flags in JDK 17
4d6f318: 8257726: Make -XX:+StressLdcRewrite option a diagnostic option
8bf46c7: 8258438: build error in test/hotspot/gtest/runtime/test_os.cpp
7977e38: 8254024: Enhance native libs for AWT and Swing to work with GraalVM Native Image
... and 6 more: https://git.openjdk.java.net/jdk/compare/a372be4ba2216adb325f7616e54c1d58ac8fdf2a...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this 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.

hseigel · 2020-12-15T21:59:34Z

Please review this updated webrev. It contains the change suggested by Lois. Here's sample output from the added logging:

[0.355s][trace][class,sealed] Calling GetPermittedSubclasses for sealed type NoLoadSubclasses
[0.355s][trace][class,sealed] - sealed class has 2 permitted subclasses
[0.355s][trace][class,sealed] - [1] = ExistingClassFile
[0.355s][trace][class,sealed] - resolution of permitted subclass I/Dont/Exist/Either failed: java.lang.NoClassDefFoundError: I/Dont/Exist/Either
[0.355s][trace][class,sealed] - compacting array from length 2 to 1

Thanks, Harold

lfoltan · 2020-12-15T22:07:02Z

Looks good. Thanks for making the change I suggested!

src/hotspot/share/prims/jvm.cpp

dholmes-ora

Hi Harold,
Looks good. One minor comment below.
Thanks,
David

src/hotspot/share/prims/jvm.cpp

hseigel · 2020-12-16T16:11:15Z

Thanks Lois, Coleen, and David for reviewing this change. Please review the latest commit that adds the missing ResourceMark and removes the useless logging statement that David mentioned.
Thanks! Harold

coleenp

Looks good!

hseigel · 2020-12-16T19:20:31Z

/integrate

openjdk · 2020-12-16T19:21:17Z

@hseigel Since your change was applied there have been 19 commits pushed to the master branch:

c463264: 8258048: Placeholder hash code is the same as Dictionary hash code
ab5d581: 8258447: Move make/hotspot/hotspot.script to make/scripts
70183f4: 8251438: Issues with our POSIX set_signal_handler()
6eca296: 8258420: Move URL configuration from Docs.gmk to conf dir
3c66485: 8257906: JFR: RecordingStream leaks memory
0c8cc2c: 8258058: improve description of OutOfMemoryError relevant flags
cdb5342: 8258252: Move PtrQueue enqueue to PtrQueueSet subclasses
17ace83: 8258074: Move some flags related to compiler to compiler_globals.hpp
47ba652: 8258455: problem list tools/jdeprscan/tests/jdk/jdeprscan/TestRelease.java
1e77896: 8236225: Remove expired flags in JDK 17
... and 9 more: https://git.openjdk.java.net/jdk/compare/a372be4ba2216adb325f7616e54c1d58ac8fdf2a...master

Your commit was automatically rebased without conflicts.

Pushed as commit 330ce86.

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

8257700: Add logging for sealed classes in JVM_GetPermittedSubclasses

6c8d830

openjdk bot added the rfr Pull request is ready for review label Dec 15, 2020

openjdk bot added the hotspot hotspot-dev@openjdk.org label Dec 15, 2020

lfoltan approved these changes Dec 15, 2020

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 15, 2020

8257700: Add logging for sealed classes in JVM_GetPermittedSubclasses

4cb2719

lfoltan approved these changes Dec 15, 2020

View reviewed changes

coleenp suggested changes Dec 15, 2020

View reviewed changes

src/hotspot/share/prims/jvm.cpp Show resolved Hide resolved

dholmes-ora approved these changes Dec 16, 2020

View reviewed changes

src/hotspot/share/prims/jvm.cpp Outdated Show resolved Hide resolved

8257700: Add logging for sealed classes in JVM_GetPermittedSubclasses

7883a52

coleenp approved these changes Dec 16, 2020

View reviewed changes

openjdk bot closed this Dec 16, 2020

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 Dec 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8257700: Add logging for sealed classes in JVM_GetPermittedSubclasses #1780

8257700: Add logging for sealed classes in JVM_GetPermittedSubclasses #1780

hseigel commented Dec 15, 2020 •

edited by openjdk bot

Loading

bridgekeeper bot commented Dec 15, 2020

openjdk bot commented Dec 15, 2020

mlbridge bot commented Dec 15, 2020 •

edited

Loading

lfoltan left a comment

lfoltan Dec 15, 2020

lfoltan Dec 15, 2020

openjdk bot commented Dec 15, 2020 •

edited

Loading

hseigel commented Dec 15, 2020

lfoltan commented Dec 15, 2020

dholmes-ora left a comment

hseigel commented Dec 16, 2020

coleenp left a comment

hseigel commented Dec 16, 2020

openjdk bot commented Dec 16, 2020

8257700: Add logging for sealed classes in JVM_GetPermittedSubclasses #1780

8257700: Add logging for sealed classes in JVM_GetPermittedSubclasses #1780

Conversation

hseigel commented Dec 15, 2020 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Dec 15, 2020

openjdk bot commented Dec 15, 2020

mlbridge bot commented Dec 15, 2020 • edited Loading

Webrevs

lfoltan left a comment

Choose a reason for hiding this comment

lfoltan Dec 15, 2020

Choose a reason for hiding this comment

lfoltan Dec 15, 2020

Choose a reason for hiding this comment

openjdk bot commented Dec 15, 2020 • edited Loading

hseigel commented Dec 15, 2020

lfoltan commented Dec 15, 2020

dholmes-ora left a comment

Choose a reason for hiding this comment

hseigel commented Dec 16, 2020

coleenp left a comment

Choose a reason for hiding this comment

hseigel commented Dec 16, 2020

openjdk bot commented Dec 16, 2020

hseigel commented Dec 15, 2020 •

edited by openjdk bot

Loading

mlbridge bot commented Dec 15, 2020 •

edited

Loading

openjdk bot commented Dec 15, 2020 •

edited

Loading