8243287: Removal of Unsafe::defineAnonymousClass #3974

hseigel · 2021-05-11T12:50:31Z

Please review this large change to remove Unsafe::defineAnonymousClass(). The change removes dAC relevant code and changes a lot of tests. Many of the changed tests need renaming. I hope to do this in a follow up RFE. Some of the tests were modified to use hidden classes, others were deleted because either similar hidden classes tests already exist or they tested dAC specific functionality, such as host classes.

This change was tested with Mach5 tiers 1-2 on Linux, Mac OS, and Windows, and Mach5 tiers 3-7 on Linux x64.

Thanks, Harold

Progress

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

Issue

JDK-8243287: Removal of Unsafe::defineAnonymousClass

Reviewers

Ioi Lam (@iklam - Reviewer) ⚠️ Review applies to 874a160
Mandy Chung (@mlchung - Reviewer) ⚠️ Review applies to 5246dd7
Alan Bateman (@AlanBateman - Reviewer) ⚠️ Review applies to 5246dd7
David Holmes (@dholmes-ora - Reviewer) ⚠️ Review applies to 5246dd7

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3974/head:pull/3974
$ git checkout pull/3974

Update a local copy of the PR:
$ git checkout pull/3974
$ git pull https://git.openjdk.java.net/jdk pull/3974/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3974

View PR using the GUI difftool:
$ git pr show -t 3974

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3974.diff

bridgekeeper · 2021-05-11T12:50:46Z

👋 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 · 2021-05-11T12:53:20Z

@hseigel The following labels will be automatically applied to this pull request:

core-libs
hotspot
serviceability

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

mlbridge · 2021-05-11T12:58:06Z

Webrevs

hseigel · 2021-05-11T12:58:09Z

/csr needed

openjdk · 2021-05-11T12:59:18Z

@hseigel this pull request will not be integrated until the CSR request JDK-8266760 for issue JDK-8243287 has been approved.

test/jdk/java/lang/Class/GetModuleTest.java

iklam

The CDS VM and test changes look OK to me.

src/hotspot/share/oops/instanceMirrorKlass.inline.hpp

mlchung · 2021-05-11T20:49:46Z

src/jdk.internal.vm.ci/share/classes/jdk.vm.ci.meta/src/jdk/vm/ci/meta/MetaUtil.java

@@ -50,7 +50,7 @@ public static String getSimpleName(Class<?> clazz, boolean withEnclosingClass) {
            }
            return simpleName;
        }
-        // Must be an anonymous or local class
+        // Must be a local class


This file should not be changed. It refers to the Java language anonymous class (not VM anonymous class).

Changes have been restored.

openjdk · 2021-05-11T22:24:16Z

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

8243287: Removal of Unsafe::defineAnonymousClass

Reviewed-by: iklam, mchung, alanb, 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 43 new commits pushed to the master branch:

a564f2c: 8266821: G1: Prefetch cards during merge heap roots phase
127bfe4: 8266074: Vtable-based CHA implementation
347d41d: 8164804: sun/security/ssl/SSLSocketImpl/CloseSocket.java makes not reliable time assumption
17ceef9: 8266819: Separate the stop policies from the compile policies completely
a270cbe: 8267043: IntelliJ project doesn't handle generated sources correctly
08a5a5c: 8263382: java/util/logging/ParentLoggersTest.java failed with "checkLoggers: getLoggerNames() returned unexpected loggers"
b50fc5f: 8265528: Specification of BasicSplitPaneDivider::getMinimumSize,getPreferredSize doesn't match with its behavior.
d215743: 8231031: runtime/ReservedStack/ReservedStackTest.java fails after jsr166 refresh
ab17be2: 8252530: Fix inconsistencies in hotspot whitebox
2568d18: 8267047: Put serviceability/sa/TestJmapCoreMetaspace.java back on ZGC problem list due to JDK-8267045
... and 33 more: https://git.openjdk.java.net/jdk/compare/0344e751e2bfd2c6238fc1cea3db19015ce0db88...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.

mlchung · 2021-05-12T02:47:57Z

I reviewed java.base and took a pass on the test changes. Here are some comments:

test/hotspot/jtreg/runtime/HiddenClasses/TestHiddenClassUnloading.java has this comment:

