RFR: 8319999: Refactor MetaspaceShared::use_full_module_graph() [v2]

[David Holmes]

On Tue, 14 Nov 2023 04:19:37 GMT, Ioi Lam <iklam at openjdk.org> wrote:

>> This is another step of moving CDS config management into cdsConfig.hpp:
>> 
>> The function `MetaspaceShared::use_full_module_graph()` is split into two:
>> - `CDSConfig::is_dumping_full_module_graph()`
>> - `CDSConfig::is_loading_full_module_graph()`
>
> Ioi Lam has updated the pull request incrementally with one additional commit since the last revision:
> 
>   rename FileMapHeader::_use_full_module_graph -> _has_full_module_graph

Seems reasonable but some of the naming seems a little off to me.

Thanks.

src/hotspot/share/cds/cdsConfig.cpp line 34:

> 32: bool CDSConfig::_is_dumping_dynamic_archive = false;
> 33: bool CDSConfig::_enable_dumping_full_module_graph = true;
> 34: bool CDSConfig::_enable_loading_full_module_graph = true;

The "enable" naming isn't quite right - these fields hold the enabled state e.g. `_dumping_full_module_graph_enabled`.

src/hotspot/share/cds/cdsConfig.hpp line 33:

> 31: class CDSConfig : public AllStatic {
> 32: #if INCLUDE_CDS
> 33:   static bool     _is_dumping_dynamic_archive;

Nit: Extra spaces

src/hotspot/share/cds/filemap.cpp line 216:

> 214:   _max_heap_size = MaxHeapSize;
> 215:   _use_optimized_module_handling = MetaspaceShared::use_optimized_module_handling();
> 216:   _has_full_module_graph = CDSConfig::is_dumping_full_module_graph();

`_uses_full_module_graph` would seem more appropriate than `use` or `has`

-------------

PR Review: https://git.openjdk.org/jdk/pull/16646#pullrequestreview-1728817091
PR Review Comment: https://git.openjdk.org/jdk/pull/16646#discussion_r1391969300
PR Review Comment: https://git.openjdk.org/jdk/pull/16646#discussion_r1391969858
PR Review Comment: https://git.openjdk.org/jdk/pull/16646#discussion_r1391971212