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

[Thomas Stuefe]

On Fri, 5 May 2023 20:36:00 GMT, Gerard Ziemski <gziemski at openjdk.org> wrote:

>> 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 is 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?

Better, but still not complete. Sorry, I realized we actually have 4 cases:

A) region is fully contained in release range
B) (first) region start is outside release range (cutting off start portion)
C) (last) region end is outside release range  (cutting off end portion)
D) (single) region start and end are outside release range, but range is inside region (a hole)


The existing code (lines 527..548) already handles all these cases, but just for a single region. You want to extent this
to multi region, so you must also handle cases A..D.

But since the single region case is a special case of multi region case, you can reuse all of that code. Roughly like this:


while (remaining >0) {
  if (region is fully contained in release range) {
    release region // A
  } else if (region start is outside range) {
   // see lines 529..533
    region->exclude_region(only the end part) // B
  } else if (region end is outside range) {
   // see lines 529..533
    region->exclude_region(only the start part) // C
  } else if (region start and end are outside, but release range is inside) {
    // Case D
    // See lines 534 ff: release "hole" range and create a new region, so that we have two survivor regions.
  }
}


Note that current code folds cases B and C into one.

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

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