On Sat, 1 Jul 2023 18:36:10 GMT, Oliver Kopp <d...@openjdk.org> wrote:
>> Fix for [JDK-8240567](https://bugs.openjdk.org/browse/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 https://github.com/openjdk/jdk/pull/10704. GitHub did >> not allow me to re-open the PR, because I did a force-push to have one >> commit. > > Oliver Kopp has updated the pull request incrementally with two additional > commits since the last revision: > > - Fix test > - Fix ArrayList initialization and off-by-one errors Thanks for the update. Some comments below. The test you add does not cause new locals be defined in the helper methods. Do you think you can add such test case i.e. new elements are added in the dedup var list by the helper functions? src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java line 128: > 126: > 127: public SystemModulesPlugin() { > 128: this(75); Configuration is done via the `configure` method. This allows users to tune the batch size by configuring the plugin on the command line. For example: system-modules.argument=batch-size=<N> sets the batch size of module descriptors\n\ \ to avoid exceeding the method length limit. The default\n\ \ batch size is 75. system-modules.usage=\ \ --system-modules [batch-size=<N>]\n\ \ Fast loading of module descriptors (always enabled).\n\ \ The batch size specifies the maximum number of modules\n\ \ be handled in one method to workaround if the generated\n\ \ bytecode exceeds the method size limit. The default\n\ \ batch size is 75. src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java line 543: > 541: private static final int MAX_LOCAL_VARS = 256; > 542: > 543: private final int BUILDER_VAR = MAX_LOCAL_VARS + 1; why this one can't use the next available local variables? i.e. if no splitting, 1 is the next available slot (btw it looks like using 0 in the current implementation is a bug because this is an instance method). If module descriptors are done in multiple helper methods, 3 is the next available slot. src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java line 544: > 542: > 543: private final int BUILDER_VAR = MAX_LOCAL_VARS + 1; > 544: private final int DEDUP_LIST_VAR = MAX_LOCAL_VARS + 2; Same comment as BUILDER_VAR. But this dedup variable list is passed as a parameter to the helper method. It's already on the local slot 2. src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java line 718: > 716: } > 717: > 718: // Instead of adding the module descriptor calls in this > method, What about this? // Split the module descriptors be created by multiple helper methods. // Each helper method "subi" creates the maximum N number of module descriptors // mi, m{i+1} ... // to avoid exceeding the 64kb limit of method length. Then it will call // "sub{i+1}" to creates the next batch of module descriptors m{i+n}, m{i+n+1}... // and so on. During the construction of the module descriptors, the string sets and // modifier sets are deduplicated (see SystemModulesClassGenerator.DedupSetBuilder) // and cached in the locals. These locals are saved in an array list so // that the helper method can restore the local variables that may be // referenced by the bytecode generated for creating module descriptors. // Pseudo code looks like this: // // void subi(ModuleDescriptor[] mdescs, ArrayList<Object> localvars) { // // assign localvars to local variables // var l3 = localvars.get(0); // var l4 = localvars.get(1); // : // // fill mdescs[i] to mdescs[i+n-1] // mdescs[i] = ... // mdescs[i+1] = ... // : // // save new local variables added // localvars.add(lx) // localvars.add(l{x+1}) // : // sub{i+i}(mdescs, localvars); // } src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java line 745: > 743: ClassDesc arrayListClassDesc = > ClassDesc.ofInternalName("java/util/ArrayList"); > 744: > 745: int firstVar = nextLocalVar; Suggestion: int dedupVarStart = nextLocalVar; src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java line 746: > 744: > 745: int firstVar = nextLocalVar; > 746: var wrapper = new Object() { I think you can simply use a local variable at the beginning of each iteration. See below. src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java line 777: > 775: for (int n = 0, count = 0; n < splitModuleInfos.size(); > count += splitModuleInfos.get(n).size(), n++) { > 776: int index = n; // the index of which ModuleInfo > being processed in the current batch > 777: int start = count; // the start index to the return > ModuleDescriptor array for the current batch add `int curDedupVar = nextLocalVar;` src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java line 784: > 782: cob -> { > 783: cob.aload(2) > 784: .astore(DEDUP_LIST_VAR); I think you can keep slot 2 for the dedup var list by changing the start of `nextLocalVar` to 3? Then you don't need to reassign it to another local. src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java line 787: > 785: > 786: if (nextLocalVar > firstVar) { > 787: for (int i = firstVar; i < nextLocalVar; > i++) { Suggestion: for (int i = firstLocalVar; i < curLocalVar; i++) { src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java line 787: > 785: > 786: if (nextLocalVar > firstVar) { > 787: for (int i = firstVar; i < nextLocalVar; > i++) { Suggestion: if (curLocalVar > firstLocalVar) { for (int i = firstLocalVar; i < curLocalVar; i++) { src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java line 806: > 804: if (index < splitModuleInfos.size() - 1) { > 805: if ((nextLocalVar > firstVar) && > (nextLocalVar > wrapper.lastCopiedVar)) { > 806: for (int i = wrapper.lastCopiedVar + > 1; i < nextLocalVar; i++) { shouldn't it start with `wrapper.lastCopiedVar`? Suggestion: if (index < splitModuleInfos.size() - 1) { if (nextLocalVar > firstLocalVar && nextLocalVar > curLocalVar) { for (int i = curLocalVar; i < nextLocalVar; i++) { ------------- PR Review: https://git.openjdk.org/jdk/pull/14408#pullrequestreview-1511699439 PR Review Comment: https://git.openjdk.org/jdk/pull/14408#discussion_r1251228567 PR Review Comment: https://git.openjdk.org/jdk/pull/14408#discussion_r1251239488 PR Review Comment: https://git.openjdk.org/jdk/pull/14408#discussion_r1251240283 PR Review Comment: https://git.openjdk.org/jdk/pull/14408#discussion_r1251241479 PR Review Comment: https://git.openjdk.org/jdk/pull/14408#discussion_r1251243630 PR Review Comment: https://git.openjdk.org/jdk/pull/14408#discussion_r1251242653 PR Review Comment: https://git.openjdk.org/jdk/pull/14408#discussion_r1251244124 PR Review Comment: https://git.openjdk.org/jdk/pull/14408#discussion_r1251247256 PR Review Comment: https://git.openjdk.org/jdk/pull/14408#discussion_r1251249679 PR Review Comment: https://git.openjdk.org/jdk/pull/14408#discussion_r1251250488 PR Review Comment: https://git.openjdk.org/jdk/pull/14408#discussion_r1251251439
