8257241: CDS should not handle disableEagerInitialization for archived lambda proxy classes #1542

calvinccheung · 2020-12-01T19:30:45Z

Please review this change which includes:

If the jdk.internal.lambda.disableEagerInitializationproperty is enabled, the InnerClassLambdaMetafactory will not involve CDS to archive lambda proxy classes or to find them from an archive.
Not passing the initialize (same as !disableEagerInitialization) to LambdaProxyClassArchive.find and eventually to the JVM_LookupLambdaProxyClassFromArchive function.

Testing:

all cds/appcds tests locally on linux-x64
tiers 1 - 4 (in progress)

Progress

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

Issue

JDK-8257241: CDS should not handle disableEagerInitialization for archived lambda proxy classes

Reviewers

Ioi Lam (@iklam - Reviewer) ⚠️ Review applies to fc10e77
Claes Redestad (@cl4es - Reviewer) ⚠️ Review applies to 812777b
Mandy Chung (@mlchung - Reviewer) ⚠️ Review applies to fc10e77

Download

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

…d lambda proxy classes

bridgekeeper · 2020-12-01T19:31:06Z

👋 Welcome back ccheung! 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-01T19:31:42Z

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

core-libs
hotspot

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.

calvinccheung · 2020-12-01T19:32:59Z

/label remove hotspot
/label add hotspot-runtime

openjdk · 2020-12-01T19:33:15Z

@calvinccheung
The hotspot label was successfully removed.

openjdk · 2020-12-01T19:33:48Z

@calvinccheung
The hotspot-runtime label was successfully added.

mlbridge · 2020-12-01T19:38:01Z

Webrevs

mlchung · 2020-12-01T20:02:57Z

The change looks okay. Can you add a test to verify that it won't load from CDS archive if disableEagerInitialization is true?

iklam

LGTM. I agree with Mandy that we need a test case.

cl4es

Looks good to me!

openjdk · 2020-12-01T20:50:33Z

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

8257241: CDS should not handle disableEagerInitialization for archived lambda proxy classes

Reviewed-by: iklam, redestad, mchung

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

7104400: 8257164: Share LambdaForms for VH linkers/invokers
3e89981: 8257623: vmTestbase/nsk/jvmti/ResourceExhausted/resexhausted001/TestDescription.java shouldn't use timeout
93b6ab5: 8256818: SSLSocket that is never bound or connected leaks socket resources
692b273: 8257189: Handle concurrent updates of MH.form better
6704266: 8257565: epsilonBarrierSet.hpp should not include barrierSetAssembler
0b8c780: 8256256: UL should not use heap allocation for output string
2508bc7: 8257140: Crash in JvmtiTagMap::flush_object_free_events()
cfb50a9: 8253916: ResourceExhausted/resexhausted001 crashes on Linux-x64
287b829: 8254877: GCLogPrecious::_lock rank constrains what locks you are allowed to have when crashing
1fd0ea7: 8256382: Use try_lock for hs_err EventLog printing
... and 46 more: https://git.openjdk.java.net/jdk/compare/6eb25d7cb42fb313172d90a56f231743cf6a7884...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.

calvinccheung · 2020-12-01T21:55:14Z

@mlchung, @iklam, @cl4es Thanks for the review.
I've added a test.

mlchung · 2020-12-01T22:16:58Z

test/hotspot/jtreg/runtime/cds/appcds/LambdaEagerInit.java

+ *          from the archive.
+ * @requires vm.cds
+ * @library /test/lib /test/hotspot/jtreg/runtime/cds/appcds
+ * @compile ../../../../../jdk/java/lang/invoke/lambda/LambdaEagerInitTest.java


I think it's better to make a copy of this test along side with this test.

I can make a copy and put it under the open/test/hotspot/jtreg/runtime/cds/appcds/test-classes dir. I'd still need the @compile and would be something like @compile test-classes/LambdaEagerInitTest.java.

The @compile can be avoiding by putting LambdaEagerInitTest into the @library and referencing it as

private static final String mainClass = LambdaEagerInitTest.class.getName();

Yes, @library test-classes works.

mlchung · 2020-12-01T22:19:55Z

test/hotspot/jtreg/runtime/cds/appcds/LambdaEagerInit.java

+              .shouldNotMatch(".cds.*Loaded.*lambda.*proxy")
+              .shouldHaveExitValue(0);
+
+        // run with archive without the -Djdk.internal.lambda.disableEagerInitialization=true property


Minor suggestion: make this test case in a separate testWithEagerInitializationEnabled method. testImpl can be renamed to testWithEagerInitializationDisabled.

I also factored out the archive creation into the createArchiveWithEagerInitializationEnabled method.

2. Modifications to LambdaEagerInit.java per review comments

mlchung · 2020-12-01T23:27:22Z

test/hotspot/jtreg/runtime/cds/appcds/test-classes/LambdaEagerInitTest.java

