On Thu, 26 Oct 2023 21:24:46 GMT, Calvin Cheung <cche...@openjdk.org> wrote:
>> Please review this changeset for adding support for `--module` (-m) option >> for CDS. >> Changes in the `ModuleBootstrap.java` are needed so that the >> `ArchivedModuleGraph.archive` and `ArchivedBootLayer.archive` are called if >> the main module is specified. The module name will be stored in the ro >> region of the CDS archive. During runtime, the archived module name will be >> compared with the runtime module name. If comparison fails, the archived >> full module graph won't be used. >> >> Note: this RFE is a subtask of >> [JDK-8266329](https://bugs.openjdk.org/browse/JDK-8266329). More subtask(s) >> will be created to support other options such as `--add-modules`. >> >> Passed tiers 1 - 4 testing. > > Calvin Cheung has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains ten commits: > > - Merge master > - skip archiving full module graph is there is an incubator module > - fix typo > - simplify some code in modules.cpp > - comments from Alan and Ioi; add missing @run tag in the test > - Merge branch 'master' into improve-CDS-module-graph > - better way to check if a module is a JDK module > - initial review comments from Ioi > - 8316969: Improve CDS module graph support for --module option The new version looks really good now. I found one merge error and have a suggestion for adding one more test. src/hotspot/share/cds/metaspaceShared.cpp line 790: > 788: ArchiveHeapWriter::init(); > 789: if (use_full_module_graph()) { > 790: HeapShared::reset_archived_object_states(CHECK); This block of code needs to be inside `if (CDSConfig::is_dumping_heap())` test/hotspot/jtreg/runtime/cds/appcds/jigsaw/module/ModuleOption.java line 107: > 105: oa.shouldHaveExitValue(0) > 106: // module graph won't be archived with an incubator module > 107: .shouldContain("archivedBootLayer not available, disabling > full module graph"); For thoroughness, I think we should also check for this log in heapShared.cpp: if (record->is_full_module_graph() && !CDSConfig::is_loading_full_module_graph()) { if (log_is_enabled(Info, cds, heap)) { ResourceMark rm(THREAD); log_info(cds, heap)("subgraph %s cannot be used because full module graph is disabled", k->external_name()); } return nullptr; } ------------- Changes requested by iklam (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16016#pullrequestreview-1702150002 PR Review Comment: https://git.openjdk.org/jdk/pull/16016#discussion_r1374801536 PR Review Comment: https://git.openjdk.org/jdk/pull/16016#discussion_r1374805801
