RFR: 8337217: Port VirtualMemoryTracker to use VMATree [v8]

[Robert Toyonaga]

On Tue, 10 Dec 2024 09:55:28 GMT, Afshin Zafari <azafari at openjdk.org> wrote:

>> I have a couple more questions about this function:
>> 
>> Why bother keeping the variable `base`. It's updated whenever `prev` is updated, so why not just use `prev`?
>> 
>> Why do you allow for grouping two adjacent committed regions with different `MemTags` together when creating `CommittedMemoryRegion`? Shouldn't they be treated as separate committed regions?  And if we are supposed to be grouping adjacent regions, shouldn't we be using the address of the first region, not the current one, as the base address when creating `CommittedMemoryRegion` (since we've been incrementing `comm_size`)?
>
> Your point on `base` is correct. `prev.position` can be used instead.
> 
> I agree that there is ambiguity in the method name and its args. This method SHOULD be called for a `ReservedMemoryRegion [base, base + size)`. This is implicitly assumed. The signature as `visit_committed_regions(ReservedMemoryRegion& rgn, F func)` is more declarative then.
> 
> With the above assumption, if two committed regions are adjacent with different `MemTag`, they certainly belong to different `ReservedMemoryRegion`s, one for CDS with `mtClassShared` and another for stack with `mtThreadStack` MemTag. This method doesn't care the `MemTag`s because all the sub-regions within the `ReservedMemoryRegion [base, base + size)` are assumed to have the same `MemTag`.

Ok that makes sense to me if `visit_committed_regions` is only ever called on a single ReservedRegion.  But if that assumption is true, then isn't it also true that we should never encounter two adjacent committed regions (they should already be coalesced)? 

So then maybe you don't need the extra handling such as tracking `comm_size` or checking `if (!curr.is_committed_begin())`. Maybe you could just directly have something like:

if (prev.is_valid() && prev.is_committed_begin()) {
    CommittedMemoryRegion cmr((address) prev.position(), curr.distance_from(prev), st);
...
}

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1878235516