+ */
+
+/**
+ * @test


This is part of LambdaEagerInit test. This does not need to be @test and so the entire comment block can be removed.

If you want to verify, you could add a test case in LambdaEagerInit.java to run this main class without the CDS archive.

2. Add more test cases using the default CDS archive in LambdaEagerInit.java

calvinccheung · 2020-12-02T22:17:14Z

/integrate

openjdk · 2020-12-02T22:18:16Z

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

7104400: 8257164: Share LambdaForms for VH linkers/invokers
3e89981: 8257623: vmTestbase/nsk/jvmti/ResourceExhausted/resexhausted001/TestDescription.java shouldn't use timeout
93b6ab5: 8256818: SSLSocket that is never bound or connected leaks socket resources
692b273: 8257189: Handle concurrent updates of MH.form better
6704266: 8257565: epsilonBarrierSet.hpp should not include barrierSetAssembler
0b8c780: 8256256: UL should not use heap allocation for output string
2508bc7: 8257140: Crash in JvmtiTagMap::flush_object_free_events()
cfb50a9: 8253916: ResourceExhausted/resexhausted001 crashes on Linux-x64
287b829: 8254877: GCLogPrecious::_lock rank constrains what locks you are allowed to have when crashing
1fd0ea7: 8256382: Use try_lock for hs_err EventLog printing
... and 46 more: https://git.openjdk.java.net/jdk/compare/6eb25d7cb42fb313172d90a56f231743cf6a7884...master

Your commit was automatically rebased without conflicts.

Pushed as commit 3da30e9.

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

8257241: CDS should not handle disableEagerInitialization for archive…

812777b

…d lambda proxy classes

openjdk bot added hotspot hotspot-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels Dec 1, 2020

openjdk bot removed the hotspot hotspot-dev@openjdk.org label Dec 1, 2020

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

calvinccheung marked this pull request as ready for review December 1, 2020 19:34

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

iklam reviewed Dec 1, 2020

View reviewed changes

cl4es approved these changes Dec 1, 2020

View reviewed changes

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

added a test

880407c

mlchung reviewed Dec 1, 2020

View reviewed changes

1. Make a copy of LambdaEagerInitTest.java to the test-classes dir

edd4a09

2. Modifications to LambdaEagerInit.java per review comments

mlchung reviewed Dec 1, 2020

View reviewed changes

1. Update comment block in LambdaEagerInitTest.java

fc10e77

2. Add more test cases using the default CDS archive in LambdaEagerInit.java

iklam approved these changes Dec 2, 2020

View reviewed changes

mlchung approved these changes Dec 2, 2020

View reviewed changes

Skip testing with default CDS archive on aarch64 platform

ec851f0

openjdk bot closed this Dec 2, 2020

openjdk bot added the integrated Pull request has been integrated label Dec 2, 2020

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 2, 2020

calvinccheung deleted the 8257241-ignore-disableEagerInit branch December 2, 2020 22:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8257241: CDS should not handle disableEagerInitialization for archived lambda proxy classes #1542

8257241: CDS should not handle disableEagerInitialization for archived lambda proxy classes #1542

calvinccheung commented Dec 1, 2020 •

edited by openjdk bot

Loading

bridgekeeper bot commented Dec 1, 2020

openjdk bot commented Dec 1, 2020

calvinccheung commented Dec 1, 2020

openjdk bot commented Dec 1, 2020

openjdk bot commented Dec 1, 2020

mlbridge bot commented Dec 1, 2020 •

edited

Loading

mlchung commented Dec 1, 2020

iklam left a comment

cl4es left a comment

openjdk bot commented Dec 1, 2020 •

edited

Loading

calvinccheung commented Dec 1, 2020

mlchung Dec 1, 2020

calvinccheung Dec 1, 2020

iklam Dec 1, 2020

calvinccheung Dec 1, 2020

mlchung Dec 1, 2020

calvinccheung Dec 1, 2020

mlchung Dec 1, 2020

calvinccheung commented Dec 2, 2020

openjdk bot commented Dec 2, 2020

+               */
+              /**
+               * @test

8257241: CDS should not handle disableEagerInitialization for archived lambda proxy classes #1542

8257241: CDS should not handle disableEagerInitialization for archived lambda proxy classes #1542

Conversation

calvinccheung commented Dec 1, 2020 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Dec 1, 2020

openjdk bot commented Dec 1, 2020

calvinccheung commented Dec 1, 2020

openjdk bot commented Dec 1, 2020

openjdk bot commented Dec 1, 2020

mlbridge bot commented Dec 1, 2020 • edited Loading

Webrevs

mlchung commented Dec 1, 2020

iklam left a comment

Choose a reason for hiding this comment

cl4es left a comment

Choose a reason for hiding this comment

openjdk bot commented Dec 1, 2020 • edited Loading

calvinccheung commented Dec 1, 2020

mlchung Dec 1, 2020

Choose a reason for hiding this comment

calvinccheung Dec 1, 2020

Choose a reason for hiding this comment

iklam Dec 1, 2020

Choose a reason for hiding this comment

calvinccheung Dec 1, 2020

Choose a reason for hiding this comment

mlchung Dec 1, 2020

Choose a reason for hiding this comment

calvinccheung Dec 1, 2020

Choose a reason for hiding this comment

mlchung Dec 1, 2020

Choose a reason for hiding this comment

calvinccheung commented Dec 2, 2020

openjdk bot commented Dec 2, 2020

calvinccheung commented Dec 1, 2020 •

edited by openjdk bot

Loading

mlbridge bot commented Dec 1, 2020 •

edited

Loading

openjdk bot commented Dec 1, 2020 •

edited

Loading