On Thu, 19 Dec 2024 19:14:04 GMT, Henry Jen <henry...@openjdk.org> wrote:
>> This PR split out large array/set construction into separate factory methods >> to avoid oversized method trying to construct several of those. >> >> In order to do that, we will need to generate those help methods on demand >> in the class builder. Here we have two approach, one is for dedup set, which >> is processed in advance so we can know what methods should be created. >> >> Another is for random set, such as packages, thus we put those request into >> a queue to amend the class later. >> >> To keep the optimization of caching built value that are references more >> than once, it was implemented using local vars, which doesn't work well for >> helper methods. The existing approach to populate local vars doesn't work >> well with larger scope of split operation, as the slot was allocated on >> lazily built, but the transfer is captured in advance, this count could >> mismatch as built time and run time. >> >> So we make this build in advance, and use a static array for values referred >> more than once. >> >> All the codegen instead of giving index to be loaded, the builder snippet >> now load the wanted set/array to the operand stack to be consistent. > > Henry Jen 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 16 additional commits since > the last revision: > > - Merge remote-tracking branch 'openjdk/master' into JDK-8321413 > - Javadoc and some minor refactoring > - Move up Snippet setup as a builder > - Merge remote-tracking branch 'openjdk/master' into JDK-8321413 > - Minor cleanup > - Rename based on feedback to emphasis building a snippet for loading a > reference > - Fix typo and comment to match latest implementation > - Fix regression failed to setup helper methods properly > - Separate out ModuleDescriptorBuilder and use LoadableArray for paging > - Merge remote-tracking branch 'openjdk/master' into JDK-8321413 > - ... and 6 more: https://git.openjdk.org/jdk/compare/363ed696...d5a93295 Looks quite good. I only skim on the javadoc/comment this time. It seems okay for implementation use. src/jdk.jlink/share/classes/jdk/tools/jlink/internal/Snippets.java line 96: > 94: > 95: /** > 96: * Describe a operand that can be load onto the operand stack. Suggestion: * Describe an operand that can be load onto the operand stack. src/jdk.jlink/share/classes/jdk/tools/jlink/internal/Snippets.java line 175: > 173: public static abstract class CollectionSnippetBuilder { > 174: /** > 175: * Tested page size of string array Suggestion: * Default page size of string array src/jdk.jlink/share/classes/jdk/tools/jlink/internal/Snippets.java line 180: > 178: > 179: /** > 180: * Tested page size of enum array Suggestion: * Default page size of enum array src/jdk.jlink/share/classes/jdk/tools/jlink/internal/Snippets.java line 190: > 188: > 189: /** > 190: * Arbitary default values based on 15K code size on ~30 bytes > per element Suggestion: * Default threshold based on 15K code size on ~30 bytes per element src/jdk.jlink/share/classes/jdk/tools/jlink/internal/Snippets.java line 227: > 225: */ > 226: public CollectionSnippetBuilder enablePagination(String > methodNamePrefix, int pageSize) { > 227: return enablePagination(methodNamePrefix, pageSize, > Math.min(pageSize, DEFAULT_THRESHOLD)); shouldn't it use the current `activatePagingThreshold` value? Otherwise, this method will override the threshold being set to a non-default value. Same comment for `enablePagination(String methodNamePrefix)` src/jdk.jlink/share/classes/jdk/tools/jlink/internal/Snippets.java line 243: > 241: public CollectionSnippetBuilder disablePagination() { > 242: this.activatePagingThreshold = -1; > 243: this.methodNamePrefix = null; suggest to reset `activatePagingThreshold` and `pageSize` to default value. Set `methodNamePrefix` to null which indicates pagination is disabled. So the constructor should initialize `methodNamePrefix` to null by default. src/jdk.jlink/share/classes/jdk/tools/jlink/internal/Snippets.java line 408: > 406: cob.invokestatic( > 407: ownerClassDesc, > 408: methodNamePrefix + (pageNo + 1), Suggestion: methodNamePrefix + "_" + (pageNo + 1), Suggest to add a character e.g. `_` or `$` to separate the prefix and counter since the prefix may end with a number which is hard to see which part is prefix and which is page number. src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java line 1045: > 1043: * the bytecode to populate that cache. > 1044: */ > 1045: static record DedupSet(Map<Set<String>, Snippet> stringSets, Suggestion: static record DedupSnippets(Map<Set<String>, Snippet> stringSets, src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java line 1101: > 1099: /* > 1100: * Generate bytecode to load a set onto the operand stack. > 1101: * Use cache if the set is references more than once. Suggestion: * Use cache if the set is referenced more than once. src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java line 1216: > 1214: // and is load from the cache array with > 1215: // dedupSetValues[index] > 1216: // Otherwise, LoadableSet will load the set onto the > operand stack. This comment needs update. No more `LoadableSet` src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java line 1239: > 1237: > 1238: /* > 1239: * Facilitate the cache in the class. Return a snippet > to populate the cache in <clinit>. Suggestion: * Returns a snippet that populates the cached values in <clinit>. test/jdk/tools/jlink/JLink20000Packages.java line 109: > 107: JImageGenerator.getJLinkTask() > 108: .modulePath(out) > 109: .output(src.resolve("out-jlink")) Nit: define a variable for the image output directory that can be used in line 114 as well. test/jdk/tools/jlink/SnippetsTest.java line 64: > 62: */ > 63: public class SnippetsTest { > 64: private static final boolean WRITE_CLASS_FILE = > Boolean.parseBoolean(System.getProperty("DumpArraySnippetsTestClasses", > "true")); I assume this is set to true for debug only. Default should be false? ------------- PR Review: https://git.openjdk.org/jdk/pull/21022#pullrequestreview-2518387440 PR Review Comment: https://git.openjdk.org/jdk/pull/21022#discussion_r1894464733 PR Review Comment: https://git.openjdk.org/jdk/pull/21022#discussion_r1894461694 PR Review Comment: https://git.openjdk.org/jdk/pull/21022#discussion_r1894462638 PR Review Comment: https://git.openjdk.org/jdk/pull/21022#discussion_r1894464117 PR Review Comment: https://git.openjdk.org/jdk/pull/21022#discussion_r1894469332 PR Review Comment: https://git.openjdk.org/jdk/pull/21022#discussion_r1894472499 PR Review Comment: https://git.openjdk.org/jdk/pull/21022#discussion_r1894473793 PR Review Comment: https://git.openjdk.org/jdk/pull/21022#discussion_r1894428976 PR Review Comment: https://git.openjdk.org/jdk/pull/21022#discussion_r1894429347 PR Review Comment: https://git.openjdk.org/jdk/pull/21022#discussion_r1894446871 PR Review Comment: https://git.openjdk.org/jdk/pull/21022#discussion_r1894449712 PR Review Comment: https://git.openjdk.org/jdk/pull/21022#discussion_r1894415894 PR Review Comment: https://git.openjdk.org/jdk/pull/21022#discussion_r1894416655
