On Mon, 12 Jun 2023 05:03:28 GMT, Alan Bateman <al...@openjdk.org> wrote:
>> Oliver Kopp has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains five additional >> commits since the last revision: >> >> - Merge remote-tracking branch 'upstream/master' into fix-8240567 >> - Merge remote-tracking branch 'upstream/master' into fix-8240567 >> - Remove "final" at "enabled" - not part of the point of this PR >> - Remove comments >> - 8240567: MethodTooLargeException thrown while creating a jlink image >> >> Co-authored-by: Christoph Schwentker <siedlerkil...@gmail.com> > > @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. @AlanBateman The diff to the "old" PR https://github.com/openjdk/jdk/pull/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(); } } ------------- PR Comment: https://git.openjdk.org/jdk/pull/14408#issuecomment-1594471485
