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

[Thomas Stuefe]

On Fri, 5 May 2023 20:34:26 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.
>> 
>> 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:
> 
>   support partial release, add corresponding test

Changes requested by stuefe (Reviewer).

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

> 561:       size_t remove_size = MIN(remove_rgn->size(), remaining);
> 562:       assert(remove_rgn->base()+remove_size<=end, "not contained");
> 563:       assert(remove_released_region(remove_rgn->base(), remove_size), "error in remove_released_region");

Never put instructions into assert! assert(X), in release, makes puff and disappears together with X:

https://github.com/openjdk/jdk/blob/7d6a6c0c6d3df0b004b93c555fb24facf7b5e61b/src/hotspot/share/utilities/debug.hpp#L144

So, you won't execute remove_released_region in release.

test/hotspot/gtest/runtime/test_os.cpp line 553:

> 551: }
> 552: #endif // !AIX
> 553: 

- This tests case B, and we need case C and D too.
- You should make the first - middle - release the testing release with the partwise releasing. The final release is just to clean up the mess. For that to work, the stripes must be larger and a multiple of os::allocation_granularity, since otherwise you cannot release them partwise.

Would be cool if you fit all of these cases into a single function, e.g. 

static void test_multi_release(size_t stripe_size, int num_stripes, size_t release_range_offset, size_t release_range_size)


And then call them 4 times in 4 tests, for each of the test cases. E.g. with a stripe len set to 1M could be like this:


TEST_VM(TEST_VM, release_multi_mappings_full_regions) {
  test_multi_release(M, 4, M, 2*M); // release stripes 2 and 3 fully
}

TEST_VM(TEST_VM, release_multi_mappings_partial_regions) {
  test_multi_release(M, 4, M + 512*K, 512*K + M); // release stripe 2 partly, stripe 3 fully
}

TEST_VM(TEST_VM, release_multi_mappings_partial_regions) {
  test_multi_release(M, 4, M, M + 512*K); // release stripe 2 fully, stripe 3 partly
}

TEST_VM(TEST_VM, release_multi_mappings_partial_regions) {
  test_multi_release(M, 4, M + 256K, 512 * K); // release a 512K hole into stripe 2
}

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

PR Review: https://git.openjdk.org/jdk/pull/13813#pullrequestreview-1416140370
PR Review Comment: https://git.openjdk.org/jdk/pull/13813#discussion_r1187036805
PR Review Comment: https://git.openjdk.org/jdk/pull/13813#discussion_r1187058527