RFR: 8316969: Improve CDS module graph support for --module option [v6]

[Ioi Lam]

On Thu, 26 Oct 2023 21:24:46 GMT, Calvin Cheung <ccheung at 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