8240567: MethodTooLargeException thrown while creating a jlink image #14408
Conversation
|
👋 Welcome back koppor! A progress list of the required criteria for merging this PR into |
koppor
changed the title
Co-authored-by: Christoph Schwentker <siedlerkiller@gmail.com>
|
We validated this by compiling JabRef against it and a) writing out the generated class file for the generated bytecode and b) verifying with -Xlog:init=debug -XX:+UnlockDiagnosticVMOptions -XX:+BytecodeVerificationLocal Is there a way to "parse" or call the verifyer after generation? |
Webrevs
|
There was a problem hiding this comment.
@koppor Is this ready for review? The other PR went through a dozens or so iterations before it was returned to draft. It seems like you were still battling with verifier errors. The comment on this PR says you it was created because a force-push so I can't tell if you the changes are ready or not.
|
|
||
| // Restore all (!) sets from parameter to local variables | ||
| if (nextLocalVar > firstVariableForDedup) { | ||
| // We need to go from the end to the beginning as we will probably overwrite position 2 (which holds the list at the beginning) |
There was a problem hiding this comment.
Do you mind fixing all the comments as these 160+ lines make it impossible to look at the changes side-by-side again. You had fixed that in the original PR but it looks like they have come back with this PR.
There was a problem hiding this comment.
Do you mind fixing all the comments as these 160+ lines
Done. I copied the (for me) important comments to the GitHub reply #14408 (comment).
You had fixed that in the original PR but it looks like they have come back with this PR.
This is my job training 😇: Comment the core ideas (and do not verbatim repeat what is said by the code directly). - For me, the good thing is, the diff now has all the comments (23bbc0c). OK, at least one has go to nirvana in all cases.
There was a problem hiding this comment.
Yes, the verify errors are gone. Application (JabRef) starts sucessfully against the compilation. Is there a way to verify in a tesr the creation of the helper methos in the bytecode? e.g. something like getDeclaredMethods?
|
@koppor 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. |
|
TL;DR: summary of the changes
Yes, it is! We especially checked it with our (rather large) desktop application.
I remember 😅. Therefore, we first had this as draft, waited for the "on-site checks" and then opened it.
We missed an important point and the other PR.
I am sorry for that. I weighted the requirement to have a single commit higher than reviewing the changes. The "excuse" for this decision is that nearly all code submitted back than has changed. |
|
@asotona At https://bugs.openjdk.org/browse/JDK-8240567?focusedCommentId=14588425&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14588425 you asked the following:
I don't have a bugs.openjdk.org account; thus I reply here. The constant pool is also an "interesting" limitation. We did not address it (yet) because the current patch is IMHO a major step forward: It unblocks feature development of our software: We can add new external libraries 🎉 Looking more close to that limit and the implications to the patch: I understand that this limit affects the number of methods in a class. If this is correct, let's assume that a project requires thousands of modules and would use half of the limit for the generation of the module descriptors. That would result in roughly 32,000 methods. The patch puts 50 module descriptors in one method. Assuming 32,000 methods leads to ~1.6 million modules for a Java program. Currently, our project JabRef has a total number of ~130 modules. We cannot move forward with our release as we cannot add new dependencies. We hope that other projects with many modules will benefit from this patch as well. |
|
@AlanBateman The diff to the "old" PR #10704 is as follows. Maybe this helps at reviewing? IMHO this diff shows very good the new passing of the locals required by the deduplication functionality. diff --git a/src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java b/src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java
old mode 100755
new mode 100644
index 8e98a5b94e2..a87fa84ebe2
--- a/src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java
+++ b/src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java
@@ -118,7 +118,7 @@ public final class SystemModulesPlugin extends AbstractPlugin {
ClassDesc.ofInternalName("jdk/internal/module/SystemModules");
private static final ClassDesc CD_SYSTEM_MODULES_MAP =
ClassDesc.ofInternalName(SYSTEM_MODULES_MAP_CLASSNAME);
- private final boolean enabled;
+ private boolean enabled;
public SystemModulesPlugin() {
super("system-modules");
@@ -519,7 +519,7 @@ public final class SystemModulesPlugin extends AbstractPlugin {
private static final int MAX_LOCAL_VARS = 256;
- private final int BUILDER_VAR = 0;
+ private final int BUILDER_VAR = MAX_LOCAL_VARS + 1;
private final int MD_VAR = 1; // variable for ModuleDescriptor
private final int MT_VAR = 1; // variable for ModuleTarget
private final int MH_VAR = 1; // variable for ModuleHashes
@@ -666,7 +666,7 @@ public final class SystemModulesPlugin extends AbstractPlugin {
* Generate bytecode for moduleDescriptors method
*/
private void genModuleDescriptorsMethod(ClassBuilder clb) {
- if (moduleInfos.size() <= 71) {
+ if (moduleInfos.size() <= 75) {
clb.withMethodBody(
"moduleDescriptors",
MethodTypeDesc.of(CD_MODULE_DESCRIPTOR.arrayType()),
@@ -688,71 +688,98 @@ public final class SystemModulesPlugin extends AbstractPlugin {
return;
}
- // split up module infos in "consumable" packages
List<List<ModuleInfo>> splitModuleInfos = new ArrayList<>();
List<ModuleInfo> currentModuleInfos = null;
for (int index = 0; index < moduleInfos.size(); index++) {
- // The method is "manually split" based on the heuristics that 90 ModuleDescriptors are smaller than 64kb
- // The number 10 is chosen "randomly" to be below the 64kb limit of a method
- if (index % 10 == 0) {
- // Prepare new list
+ if (index % 50 == 0) {
currentModuleInfos = new ArrayList<>();
splitModuleInfos.add(currentModuleInfos);
}
currentModuleInfos.add(moduleInfos.get(index));
}
- // generate all helper methods
+
final String helperMethodNamePrefix = "moduleDescriptorsSub";
+ final ClassDesc arrayListClassDesc = ClassDesc.ofInternalName("java/util/ArrayList");
+
+ final int firstVariableForDedup = nextLocalVar;
+
+ clb.withMethodBody(
+ "moduleDescriptors",
+ MethodTypeDesc.of(CD_MODULE_DESCRIPTOR.arrayType()),
+ ACC_PUBLIC,
+ cob -> {
+ cob.constantInstruction(moduleInfos.size())
+ .anewarray(CD_MODULE_DESCRIPTOR)
+ .dup()
+ .astore(MD_VAR);
+ cob.new_(arrayListClassDesc)
+ .dup()
+ .invokespecial(arrayListClassDesc, "<init>", MethodTypeDesc.of(CD_void))
+ .astore(nextLocalVar);
+ cob.aload(0)
+ .aload(MD_VAR)
+ .aload(nextLocalVar)
+ .invokevirtual(
+ this.classDesc,
+ helperMethodNamePrefix + "0",
+ MethodTypeDesc.of(CD_void, CD_MODULE_DESCRIPTOR.arrayType(), arrayListClassDesc)
+ )
+ .areturn();
+ });
+
final int[] globalCount = {0};
for (final int[] index = {0}; index[0] < splitModuleInfos.size(); index[0]++) {
clb.withMethodBody(
helperMethodNamePrefix + index[0],
- MethodTypeDesc.of(CD_void, CD_MODULE_DESCRIPTOR.arrayType()),
+ MethodTypeDesc.of(CD_void, CD_MODULE_DESCRIPTOR.arrayType(), arrayListClassDesc),
ACC_PUBLIC,
cob -> {
List<ModuleInfo> moduleInfosPackage = splitModuleInfos.get(index[0]);
- if (index[0] > 0) {
- // call last helper method
- cob.aload(0)
- .aload(MD_VAR) // load first parameter, which is MD_VAR
- .invokevirtual(
- this.classDesc,
- helperMethodNamePrefix + (index[0] - 1),
- MethodTypeDesc.of(CD_void, CD_MODULE_DESCRIPTOR.arrayType())
- );
+
+ if (nextLocalVar > firstVariableForDedup) {
+ for (int i = nextLocalVar-1; i >= firstVariableForDedup; i--) {
+ cob.aload(2)
+ .constantInstruction(i-firstVariableForDedup)
+ .invokevirtual(arrayListClassDesc, "get", MethodTypeDesc.of(CD_Object, CD_int))
+ .astore(i);
+ }
}
+
for (int j = 0; j < moduleInfosPackage.size(); j++) {
ModuleInfo minfo = moduleInfosPackage.get(j);
- // executed after the call, thus it is OK to overwrite index 0 (BUILDER_VAR)
new ModuleDescriptorBuilder(cob,
minfo.descriptor(),
minfo.packages(),
globalCount[0]).build();
globalCount[0]++;
}
+
+ if (index[0] + 1 < (splitModuleInfos.size())) {
+ if (nextLocalVar > firstVariableForDedup) {
+ cob.new_(arrayListClassDesc)
+ .dup()
+ .invokespecial(arrayListClassDesc, "<init>", MethodTypeDesc.of(CD_void))
+ .astore(nextLocalVar);
+ for (int i = firstVariableForDedup; i < nextLocalVar; i++) {
+ cob.aload(nextLocalVar)
+ .aload(i)
+ .invokevirtual(arrayListClassDesc, "add", MethodTypeDesc.of(CD_boolean, CD_Object))
+ .pop();
+ }
+ }
+ cob.aload(0)
+ .aload(MD_VAR)
+ .aload(nextLocalVar)
+ .invokevirtual(
+ this.classDesc,
+ helperMethodNamePrefix + (index[0] + 1),
+ MethodTypeDesc.of(CD_void, CD_MODULE_DESCRIPTOR.arrayType(), arrayListClassDesc)
+ );
+ }
+
cob.return_();
});
}
-
- // generate call to last helper method
- clb.withMethodBody(
- "moduleDescriptors",
- MethodTypeDesc.of(CD_MODULE_DESCRIPTOR.arrayType()),
- ACC_PUBLIC,
- cob -> {
- cob.constantInstruction(moduleInfos.size())
- .anewarray(CD_MODULE_DESCRIPTOR)
- .astore(MD_VAR)
- .aload(MD_VAR) // storing for the return at the end
- .aload(0)
- .aload(MD_VAR)
- .invokevirtual(
- this.classDesc,
- helperMethodNamePrefix + (splitModuleInfos.size() - 1),
- MethodTypeDesc.of(CD_void, CD_MODULE_DESCRIPTOR.arrayType())
- )
- .areturn();
- });
}
/**
diff --git a/test/jdk/tools/jlink/JLink100Modules.java b/test/jdk/tools/jlink/JLink100Modules.java
index 4a6111505d2..e71206c904f 100644
--- a/test/jdk/tools/jlink/JLink100Modules.java
+++ b/test/jdk/tools/jlink/JLink100Modules.java
@@ -27,8 +27,10 @@ import java.nio.file.Paths;
import java.util.Arrays;
import java.util.StringJoiner;
import java.util.spi.ToolProvider;
+
import tests.JImageGenerator;
import tests.JImageGenerator.JLinkTask;
+
/*
* @test
* @summary Make sure that 100 modules can be linked using jlink.
@@ -46,9 +48,9 @@ import tests.JImageGenerator.JLinkTask;
*/
public class JLink100Modules {
private static final ToolProvider JAVAC_TOOL = ToolProvider.findFirst("javac")
- .orElseThrow(() -> new RuntimeException("javac tool not found"));
+ .orElseThrow(() -> new RuntimeException("javac tool not found"));
private static final ToolProvider JLINK_TOOL = ToolProvider.findFirst("jlink")
- .orElseThrow(() -> new RuntimeException("jlink tool not found"));
+ .orElseThrow(() -> new RuntimeException("jlink tool not found"));
static void report(String command, String[] args) {
System.out.println(command + " " + String.join(" ", Arrays.asList(args)));
@@ -77,9 +79,10 @@ public class JLink100Modules {
StringBuilder builder = new StringBuilder("module ");
builder.append(name).append(" {");
- for (int j = 0; j < i; j++) {
- builder.append("requires module" + j + "x;");
+ if (i != 0) {
+ builder.append("requires module0x;");
}
+
builder.append("}\n");
Files.writeString(moduleDir.resolve("module-info.java"), builder.toString());
mainModuleInfoContent.add(name);
@@ -106,7 +109,7 @@ public class JLink100Modules {
String out = src.resolve("out").toString();
- javac(new String[] {
+ javac(new String[]{
"-d", out,
"--module-source-path", src.toString(),
"--module", "bug8240567x"
@@ -116,6 +119,7 @@ public class JLink100Modules {
.modulePath(out)
.output(src.resolve("out-jlink"))
.addMods("bug8240567x")
- .call().assertSuccess();
+ .call()
+ .assertSuccess();
}
} |
It's good that you've got to a patch that doesn't trip the verifier. It is one my list to look at the latest version. |
| List<List<ModuleInfo>> splitModuleInfos = new ArrayList<>(); | ||
| List<ModuleInfo> currentModuleInfos = null; | ||
| for (int index = 0; index < moduleInfos.size(); index++) { | ||
| if (index % 50 == 0) { |
There was a problem hiding this comment.
The size of the generated moduleDescriptors is a function of the number of modules, the number of packages in the modules, number of exports, ... I think your approach is okay for now, meaning pick some threshold that you are confident will result in a method size < 64k. If the number of modules generate a chained methods to popular the array. It might be clearer to readers to use one threshold rather than two.
The other limit that we will need to think about is the constant pool. That may be future work.
There was a problem hiding this comment.
The size of the generated moduleDescriptors is a function of the number of modules, the number of packages in the modules, number of exports, ... I think your approach is okay for now, meaning pick some threshold that you are confident will result in a method size < 64k.
I am very confident with 50 as 1) we tried out with a real life application using a high variation of dependencies (covering Apache libraries, Google libraries, and Oracle's JavaFX) and 2) it is half of the value of the number of modules where it broke at our software.
It might be clearer to readers to use one threshold rather than two.
Above, the 75 was chosen, to avoid that anything in the JDK distribution is split (number was 73 to be precise, but I did want to have a "round" number).
To be consistent, I adapted the lower split to 75. I tried it out with our app, and the app still works.
The other limit that we will need to think about is the constant pool. That may be future work.
I am leaning towards "future work" here. Nevertheless, could you outline how that limit might be reached? It IMHO cannot be the numbmer of helper functions. The current "limit" leads to 32,000 (constant pool limit; leaving the other 32k to other things) x 75 (number of module descriptors covered) = 2.4 million module descriptors. In that case we might hit the call stack limit.
| * @bug 8240567 | ||
| * @library ../lib | ||
| * @modules java.base/jdk.internal.jimage | ||
| * jdk.jdeps/com.sun.tools.classfile |
There was a problem hiding this comment.
I guess you copied this from other jlink tests. Do you know why jlink tests need com.sun.tools.classfile?
There was a problem hiding this comment.
If it is not present, I get following output (sorry for German - ./configure doesn't allow JAVA_TOOL_OPTIONS to be set to -Duser.language=en
windir='C:\WINDOWS' \
'c:\git-repositories\jdk\jdk\build\windows-x86_64-server-release\images\jdk\bin\javac' \
-J-Xmx768m \
-J-XX:MaxRAMPercentage=2.08333 \
-J'-Dtest.boot.jdk=c:\progra~1\openjdk\jdk-20~1.1' \
-J'-Djava.io.tmpdir=c:\git-repositories\jdk\jdk\build\windows-x86_64-server-release\test-support\jtreg_test_jdk_tools_jlink_JLink100Modules_java\tmp' \
-J-ea \
-J-esa \
-J-Dtest.vm.opts='-Xmx768m -XX:MaxRAMPercentage=2.08333 -Dtest.boot.jdk=c:\progra~1\openjdk\jdk-20~1.1 -Djava.io.tmpdir=c:\git-repositories\jdk\jdk\build\windows-x86_64-server-release\test-support\jtreg_test_jdk_tools_jlink_JLink100Modules_java\tmp -ea -esa' \
-J-Dtest.tool.vm.opts='-J-Xmx768m -J-XX:MaxRAMPercentage=2.08333 -J-Dtest.boot.jdk=c:\progra~1\openjdk\jdk-20~1.1 -J-Djava.io.tmpdir=c:\git-repositories\jdk\jdk\build\windows-x86_64-server-release\test-support\jtreg_test_jdk_tools_jlink_JLink100Modules_java\tmp -J-ea -J-esa' \
-J-Dtest.compiler.opts= \
-J-Dtest.java.opts= \
-J-Dtest.jdk='c:\git-repositories\jdk\jdk\build\windows-x86_64-server-release\images\jdk' \
-J-Dcompile.jdk='c:\git-repositories\jdk\jdk\build\windows-x86_64-server-release\images\jdk' \
-J-Dtest.timeout.factor=4.0 \
-J-Dtest.nativepath='c:\git-repositories\jdk\jdk\build\windows-x86_64-server-release\images\test\jdk\jtreg\native' \
-J-Dtest.root='C:\git-repositories\jdk\jdk\test\jdk' \
-J-Dtest.name=tools/jlink/JLink100Modules.java \
-J-Dtest.file='C:\git-repositories\jdk\jdk\test\jdk\tools\jlink\JLink100Modules.java' \
-J-Dtest.src='C:\git-repositories\jdk\jdk\test\jdk\tools\jlink' \
-J-Dtest.src.path='C:\git-repositories\jdk\jdk\test\jdk\tools\jlink;C:\git-repositories\jdk\jdk\test\jdk\tools\lib' \
-J-Dtest.classes='C:\git-repositories\jdk\jdk\build\windows-x86_64-server-release\test-support\jtreg_test_jdk_tools_jlink_JLink100Modules_java\classes\0\tools\jlink\JLink100Modules.d' \
-J-Dtest.class.path='C:\git-repositories\jdk\jdk\build\windows-x86_64-server-release\test-support\jtreg_test_jdk_tools_jlink_JLink100Modules_java\classes\0\tools\jlink\JLink100Modules.d;C:\git-repositories\jdk\jdk\build\windows-x86_64-server-release\test-support\jtreg_test_jdk_tools_jlink_JLink100Modules_java\classes\0\tools\lib' \
-J-Dtest.class.path.prefix='C:\git-repositories\jdk\jdk\test\jdk\tools\jlink;C:\git-repositories\jdk\jdk\build\windows-x86_64-server-release\test-support\jtreg_test_jdk_tools_jlink_JLink100Modules_java\classes\0\tools\lib' \
-J-Dtest.modules='java.base/jdk.internal.jimage jdk.jlink/jdk.tools.jlink.internal jdk.jlink/jdk.tools.jlink.plugin jdk.jlink/jdk.tools.jmod jdk.jlink/jdk.tools.jimage jdk.compiler' \
--add-modules java.base,jdk.jlink,jdk.compiler \
--add-exports java.base/jdk.internal.jimage=ALL-UNNAMED \
--add-exports jdk.jlink/jdk.tools.jlink.internal=ALL-UNNAMED \
--add-exports jdk.jlink/jdk.tools.jlink.plugin=ALL-UNNAMED \
--add-exports jdk.jlink/jdk.tools.jmod=ALL-UNNAMED \
--add-exports jdk.jlink/jdk.tools.jimage=ALL-UNNAMED \
-d 'C:\git-repositories\jdk\jdk\build\windows-x86_64-server-release\test-support\jtreg_test_jdk_tools_jlink_JLink100Modules_java\classes\0\tools\lib' \
-sourcepath 'C:\git-repositories\jdk\jdk\test\jdk\tools\lib' \
-classpath 'C:\git-repositories\jdk\jdk\build\windows-x86_64-server-release\test-support\jtreg_test_jdk_tools_jlink_JLink100Modules_java\classes\0\tools\lib' 'C:\git-repositories\jdk\jdk\test\jdk\tools\lib\tests\Helper.java' 'C:\git-repositories\jdk\jdk\test\jdk\tools\lib\tests\JImageGenerator.java' 'C:\git-repositories\jdk\jdk\test\jdk\tools\lib\tests\JImageValidator.java' 'C:\git-repositories\jdk\jdk\test\jdk\tools\lib\tests\Result.java'
direct:
C:\git-repositories\jdk\jdk\test\jdk\tools\lib\tests\JImageValidator.java:36: Fehler: Package com.sun.tools.classfile ist nicht sichtbar
import com.sun.tools.classfile.ClassFile;
^
(Package com.sun.tools.classfile wird in Modul jdk.jdeps deklariert, aber nicht in das unbenannte Modul exportiert)
C:\git-repositories\jdk\jdk\test\jdk\tools\lib\tests\JImageValidator.java:37: Fehler: Package com.sun.tools.classfile ist nicht sichtbar
import com.sun.tools.classfile.ConstantPoolException;
^
(Package com.sun.tools.classfile wird in Modul jdk.jdeps deklariert, aber nicht in das unbenannte Modul exportiert)
2 Fehler
There was a problem hiding this comment.
I see now. It's used by JImageValidator and that can be replaced by the ClassFile API in the future. Thanks.
| * jdk.jlink/jdk.tools.jimage | ||
| * jdk.compiler | ||
| * @build tests.* | ||
| * @run main/othervm -verbose:gc -Xmx1g -Xlog:init=debug -XX:+UnlockDiagnosticVMOptions -XX:+BytecodeVerificationLocal JLink100Modules |
There was a problem hiding this comment.
This patch does not change the bytecode if it's less than 75 modules. You want to apply -XX:+UnlockDiagnosticVMOptions -XX:+BytecodeVerificationLocal flags also to a custom image with the new bytecodes. So this test should include an execution of JLink100ModulesTest from out-jlink image with these flags.
There was a problem hiding this comment.
BTW, is -verbose:gc leftover from debugging?
| @@ -666,25 +666,120 @@ private void genIncubatorModules(ClassBuilder clb) { | |||
| * Generate bytecode for moduleDescriptors method | |||
| */ | |||
| private void genModuleDescriptorsMethod(ClassBuilder clb) { | |||
| if (moduleInfos.size() <= 75) { | |||
There was a problem hiding this comment.
The plugin can take an optional argument to configure the max number of module infos to split in case it hits the limit in a future case.
There was a problem hiding this comment.
What I meant here is to change the configure method to allow the plugin to take an argument for example
--system-modules batch-size=100
This argument is optional. If not specified, the default value is 75. This way you can tune the batch-size if we hit the method size limit for whatever reason. And the test can also verify with different size if appropriate.
There was a problem hiding this comment.
plugins.properties needs to be updated to show the option added for this plugin.
BTW, the current argument described in the usage is out-dated which needs update.
system-modules.argument=retainModuleTarget
There was a problem hiding this comment.
Added it at 9835a64. This is not for the main ./configure. To which configure does that go?
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java
Outdated
Show resolved
Hide resolved
| return; | ||
| } | ||
|
|
||
| List<List<ModuleInfo>> splitModuleInfos = new ArrayList<>(); |
There was a problem hiding this comment.
Please add the comments to describe what this does. Pseudo code may also help. It'd be helpful to explain the reason why the Sub method stores and restores what local variables.
I also wonder if you consider moduleDescriptors does this:
ArrayList<Object> localVars = new ArrayList<>();
moduleDescriptorsSub0(moduleInfos, localVars);
.... // add new local variables to localVars
moduleDescriptorsSub1(moduleInfos, localVars);
.... // add new local variables to localVars
:
:
The same localVars array list is used and just add the new local variables defined from each batch (no need to create a new ArrayList and stores local variables every time)
There was a problem hiding this comment.
I assume the "old" comments were too detailed. I removed them at JabRef/jdk@23bbc0c (#2) to have the code reviewable. I can readd some of them if that helps.
I also added a compressed description of the idea at edd85c9 (#14408). Update here, in the PR one simply needs to scroll up and sees the new comment.
There was a problem hiding this comment.
Have you considered passing the same ArrayList for saving and restoring local variables? Currently each method creates one new array list and save the local variables from firstVariableForDedup to nextLocalVar, but the local variables from firstVariableForDedup are already added to the array list created by the previous method.
There was a problem hiding this comment.
Ah, I see your commit. I will check it out.
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java
Outdated
Show resolved
Hide resolved
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java
Outdated
Show resolved
Hide resolved
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java
Outdated
Show resolved
Hide resolved
…ethod (was magic number 50 / 75 before)
Thank you for the feedback! I didn't see the "proper" usage of effectively final variables.
I modified the "end-to-end" test in Note that the numbers cannot be arbitrarily increased. Otherwise, one gets errors similar to following |
There was a problem hiding this comment.
It's looking pretty good.
About the test, I don't see ArrayList::add in the generated bytecode of sub2-13. The dedup string set is used for the targets of qualified exports and opens and uses. The modifiers set of requires is deduplicated but not the required module names.
I assume you want this be backported. If so, we should give it some baked time after it's integrated to the main line. I'm okay if you want to extend the test in a follow up. You can extract the generated class to verify:
$ jimage extract --dir=dir out-jlink/lib/modules
$ javap -p -v dir/java.base/jdk/internal/module/SystemModules\$all.class
| Path binDir = src.resolve("out-jlink").resolve("bin").toAbsolutePath(); | ||
| Path bin = binDir.resolve("java"); | ||
|
|
||
| ProcessBuilder processBuilder = new ProcessBuilder(bin.toString(), "-XX:+UnlockDiagnosticVMOptions", "-XX:+BytecodeVerificationLocal", "-m", "bug8240567x/testpackage.JLink100ModulesTest"); |
There was a problem hiding this comment.
please wrap this long line into multiple lines.
Co-authored-by: Mandy Chung <mandy.chung@oracle.com>
Thank you!
Thank you for the hint.
Sounds great!
That would be great. Will take time to craft a proper setting. |
| // feed SystemModulesPlugin.SystemModulesClassGenerator.DedupSetBuilder | ||
| for (int j = 0; j < i % 20; j++) { | ||
| moduleInfoContent.append(" requires module" + j + "x;\n"); | ||
| } |
There was a problem hiding this comment.
Do you want to keep this change? or at least take out the comment since this doesn't feed DedupSetBuilder.
Can you add a comment about "the numbers cannot be arbitrarily increased".
There was a problem hiding this comment.
I reverted at 8494517 (#14408). Think, the comment is then not necessary?
|
@koppor 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: 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 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 (@AlanBateman, @mlchung) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
|
/integrate |
|
I created https://bugs.openjdk.org/browse/JDK-8311591. This issue should not be backport until JDK-8311591 is resolved. |
|
/sponsor |
|
Going to push as commit ec7da91.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@koppor do you have any update on a new test for JDK-8311591? |
Currently on a business trip. Will resume work on this next week the latest. |
Fix for JDK-8240567: "MethodTooLargeException thrown while creating a jlink image".
Java still has a 64kb limit: A method may not be longer than 64kb. The idea of the fix is to split up the generated methods in several smaller methods
This is a follow-up to #10704. GitHub did not allow me to re-open the PR, because I did a force-push to have one commit.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/14408/head:pull/14408$ git checkout pull/14408Update a local copy of the PR:
$ git checkout pull/14408$ git pull https://git.openjdk.org/jdk.git pull/14408/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 14408View PR using the GUI difftool:
$ git pr show -t 14408Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/14408.diff
Webrev
Link to Webrev Comment