8311591: Add SystemModulesPlugin test case that splits module descriptors with new local variables defined by DedupSetBuilder by Siedlerchr · Pull Request #15234 · openjdk/jdk

Siedlerchr · 2023-08-10T21:42:41Z

Add new test case with sample modules that contains some requires/exports/uses/provides.

We are just unsure if and how we should add some last step of verificaiton with the extracted and decompiled class.

Follow up task from #14408

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8311591: Add SystemModulesPlugin test case that splits module descriptors with new local variables defined by DedupSetBuilder (Bug - P3)

Reviewers

Mandy Chung (@mlchung - Reviewer) ⚠️ Review applies to 2bcdd2fc

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/15234/head:pull/15234
$ git checkout pull/15234

Update a local copy of the PR:
$ git checkout pull/15234
$ git pull https://git.openjdk.org/jdk.git pull/15234/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 15234

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/15234.diff

Webrev

Link to Webrev Comment

* origin/fix-8311591: Set module split size to 1

prepare methods for verifying

use parameter for jlink

bridgekeeper · 2023-08-10T21:43:37Z

👋 Welcome back Siedlerchr! 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 · 2023-08-10T21:44:46Z

⚠️ @Siedlerchr This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

openjdk · 2023-08-10T21:45:03Z

@Siedlerchr The following label will be automatically applied to this pull request:

core-libs

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.

openjdk · 2023-08-10T22:01:36Z

@Siedlerchr Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

…tors with new local variables defined by DedupSetBuilder Co-authored-by: Oliver Kopp <kopp.dev@gmail.com>

mlbridge · 2023-08-10T22:06:58Z

Webrevs

openjdk · 2023-08-10T22:07:55Z

@Siedlerchr Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

koppor · 2023-08-10T22:09:44Z

We accidently pushed the full internal history - and "immediatly" squashed into one commit. From now on, we will update the branch "as usual"

* upstream/master: (49 commits) 8313904: [macos] All signing tests which verifies unsigned app images are failing 8314139: TEST_BUG: runtime/os/THPsInThreadStackPreventionTest.java could fail on machine with large number of cores 8313798: [aarch64] sun/tools/jhsdb/HeapDumpTestWithActiveProcess.java sometimes times out on aarch64 8313244: NM flags handling in configure process 8314113: G1: Remove unused G1CardSetInlinePtr::card_at 8311648: Refactor the Arena/Chunk/ChunkPool interface 8313224: Avoid calling JavaThread::current() in MemAllocator::Allocation constructor 8312461: JNI warnings in SunMSCApi provider 8312882: Update the CONTRIBUTING.md with pointers to lifecycle of a PR 8304292: Memory leak related to ClassLoader::update_class_path_entry_list 8313899: JVMCI exception Translation can fail in TranslatedException.<clinit> 8313633: [macOS] java/awt/dnd/NextDropActionTest/NextDropActionTest.java fails with java.lang.RuntimeException: wrong next drop action! 8312259: StatusResponseManager unused code clean up 8314061: [JVMCI] DeoptimizeALot stress logic breaks deferred barriers 8313905: Checked_cast assert in CDS compare_by_loader 8313654: Test WaitNotifySuspendedVThreadTest.java timed out 8312194: test/hotspot/jtreg/applications/ctw/modules/jdk_crypto_ec.java cannot handle empty modules 8313616: support loading library members on AIX in os::dll_load 8313882: Fix -Wconversion warnings in runtime code 8313239: InetAddress.getCanonicalHostName may return ip address if reverse lookup fails ...

* origin/fix-8311591: 8311591: Add SystemModulesPlugin test case that splits module descriptors with new local variables defined by DedupSetBuilder

mlchung

Thanks for adding this case. The steps to extract from jimage and inspect the generated bytecode of SystemModules$all is for us to manually verify if the test does the work, i.e. each subX method adds more local variables due to deduplication. It's not needed in the test itself.

Since the batch size is 1, I would suggest that p4.Main can also load jdk.internal.module.SystemModules\$all and verify the expected numbers of subX methods (one per each module). To find all modules in the image, you can simply do ModuleFinder.ofSystem().findAll().

