RFR: 8263464: NMT: assert in gtest os.release_multi_mappings_vm [v7]

[Johan Sjölen]

On Fri, 19 May 2023 16:58:00 GMT, Gerard Ziemski <gziemski at openjdk.org> wrote:

>> This fix allows NMT to account for released memory that was allocated in chunks, as long as the final pointer and size refer to total contiguous regions of the requested size.
>> 
>> The main idea here is to iterate over the "synthetic" region that was given to us and split it up into the underlaying existing regions, and then recursively handle them one at a time as we normally do.
>> 
>> Tested via `Mach5 tier1,tier2,tier3` and locally via `gtest:NMT*:os*`
>
> Gerard Ziemski has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Johan's feedback: add comments, handle pesky NULL

Hi!

Some more comments.

src/hotspot/share/services/virtualMemoryTracker.cpp line 528:

> 526:   }
> 527: 
> 528:   if (size <= reserved_rgn->size() && reserved_rgn->contain_region(addr, size)) {

A region which contains another must necessarily have at least the size of the contained region, therefore the size check is unnecessary.

src/hotspot/share/services/virtualMemoryTracker.cpp line 533:

> 531:     VirtualMemorySummary::record_released_memory(size, reserved_rgn->flag());
> 532:     assert(reserved_rgn->contain_region(addr, size), "Not completely contained");
> 533:     if (reserved_rgn->base() == addr || reserved_rgn->end() == addr + size) {

This is the one I wanted a comment for! "If the two regions start at the same place, or ends at the same place, then we can shrink the region from the left or right. Otherwise, we'll have to split it up"

src/hotspot/share/services/virtualMemoryTracker.cpp line 534:

> 532:     assert(reserved_rgn->contain_region(addr, size), "Not completely contained");
> 533:     if (reserved_rgn->base() == addr || reserved_rgn->end() == addr + size) {
> 534:       reserved_rgn->exclude_region(addr, size);

This is unfortunately not named "shrink_region_from" or something similar :-(. I don't expect you to rename the function, to be clear :).

src/hotspot/share/services/virtualMemoryTracker.cpp line 536:

> 534:       reserved_rgn->exclude_region(addr, size);
> 535:       return true;
> 536:     } else {

OK! We split the region into two, that looks good. What happens to the committed regions that are part of both the removed reserved region and one of the surviving regions (or why not both?).


          *   &
          |   |
[---------------]
       [----]
~>
[-----]     [---]


What if we have a committed region starting by the `*` and ending at the `&`, what happens to it?

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

PR Review: https://git.openjdk.org/jdk/pull/13813#pullrequestreview-1439165970
PR Review Comment: https://git.openjdk.org/jdk/pull/13813#discussion_r1201923368
PR Review Comment: https://git.openjdk.org/jdk/pull/13813#discussion_r1201927497
PR Review Comment: https://git.openjdk.org/jdk/pull/13813#discussion_r1201929344
PR Review Comment: https://git.openjdk.org/jdk/pull/13813#discussion_r1201946427