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

[Johan Sjölen]

On Wed, 17 May 2023 18:00:18 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:
> 
>   use size_t instead long

Hi Gerard,

Thank you for this. It's a very nice feature to have. I've actually had this precise problem. Your idea seems correct. I'm leaving some smaller comments now, and I hope to get back to you with a complete review soon. This problem requires a lot of attention to detail!

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

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

Can you assign the condition a bool variable with a descriptive name or have a comment? Something such as `bool not_in_middle` or `// Can only exclude if not from middle`

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

> 548:       }
> 549:     }
> 550:   } else {

`if (size <= reserved_rgn->size() && reserved_rgn->contain_region(addr, size))` always returns, so we don't need an `else` branch here, right?

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

> 555:     while (remaining > 0) {
> 556:       ReservedMemoryRegion* remove_rgn = node_rgn->data();
> 557:       assert(remove_rgn != nullptr, "NULL region");

Nit: `NULL` -> `null` in assert msg

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

> 575:     return true;
> 576:   }
> 577:   return false;

When exactly is this reachable?

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

PR Review: https://git.openjdk.org/jdk/pull/13813#pullrequestreview-1434275157
PR Review Comment: https://git.openjdk.org/jdk/pull/13813#discussion_r1198865949
PR Review Comment: https://git.openjdk.org/jdk/pull/13813#discussion_r1198870745
PR Review Comment: https://git.openjdk.org/jdk/pull/13813#discussion_r1198875565
PR Review Comment: https://git.openjdk.org/jdk/pull/13813#discussion_r1198872189