On Fri, 27 Oct 2023 16:28:10 GMT, Ioi Lam <ik...@openjdk.org> wrote:
>> 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
>
> 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())`
Thanks for catching this. Fixed.
Also moved the `HeapShared::init_for_dumping(CHECK)` inside the "if" block.
> 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;
> }
For this case, the record is null. Per our discussion, I've added another log
for the null record case:
```
968 if (record == nullptr) {
969 if (log_is_enabled(Info, cds, heap)) {
970 ResourceMark rm(THREAD);
971 log_info(cds, heap)("subgraph %s is not recorded",
972 k->external_name());
973 }
and updated the test accordingly.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/16016#discussion_r1377097395
PR Review Comment: https://git.openjdk.org/jdk/pull/16016#discussion_r1377097436
