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

[Thomas Stuefe]

On Fri, 2 Dec 2022 22:44:14 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.
>
> Matias Saavedra Silva has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Assert should now pass on windows

Hi Matias,

Nice, this is useful! Small nits, but nothing major.

About the terminology: Ioi did suggest "shareable", whereas you use "readonly". IMHO "shareable" is a bit clearer, and also more correct since on Windows, that memory is not readonly. Also, you can have read-only mappings that are not shareable, and rw mappings that are.

Long term, not for this RFE: it would be cool if we could track shareability inside NMT as property of a mapping info. Or even just query the OS if the mapping is shared or private. That way we can remove any flag-specific special handling from NMT, and become a lot more flexible (e.g. if someone shares memory for a different MEMFLAG). 

Cheers, Thomas

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

> 53:                   amount_in_current_scale(reserved), scale,
> 54:                   amount_in_current_scale(committed), scale,
> 55:                   amount_in_current_scale(read_only), scale);

Since print_total(r,c,ro) is a superset of print_total(r,c), could we combine them to remove some duplication?

For example:


void MemReporterBase::print_total(size_t reserved, size_t committed, size_t read_only=0) const {
  const char* scale = current_scale();
  output()->print("reserved=" SIZE_FORMAT "%s, committed=" SIZE_FORMAT "%s", ..);
  if (shareable > 0) {
    output()->print(" (shareable=" SIZE_FORMAT "%s)", ..);
  }


and then hand in, to report_summary_of_type(m, v, ro), 0 for non-cds, >0 for cds?

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

> 119:   FileMapRegion* r = FileMapInfo::current_info()->region_at(MetaspaceShared::ro);
> 120:   // Region will be read-write on windows, otherwise this is a sanity check
> 121:   if (!MetaspaceShared::use_windows_memory_mapping())

Can be folded into the assert, e.g.
```assert(MetaspaceShared::use_windows_memory_mapping() || r->read_only())```
(otherwise, would need curly brackets)

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

> 70:     // Match 'Total: reserved=<reserved>KB, committed=<committed>KB'
> 71:     Pattern totalMemoryPattern = Pattern.compile("Total\\:\\sreserved=(?<reserved>\\d+)KB,\\scommitted=(?<committed>\\d+)KB,\\sreadonly=(?<readonly>\\d+)KB");
> 72:     // Match '- <mtType> (reserved=<reserved>KB, committed=<committed>KB)

update comment?

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

> 80:           totalCommitted = Long.parseLong(totalMemoryMatcher.group("committed"));
> 81:           totalReserved = Long.parseLong(totalMemoryMatcher.group("reserved"));
> 82:           totalReadonly = Long.parseLong(totalMemoryMatcher.group("readonly"));

totalReadonly is nowhere used. Remove it? Or add a check for it?

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

> 98:           }
> 99:           // Make sure readonly is always less or equals
> 100:           if (typeShared > typeReserved) {

Shouldn't this be committed, not reserved? Shareable data should be committed, right?

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

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