On Mon, 21 Apr 2025 18:39:39 GMT, Ioi Lam <ik...@openjdk.org> wrote: >> Calvin Cheung has updated the pull request incrementally with one additional >> commit since the last revision: >> >> @iklam comment > > src/java.base/share/classes/jdk/internal/module/ModuleBootstrap.java line 165: > >> 163: BootLoader.getUnnamedModule(); // trigger <clinit> of >> BootLoader. >> 164: >> CDS.defineArchivedModules(ClassLoaders.platformClassLoader(), >> ClassLoaders.appClassLoader()); >> 165: boolean extraExportsOrOpens = >> addExtraExportsAndOpens(bootLayer); > > (1) The returned value of `addExtraExportsAndOpens()` is not used. So I think > this function can be changed to `void`, and all occurrences of the local > variable `extraExportsOrOpens` can be removed. > > (2) I traced the code paths that depend on the effects of `--add-opens` and > `--add-exports`. It Looks like some of the effects are recorded in the > `java.lang.Module$ReflectionData::export` table: > > https://github.com/openjdk/jdk/blob/684d3b336e9cb31707d35e75f9b785e04e1fdbee/src/java.base/share/classes/java/lang/Module.java#L398C2-L412 > > > /** > * A module (1st key) exports or opens a package to another module > * (2nd key). The map value is a map of package name to a boolean > * that indicates if the package is opened. > */ > static final WeakPairMap<Module, Module, Map<String, Boolean>> > exports = > new WeakPairMap<>(); > > > This table is *not* stored as part of the `ArchivedBootLayer`, so we must > re-initialize this table in the production run. @AlanBateman could you > confirm that this is correct. > > Eventually, we should enhance the `ArchivedBootLayer` to also include the > tables in `Module$ReflectionData`. That will obviate the call to > `addExtraExportsAndOpens()` and save a few bytecodes during start-up (but the > overall impact would be small, so it's not critical in the current PR). > Because these tables use WeakReference, we need to wait for > [JDK-8354897](https://bugs.openjdk.org/browse/JDK-8354897).
Fixed (1) above. > test/hotspot/jtreg/runtime/cds/appcds/jigsaw/modulepath/AddOpens.java line > 109: > >> 107: "--add-opens", addOpensArg, >> 108: "--module-path", moduleDir.toString(), >> 109: "-m", TEST_MODULE1, "with_add_opens") > > Could you add a test case for `=ALL-UNNAMED` as well? That is frequently used > by applications, and it takes a different code path inside the implementation. > > I think this can be done by running `com.simple.Main` in the classpath rather > than module path. Added the test case. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/24695#discussion_r2053082635 PR Review Comment: https://git.openjdk.org/jdk/pull/24695#discussion_r2053082907
