On Thu, 15 May 2025 11:47:07 GMT, Joel Sikström <jsiks...@openjdk.org> wrote:
>> Hello,
>>
>> The goal of this RFE is to separate Metaspace printing from GC printing. The
>> main reason Metaspace and GC printing is coupled the way it is right now is
>> because historically, the permanent generation (PermGen), which was replaced
>> by Metaspace, was part of the GC heap. Hence, it made sense to also print
>> info about the PermGen when printing the GC heap.
>>
>> With Metaspace replacing the PermGen, which uses memory that is separate
>> from the GC heap, the coupling has become more loose, raising the question
>> if Metaspace should be printed somewhere else (maybe when printing *other*
>> Metaspace stuff?). A reason to still print Metaspace when printing the heap
>> is that the GC is responsible for unloading classes and nmethods, which
>> means it makes sense to print Metaspace information in connection to when a
>> GC is performed.
>>
>> To better reflect the current state of the JVM, I propse we make the
>> following changes to separate Metaspace from GC printing:
>> * Move Metaspace printing from HeapInfoDCmd to MetaspaceDCmd.
>> * Move Metaspace printing from the "Heap:" section to "Metaspace:" section
>> in vmError.cpp (hs_err files, the VM.info jcmd and -XX:+PrintVMInfoAtExit).
>> * Use gc+exit instead of gc+heap+exit as tags for the LogTarget during exit
>> printing to reflect that it's not only the heap being printed.
>> * And the largest change in terms of LOC, separate Metaspace and GC Heap
>> prints in the before/after GC invocation(s) printing. This is also recorded
>> in a ring buffer, which is printed in vmError.cpp.
>>
>> Testing:
>> * GHA, Oracle's tier 1-4
>> * Manuel inspection of printed content
>
> Joel Sikström has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Update new order in tests
Metaspace changes (the few) seem fine. GC changes too.
Minor remarks inline
src/hotspot/share/memory/metaspace/metaspaceDCmd.cpp line 62:
> 60: void MetaspaceDCmd::execute(DCmdSource source, TRAPS) {
> 61: MetaspaceUtils::print_on(output());
> 62:
Okay, though arguably somewhat redundant with the following output
test/hotspot/jtreg/serviceability/dcmd/gc/HeapInfoTest.java line 48:
> 46: OutputAnalyzer output = executor.execute(cmd);
> 47: output.shouldNotContain("Unknown diagnostic command");
> 48: output.shouldHaveExitValue(0);
This was already kind of weak before and is almost useless now :) can we
improve on that? A command reporting back nothing would now result in a green
test?
-------------
PR Review: https://git.openjdk.org/jdk/pull/25214#pullrequestreview-2860244469
PR Review Comment: https://git.openjdk.org/jdk/pull/25214#discussion_r2101938786
PR Review Comment: https://git.openjdk.org/jdk/pull/25214#discussion_r2101933679
