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

[Gerard Ziemski]

On Fri, 5 May 2023 10:37:14 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> Gerard Ziemski has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   support partial release, add corresponding test
>
> src/hotspot/share/services/virtualMemoryTracker.cpp line 566:
> 
>> 564: 
>> 565:     return true;
>> 566:   }
> 
> - you don't need to carry/calculate remaining. The end pointer, and comparing each region's end with it, should be sufficient.
> - Some sort of assert that addresses are monotonously ascending would be assuring
> - The last region may not be completely part of the released memory range. See above, lines 534..546. Needs to be treated the same way here. Possibly you can fold both approaches together roughly like this:
> 
> while (release range end < current region end) {
>   release whole region
>   region = next region
> }
> // last region may not be fully contained
> if (last region not fully contained) {
>   split region, release lower half
> }

Thank you for the feedback!

I prefer:


    while (remaining > 0) {
    }


since the size if one of the primary arguments, which the existing logic uses to figure out how to handle it, so using `remaining` fits nicely in my opinion.

You suggested: 


// last region may not be fully contained
if (last region not fully contained) {
  split region, release lower half
}


but I'd rather not duplicate the logic in the lines 534..546

Hopefully, I accomplished the same functionality within the `while` loop using `remove_size` variable, that you are OK with?

> test/hotspot/gtest/runtime/test_os.cpp line 465:
> 
>> 463:     return;
>> 464:   }
>> 465: 
> 
> Since appearantly it is possible to release part of a range, we need also a test that tests multi-region-release but with the last region only partly covered.

Added `partial_release_multi_mappings` test to cover this.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/13813#discussion_r1186481705
PR Review Comment: https://git.openjdk.org/jdk/pull/13813#discussion_r1186482008