+// This is based on test compiler/classUnloading/anonymousClass/TestAnonymousClassUnloading.java

This comment can be removed as this test will be removed. A few tests under test/hotspot/jtreg/runtime/HiddenClasses also have similar comment that should be removed.

test/hotspot/jtreg/vmTestbase/vm/mlvm/anonloader/func/castToGrandparent/Test.java
test/hotspot/jtreg/vmTestbase/vm/mlvm/anonloader/func/classNameInStackTrace/Test.java

I think these tests are already well covered by test/hotspot/jtreg/runtime/HiddenClasses/CastToParentTest.java
and test/hotspot/jtreg/runtime/HiddenClasses/HiddenClassStack.java
I suggest to copy the description from the anonloader tests to these hidden class tests

test/hotspot/jtreg/vmTestbase/vm/mlvm/anonloader/share/StressClassLoadingTest.java

test/hotspot/jtreg/runtime/HiddenClasses/StressClassLoadingTest.java is a subset of this test. Should we remove test/hotspot/jtreg/runtime/HiddenClasses/StressClassLoadingTest.java?

test/jdk/java/lang/invoke/VMAnonymousClass.java

FYI. I have added a new test to verify hidden class (see JDK-8266925)

hseigel · 2021-05-12T16:06:34Z

Thanks Alan, Ioi, and Mandy for looking at this. The latest changes should address the issues that you found.
Harold

mlchung

Overall looks good to me.

mlchung · 2021-05-12T22:28:37Z

test/hotspot/jtreg/runtime/HiddenClasses/StressHiddenClasses.java

@@ -36,7 +36,6 @@
 import static java.lang.invoke.MethodHandles.Lookup.ClassOption.*;
 import jdk.test.lib.compiler.InMemoryJavaCompiler;

-// This test is based on vmTestbase/vm/mlvm/anonloader/share/StressClassLoadingTest.java
 public class StressHiddenClasses {


Since StressClassLoadingTest is revised to test hidden class, this test is a subset of it.
I think this can be removed as JDK-8265694 suggests.

Thanks Mandy. I will remove the test as the fix for JDK-8265694 after this change is pushed.

src/hotspot/share/classfile/classLoaderData.cpp

dholmes-ora

Hi Harold,

Big change! :) This looks good. All the removals of references to unsafe_anonymous were easy enough to follow.

I didn't realize that cp patching and psuedo-strings were only related to VM anonymous. It is good to see all that code go as well (but hard to see if you got it all :) ). But as per comment below are we sure psuedo-strings can't be used elsewhere?

Thanks,
David

dholmes-ora · 2021-05-13T07:19:03Z

src/hotspot/share/oops/constantPool.hpp

-  // Methods internally created for method handles may also
-  // use pseudo-strings to link themselves to related metaobjects.


Is this comment wrong? Are psuedo-strings not used by anything now?

Thanks for looking at this. I discussed pseudo strings with Coleen and we didn't find any use of them besides unsafe.DefineAnonymousClass.

hseigel · 2021-05-13T12:45:13Z

Thanks Ioi, Alan, Mandy, and David for reviewing this big change!

/integrate

openjdk · 2021-05-13T12:47:20Z

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

a564f2c: 8266821: G1: Prefetch cards during merge heap roots phase
127bfe4: 8266074: Vtable-based CHA implementation
347d41d: 8164804: sun/security/ssl/SSLSocketImpl/CloseSocket.java makes not reliable time assumption
17ceef9: 8266819: Separate the stop policies from the compile policies completely
a270cbe: 8267043: IntelliJ project doesn't handle generated sources correctly
08a5a5c: 8263382: java/util/logging/ParentLoggersTest.java failed with "checkLoggers: getLoggerNames() returned unexpected loggers"
b50fc5f: 8265528: Specification of BasicSplitPaneDivider::getMinimumSize,getPreferredSize doesn't match with its behavior.
d215743: 8231031: runtime/ReservedStack/ReservedStackTest.java fails after jsr166 refresh
ab17be2: 8252530: Fix inconsistencies in hotspot whitebox
2568d18: 8267047: Put serviceability/sa/TestJmapCoreMetaspace.java back on ZGC problem list due to JDK-8267045
... and 33 more: https://git.openjdk.java.net/jdk/compare/0344e751e2bfd2c6238fc1cea3db19015ce0db88...master