BTW, dedup/src/* files should also have the copyright header.

mlchung · 2023-08-14T17:57:04Z

+    provides ServiceInterface
+            with AInterface;


Nit: this can be in 1 line. No line break needed. Same for other module-info file.

mlchung · 2023-08-14T18:01:35Z

+                .output(src.resolve("out-jlink-dedup"))
+                .addMods("m1")
+                .addMods("m2")
+                .addMods("m2")


I think it's not needed.

mlchung · 2023-08-14T21:48:58Z

 *          jdk.jlink/jdk.tools.jimage
 *          jdk.compiler
 * @build tests.*
+ *        jdk.test.lib.compiler.CompilerUtils


This change is not needed, right?

mlchung · 2023-08-14T21:49:23Z

@@ -0,0 +1,145 @@
+/*
+ * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.


copyright header start year is 2023.

mlchung · 2023-08-14T21:54:21Z

+
+    public static void main(String[] args) throws Throwable {
+        compileAll();
+        Path src = Paths.get("bug8311591");


suggest to rename src to image to make it clear that it's the resulting image.

Revert unnecesary CompilerUtil

Assert number of module description generated sub modules Co-authored-by: Oliver Kopp <kopp.dev@gmail.com>

Siedlerchr · 2023-08-15T19:11:47Z

Since the batch size is 1, I would suggest that p4.Main can also load jdk.internal.module.SystemModules\$all and verify the expected numbers of subX methods (one per each module). To find all modules in the image, you can simply do ModuleFinder.ofSystem().findAll().

Thanks for the suggestion. We added now a check in the main class for the number of generated sub methods. We put a fixed number, as we did not get the relation between the number of "require modules" statements and the total number of modules.

mlchung

Looks good. Since the image is created with batchSize = 1, it means that one subX method is created for each module linked in the image. So the test can count the number of modules in the image and the number of subX methods should match the module count. I include the suggested code to check that.

mlchung · 2023-08-15T19:27:25Z

+        extractJImage(image);
+        decompileWitJavap(image);


No needed. These 2 lines along with the methods can be deleted.

mlchung · 2023-08-15T19:28:23Z

+                .output(image.resolve("out-jlink-dedup"))
+                .addMods("m1")
+                .addMods("m2")
+                .addMods("m2")


m2 is added twice. The duplicated line should be removed.

mlchung · 2023-08-15T19:28:41Z

+
+/*
+ * @test
+ * @summary Make sure that modules can be linked using jlink and deduplication works correctly when creating sub methods


please wrap this long line.

mlchung · 2023-08-15T19:28:55Z

@@ -0,0 +1,13 @@
+package p1;


missing copyright header.

Co-authored-by: Mandy Chung <mandy.chung@oracle.com>

Remove duplicated module in add mods call Co-authored-by: Oliver Kopp <kopp.dev@gmail.com>

* origin/fix-8311591: Update test/jdk/tools/jlink/dedup/src/m4/p4/Main.java Update test/jdk/tools/jlink/dedup/src/m4/p4/Main.java

Siedlerchr · 2023-08-15T19:53:23Z

Thanks a lot for your explanation and suggestions! Now we understood this :)

mlchung

JLinkDedupTestBatchSizeOne.java line 115-144 can be deleted; otherwise, looks good.

mlchung · 2023-08-15T20:41:03Z

+import java.util.ServiceLoader;
+
+public class Main {
+    private final static int MODULE_SUB_METHOD_COUNT = 9;


This is leftover and should be deleted

Ah yeah overlooked that!

openjdk · 2023-08-15T20:42:41Z

⚠️ @Siedlerchr the full name on your profile does not match the author name in this pull requests' HEAD commit. If this pull request gets integrated then the author name from this pull requests' HEAD commit will be used for the resulting commit. If you wish to push a new commit with a different author name, then please run the following commands in a local repository of your personal fork:

$ git checkout fix-8311591
$ git commit --author='Preferred Full Name <you@example.com>' --allow-empty -m 'Update full name'
$ git push

openjdk · 2023-08-15T20:42:42Z

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

8311591: Add SystemModulesPlugin test case that splits module descriptors with new local variables defined by DedupSetBuilder

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

f66c73d: 8314166: Update googletest to v1.14.0
f239954: 8310134: NMT: thread count in Thread section of VM.native_memory output confusing with virtual threads
2e8a0ab: 8314120: Add tests for FileDescriptor.sync
80809ef: 8314248: Remove HotSpotConstantPool::isResolvedDynamicInvoke
004651d: 8311557: [JVMCI] deadlock with JVMTI thread suspension
9ded868: 8314114: Fix -Wconversion warnings in os code, primarily linux
a02d65e: 8310308: IR Framework: check for type and size of vector nodes
dff99f7: 8313782: Add user-facing warning if THPs are enabled but cannot be used
f4e72c5: 8313949: Missing word in GPLv2 license text in StackMapTableAttribute.java
6338927: 8314197: AttachListener::pd_find_operation always returning nullptr
... and 25 more: https://git.openjdk.org/jdk/compare/ec0cc6300a02dd92b25d9072b8b3859dab583bbd...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@mlchung) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

Siedlerchr · 2023-08-15T20:49:57Z

/integrate

openjdk · 2023-08-15T20:50:58Z

@Siedlerchr
Your change (at version 42df3f5) is now ready to be sponsored by a Committer.

mlchung · 2023-08-15T22:33:58Z

/sponsor

openjdk · 2023-08-15T22:34:40Z

Going to push as commit bc8e9f4.
Since your change was applied there have been 36 commits pushed to the master branch:

0f5e030: 8309335: Get rid of use of reflection to call Thread.isVirtual() in nsk/jdi/EventRequestManager/stepRequests/stepreq001t.java
f66c73d: 8314166: Update googletest to v1.14.0
f239954: 8310134: NMT: thread count in Thread section of VM.native_memory output confusing with virtual threads
2e8a0ab: 8314120: Add tests for FileDescriptor.sync
80809ef: 8314248: Remove HotSpotConstantPool::isResolvedDynamicInvoke
004651d: 8311557: [JVMCI] deadlock with JVMTI thread suspension
9ded868: 8314114: Fix -Wconversion warnings in os code, primarily linux
a02d65e: 8310308: IR Framework: check for type and size of vector nodes
dff99f7: 8313782: Add user-facing warning if THPs are enabled but cannot be used
f4e72c5: 8313949: Missing word in GPLv2 license text in StackMapTableAttribute.java
... and 26 more: https://git.openjdk.org/jdk/compare/ec0cc6300a02dd92b25d9072b8b3859dab583bbd...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-08-15T22:34:48Z

@mlchung @Siedlerchr Pushed as commit bc8e9f4.

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

Siedlerchr · 2023-08-16T16:35:03Z

@mlchung Thanks again for your help. What would be needed to get the at ec7da91 back ported to JDK21? I guess for the initial release it's too late but maybe for one of the jdk21u releases? That would be really a huge step forward for us at JabRef. (Currently we rely on a custom build)

mlchung · 2023-08-16T18:31:42Z

I created the backport PRs for JDK-8240567 and JDK-8311591.

openjdk/jdk21u#65
openjdk/jdk21u#66

Siedlerchr and others added 11 commits August 9, 2023 20:23

add sample modules

cd978f8

test

595e476

test

3721e9f

Set module split size to 1

274c0ad

fix test

0a33c25

Merge remote-tracking branch 'origin/fix-8311591' into fix-8311591

39b6bb5

* origin/fix-8311591: Set module split size to 1

add some more opens and exports

0500a25

prepare methods for verifying

extract jimage

364e8cf

add decompilation via javap

88d94c6

revert back to default size of 75 for module desriptors

87cd647

use parameter for jlink

rename and cleanup

709ced6

Siedlerchr mentioned this pull request Aug 10, 2023

8240567: MethodTooLargeException thrown while creating a jlink image #14408

Closed

3 tasks

openjdk Bot added the rfr Pull request is ready for review label Aug 10, 2023

openjdk Bot added the core-libs core-libs-dev@openjdk.org label Aug 10, 2023

cleanup

9577e7e

koppor force-pushed the fix-8311591 branch from 9577e7e to 9f51d07 Compare August 10, 2023 22:00

8311591: Add SystemModulesPlugin test case that splits module descrip…

9b9b0bb

…tors with new local variables defined by DedupSetBuilder Co-authored-by: Oliver Kopp <kopp.dev@gmail.com>

koppor force-pushed the fix-8311591 branch from 9f51d07 to 9b9b0bb Compare August 10, 2023 22:07

Siedlerchr added 2 commits August 13, 2023 18:10

Merge remote-tracking branch 'origin/fix-8311591' into fix-8311591

996c187

* origin/fix-8311591: 8311591: Add SystemModulesPlugin test case that splits module descriptors with new local variables defined by DedupSetBuilder

mlchung reviewed Aug 14, 2023

View reviewed changes

Siedlerchr and others added 2 commits August 15, 2023 19:56

Add copyright header and apply some formatting

cf7e804

Revert unnecesary CompilerUtil

Add another required transitive desktop

1a2e463

Assert number of module description generated sub modules Co-authored-by: Oliver Kopp <kopp.dev@gmail.com>

mlchung reviewed Aug 15, 2023

View reviewed changes

Siedlerchr and others added 5 commits August 15, 2023 21:44

Update test/jdk/tools/jlink/dedup/src/m4/p4/Main.java

9e4cc9c

Co-authored-by: Mandy Chung <mandy.chung@oracle.com>

Update test/jdk/tools/jlink/dedup/src/m4/p4/Main.java

7f52bca

Co-authored-by: Mandy Chung <mandy.chung@oracle.com>

Reformat, add missing copyright header

4a7c67a

Remove duplicated module in add mods call Co-authored-by: Oliver Kopp <kopp.dev@gmail.com>

Merge remote-tracking branch 'origin/fix-8311591' into fix-8311591

015a3c2

* origin/fix-8311591: Update test/jdk/tools/jlink/dedup/src/m4/p4/Main.java Update test/jdk/tools/jlink/dedup/src/m4/p4/Main.java

Fix missing import, use size instead of count

2ebc059

mlchung reviewed Aug 15, 2023

View reviewed changes

remove obsolete jimage and decompile methods

2bcdd2f

mlchung reviewed Aug 15, 2023

View reviewed changes

mlchung approved these changes Aug 15, 2023

View reviewed changes

openjdk Bot added the ready Pull request is ready to be integrated label Aug 15, 2023

Siedlerchr added 2 commits August 15, 2023 22:43

remove obsolete constant

94fcd8d

Update full name

42df3f5

openjdk Bot added the sponsor Pull request is ready to be sponsored label Aug 15, 2023

openjdk Bot added the integrated Pull request has been integrated label Aug 15, 2023

openjdk Bot closed this Aug 15, 2023

openjdk Bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Aug 15, 2023

		@@ -0,0 +1,145 @@
		/*
		* Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.

Conversation

Siedlerchr commented Aug 10, 2023 • edited by openjdk Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper Bot commented Aug 10, 2023

Uh oh!

openjdk Bot commented Aug 10, 2023

Uh oh!

openjdk Bot commented Aug 10, 2023

Uh oh!

openjdk Bot commented Aug 10, 2023

Uh oh!

mlbridge Bot commented Aug 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

openjdk Bot commented Aug 10, 2023

Uh oh!

koppor commented Aug 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlchung left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Siedlerchr commented Aug 15, 2023

Uh oh!

mlchung left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Siedlerchr commented Aug 15, 2023

Uh oh!

mlchung left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openjdk Bot commented Aug 15, 2023

Uh oh!

openjdk Bot commented Aug 15, 2023

Uh oh!

Siedlerchr commented Aug 15, 2023

Uh oh!

openjdk Bot commented Aug 15, 2023

Uh oh!

mlchung commented Aug 15, 2023

Uh oh!

openjdk Bot commented Aug 15, 2023

Uh oh!

openjdk Bot commented Aug 15, 2023

Uh oh!

Siedlerchr commented Aug 16, 2023

Uh oh!

mlchung commented Aug 16, 2023

Siedlerchr commented Aug 10, 2023 •

edited by openjdk Bot

Loading

mlbridge Bot commented Aug 10, 2023 •

edited

Loading

koppor commented Aug 10, 2023 •

edited

Loading