RFR: 8281946: VM.native_memory should report size of shareable memory [v7]

[Thomas Stuefe]

On Tue, 6 Dec 2022 21:11:28 GMT, Matias Saavedra Silva <matsaave at openjdk.org> wrote:

>> The native memory reporting has been improved to also include the amount of shareable (read only) data. Verified with test tiers 1,2, and 4.
>> 
>> Sample output:
>> 
>> Total: reserved=33890498670, committed=2360907886
>>        malloc: 75159662 #26118
>>        mmap:   reserved=33815339008, committed=2285748224
>> .............
>> -        Shared class space (reserved=16777216, committed=13545472, readonly=8315472)
>>                             (mmap: reserved=16777216, committed=13545472) 
>>  
>> -               Arena Chunk (reserved=441792, committed=441792)
>>                             (malloc=441792)
>
> Matias Saavedra Silva has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fixed mistake during merging

Hi Matias,

see remarks inline. Since the goal is only printing readonly on the "Shared class" line, the patch can be simplified. I am concerned with keeping the patch as simple as possible because reporting code is already quite complex and brittle. I'd like to make future revamps as easy as possible.

Cheers, Thomas

src/hotspot/share/services/memReporter.cpp line 42:

> 40: }
> 41: 
> 42: // There can be upto two CDS archives which can contain readonly data. On Windows, pages are not

MemReporterBase::readonly_total is CDS stuff, can you please move it into CDS and make it a query function there? Lets not leak more CDS knowledge into NMT than necessary.

Typo: upto

src/hotspot/share/services/memReporter.cpp line 55:

> 53: }
> 54: 
> 55: void MemReporterBase::print_total(size_t reserved, size_t committed, size_t read_only) const {

Since we now only print readonly for one tag in one report, we can keep this patch much smaller. The less complex the better. print_total gets called from several places that have nothing to do with CDS readonly.

My suggestion would be to handle CDS-specifics directly in `MemSummaryReporter::report_summary_of_type` since only there we want the "Shared class" line in the summary report to show readonly. My proposal would be:


--- a/src/hotspot/share/services/memReporter.cpp
+++ b/src/hotspot/share/services/memReporter.cpp
@@ -214,7 +214,11 @@ void MemSummaryReporter::report_summary_of_type(MEMFLAGS flag,
     outputStream* out   = output();
     const char*   scale = current_scale();
     out->print("-%26s (", NMTUtil::flag_to_name(flag));
-    flag == mtClassShared ? print_total(reserved_amount, committed_amount, read_only_bytes) : print_total(reserved_amount, committed_amount);
+    print_total(reserved_amount, committed_amount);
+    if (flag == mtClassShared) {
+      output()->print(", readonly=" SIZE_FORMAT "%s",
+                      amount_in_current_scale(read_only_bytes), scale);
+    }

src/hotspot/share/services/memReporter.cpp line 189:

> 187: 
> 188: void MemSummaryReporter::report_summary_of_type(MEMFLAGS flag,
> 189:   MallocMemory*  malloc_memory, VirtualMemory* virtual_memory, size_t read_only_bytes) {

You can actually move the retrieval of CDS read_only_bytes into this function now. No need to expand the interface. This function already retrieves a number of things, see e.g. thead stack printing, so there is a precedence.

test/hotspot/jtreg/runtime/NMT/SummarySanityCheck.java line 68:

> 66: 
> 67:     // Match '- <mtType> (reserved=<reserved>KB, committed=<committed>KB) and some times readonly=<readonly>KB
> 68:     Pattern mtTypePattern = Pattern.compile("-\\s+(?<typename>[\\w\\s]+)\\(reserved=(?<reserved>\\d+)KB,\\scommitted=(?<committed>\\d+)KB((,\\sreadonly=(?<readonly>\\d+)KB)|)\\)");

You can simplify this, since the only line where we want to see readonly is "Shared class space". That makes the test also more strict, since we do not want to see readonly on any other line.

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

Changes requested by stuefe (Reviewer).

PR: https://git.openjdk.org/jdk/pull/11401