Your commit was automatically rebased without conflicts.

Pushed as commit e14b026.

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

mlbridge · 2021-05-14T18:29:15Z

Mailing list message from Brian Goetz on hotspot-dev:

There may be some JDK code that checks for anon classes by comparing the name to see if it contains a slash, especially tests, but which don?t say ?anonymous?. Did you do a search for these idioms too, which are now dead tests?

Sent from my iPad

mlbridge · 2021-05-14T18:29:26Z

Mailing list message from Harold Seigel on hotspot-dev:

Hi Brian,

Thanks for looking at this.

The JDK no longer creates unsafe anon classes.? So, those tests could
only find an unsafe anonymous class if they explicitly created one.? In
which case, the tests would need to call Unsafe.defineAnonymousClass().?
And, hopefully, those tests have been handled in this webrev.

If there are dead tests then they probably died when the JDK stopped
creating unsafe anon classes.? Note that none of them showed up as
failures during regression testing.

Harold

On 5/11/2021 9:20 AM, Brian Goetz wrote:

There may be some JDK code that checks for anon classes by comparing the name to see if it contains a slash, especially tests, but which don?t say ?anonymous?. Did you do a search for these idioms too, which are now dead tests?

Sent from my iPad

mlbridge · 2021-05-14T18:29:29Z

Mailing list message from Harold Seigel on hotspot-dev:

Hi Brian,

Thanks for looking at this.

The JDK no longer creates unsafe anon classes.? So, those tests could
only find an unsafe anonymous class if they explicitly created one.? In
which case, the tests would need to call Unsafe.defineAnonymousClass().?
And, hopefully, those tests have been handled in this webrev.

If there are dead tests then they probably died when the JDK stopped
creating unsafe anon classes.? Note that none of them showed up as
failures during regression testing.

Harold

On 5/11/2021 9:20 AM, Brian Goetz wrote:

There may be some JDK code that checks for anon classes by comparing the name to see if it contains a slash, especially tests, but which don?t say ?anonymous?. Did you do a search for these idioms too, which are now dead tests?

Sent from my iPad

mlbridge · 2021-05-14T18:29:30Z

Mailing list message from Mandy Chung on hotspot-dev:

I did a search on java.base and the tests on `String::indexOf` and
`String::contains` of a slash and don't spot any such test.? The JDK has
no use of VM anonymous class.?? If the test is trying to determine if
it's lambda proxy class, it should be converted to call
`Class::isHidden` but testing of the class name containing a slash is
still valid (I haven't found any of such test though).

Mandy

On 5/11/21 6:20 AM, Brian Goetz wrote:

There may be some JDK code that checks for anon classes by comparing the name to see if it contains a slash, especially tests, but which don?t say ?anonymous?. Did you do a search for these idioms too, which are now dead tests?

Sent from my iPad

mlbridge · 2021-05-14T18:29:35Z

Mailing list message from Brian Goetz on hotspot-dev:

Thanks for checking.? I thought I remembered something like this
somewhere, but it may be that you already fixed such tests when you did
hidden classes?? In any case, seems like we're all good now.

Cheers,
-Brian

On 5/13/2021 2:50 PM, Mandy Chung wrote:

I did a search on java.base and the tests on `String::indexOf` and
`String::contains` of a slash and don't spot any such test.? The JDK
has no use of VM anonymous class. If the test is trying to determine
if it's lambda proxy class, it should be converted to call
`Class::isHidden` but testing of the class name containing a slash is
still valid (I haven't found any of such test though).

Mandy

On 5/11/21 6:20 AM, Brian Goetz wrote:

There may be some JDK code that checks for anon classes by comparing the name to see if it contains a slash, especially tests, but which don?t say ?anonymous?. Did you do a search for these idioms too, which are now dead tests?

Sent from my iPad

mlbridge · 2021-05-14T18:29:37Z

Mailing list message from Mandy Chung on hotspot-dev:

I did a search on java.base and the tests on `String::indexOf` and
`String::contains` of a slash and don't spot any such test.? The JDK has
no use of VM anonymous class.?? If the test is trying to determine if
it's lambda proxy class, it should be converted to call
`Class::isHidden` but testing of the class name containing a slash is
still valid (I haven't found any of such test though).

Mandy

On 5/11/21 6:20 AM, Brian Goetz wrote:

There may be some JDK code that checks for anon classes by comparing the name to see if it contains a slash, especially tests, but which don?t say ?anonymous?. Did you do a search for these idioms too, which are now dead tests?

Sent from my iPad

mlbridge · 2021-05-14T18:29:38Z

Mailing list message from Mandy Chung on hotspot-dev:

Maybe you were thinking
`jdk.internal.reflect.ReflectUtil::isVMAnonymousClass` that is now
removed by this patch [1].

[1]
https://github.com//pull/3974/files#diff-1af3026a3b4942af3ebe6a4df02f8952fb9d51bf93336a2f7f93ce175d047574

On 5/13/21 12:03 PM, Brian Goetz wrote:

Thanks for checking.? I thought I remembered something like this
somewhere, but it may be that you already fixed such tests when you
did hidden classes?? In any case, seems like we're all good now.

Cheers,
-Brian

On 5/13/2021 2:50 PM, Mandy Chung wrote:

I did a search on java.base and the tests on `String::indexOf` and
`String::contains` of a slash and don't spot any such test.? The JDK
has no use of VM anonymous class.?? If the test is trying to
determine if it's lambda proxy class, it should be converted to call
`Class::isHidden` but testing of the class name containing a slash is
still valid (I haven't found any of such test though).

Mandy

On 5/11/21 6:20 AM, Brian Goetz wrote:

There may be some JDK code that checks for anon classes by comparing the name to see if it contains a slash, especially tests, but which don?t say ?anonymous?. Did you do a search for these idioms too, which are now dead tests?

Sent from my iPad

mlbridge · 2021-05-14T18:29:42Z

Mailing list message from Brian Goetz on hotspot-dev:

Thanks for checking.? I thought I remembered something like this
somewhere, but it may be that you already fixed such tests when you did
hidden classes?? In any case, seems like we're all good now.

Cheers,
-Brian

On 5/13/2021 2:50 PM, Mandy Chung wrote:

I did a search on java.base and the tests on `String::indexOf` and
`String::contains` of a slash and don't spot any such test.? The JDK
has no use of VM anonymous class. If the test is trying to determine
if it's lambda proxy class, it should be converted to call
`Class::isHidden` but testing of the class name containing a slash is
still valid (I haven't found any of such test though).

Mandy

On 5/11/21 6:20 AM, Brian Goetz wrote:

There may be some JDK code that checks for anon classes by comparing the name to see if it contains a slash, especially tests, but which don?t say ?anonymous?. Did you do a search for these idioms too, which are now dead tests?

Sent from my iPad

mlbridge · 2021-05-14T18:29:48Z

Mailing list message from Mandy Chung on hotspot-dev:

Maybe you were thinking
`jdk.internal.reflect.ReflectUtil::isVMAnonymousClass` that is now
removed by this patch [1].

[1]
https://github.com//pull/3974/files#diff-1af3026a3b4942af3ebe6a4df02f8952fb9d51bf93336a2f7f93ce175d047574

On 5/13/21 12:03 PM, Brian Goetz wrote:

Thanks for checking.? I thought I remembered something like this
somewhere, but it may be that you already fixed such tests when you
did hidden classes?? In any case, seems like we're all good now.

Cheers,
-Brian

On 5/13/2021 2:50 PM, Mandy Chung wrote:

I did a search on java.base and the tests on `String::indexOf` and
`String::contains` of a slash and don't spot any such test.? The JDK
has no use of VM anonymous class.?? If the test is trying to
determine if it's lambda proxy class, it should be converted to call
`Class::isHidden` but testing of the class name containing a slash is
still valid (I haven't found any of such test though).

Mandy

On 5/11/21 6:20 AM, Brian Goetz wrote:

There may be some JDK code that checks for anon classes by comparing the name to see if it contains a slash, especially tests, but which don?t say ?anonymous?. Did you do a search for these idioms too, which are now dead tests?

Sent from my iPad

8243287: Removal of Unsafe::defineAnonymousClass

233a472

openjdk bot added the rfr Pull request is ready for review label May 11, 2021

openjdk bot added serviceability serviceability-dev@openjdk.org hotspot hotspot-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels May 11, 2021

openjdk bot added the csr Pull request needs approved CSR before integration label May 11, 2021

AlanBateman reviewed May 11, 2021

View reviewed changes

test/jdk/java/lang/Class/GetModuleTest.java Show resolved Hide resolved

hseigel added 2 commits May 11, 2021 14:05

test fixes

35c6634

fix GetModuleTest.java

874a160

iklam approved these changes May 11, 2021

View reviewed changes

src/hotspot/share/oops/instanceMirrorKlass.inline.hpp Outdated Show resolved Hide resolved

mlchung reviewed May 11, 2021

View reviewed changes

openjdk bot removed the csr Pull request needs approved CSR before integration label May 11, 2021

openjdk bot added the ready Pull request is ready to be integrated label May 11, 2021

test changes and small fixes

5246dd7

mlchung approved these changes May 12, 2021

View reviewed changes

AlanBateman approved these changes May 13, 2021

View reviewed changes

dholmes-ora approved these changes May 13, 2021

View reviewed changes

fix Weak hidden comment

3867576

openjdk bot closed this May 13, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated labels May 13, 2021

openjdk bot removed the rfr Pull request is ready for review label May 13, 2021

8243287: Removal of Unsafe::defineAnonymousClass #3974

8243287: Removal of Unsafe::defineAnonymousClass #3974

hseigel commented May 11, 2021 •

edited by openjdk bot

bridgekeeper bot commented May 11, 2021

openjdk bot commented May 11, 2021

mlbridge bot commented May 11, 2021 •

edited

hseigel commented May 11, 2021

openjdk bot commented May 11, 2021

iklam left a comment

mlchung May 11, 2021

hseigel May 12, 2021

openjdk bot commented May 11, 2021 •

edited

mlchung commented May 12, 2021

hseigel commented May 12, 2021

mlchung left a comment

mlchung May 12, 2021

hseigel May 13, 2021

dholmes-ora left a comment

dholmes-ora May 13, 2021

hseigel May 13, 2021

hseigel commented May 13, 2021

openjdk bot commented May 13, 2021

mlbridge bot commented May 14, 2021

mlbridge bot commented May 14, 2021

mlbridge bot commented May 14, 2021

mlbridge bot commented May 14, 2021

mlbridge bot commented May 14, 2021

mlbridge bot commented May 14, 2021

mlbridge bot commented May 14, 2021

mlbridge bot commented May 14, 2021

mlbridge bot commented May 14, 2021

		// Methods internally created for method handles may also
		// use pseudo-strings to link themselves to related metaobjects.

8243287: Removal of Unsafe::defineAnonymousClass #3974

8243287: Removal of Unsafe::defineAnonymousClass #3974

Conversation

hseigel commented May 11, 2021 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented May 11, 2021

openjdk bot commented May 11, 2021

mlbridge bot commented May 11, 2021 • edited

Webrevs

hseigel commented May 11, 2021

openjdk bot commented May 11, 2021

iklam left a comment

Choose a reason for hiding this comment

mlchung May 11, 2021

Choose a reason for hiding this comment

hseigel May 12, 2021

Choose a reason for hiding this comment

openjdk bot commented May 11, 2021 • edited

mlchung commented May 12, 2021

hseigel commented May 12, 2021

mlchung left a comment

Choose a reason for hiding this comment

mlchung May 12, 2021

Choose a reason for hiding this comment

hseigel May 13, 2021

Choose a reason for hiding this comment

dholmes-ora left a comment

Choose a reason for hiding this comment

dholmes-ora May 13, 2021

Choose a reason for hiding this comment

hseigel May 13, 2021

Choose a reason for hiding this comment

hseigel commented May 13, 2021

openjdk bot commented May 13, 2021

mlbridge bot commented May 14, 2021

mlbridge bot commented May 14, 2021

mlbridge bot commented May 14, 2021

mlbridge bot commented May 14, 2021

mlbridge bot commented May 14, 2021

mlbridge bot commented May 14, 2021

mlbridge bot commented May 14, 2021

mlbridge bot commented May 14, 2021

mlbridge bot commented May 14, 2021

hseigel commented May 11, 2021 •

edited by openjdk bot

mlbridge bot commented May 11, 2021 •

edited

openjdk bot commented May 11, 2021 •

edited