RFR: 8312132: Add tracking of multiple address spaces in NMT [v46]

[Johan Sjölen]

On Tue, 23 Apr 2024 14:21:23 GMT, Afshin Zafari <azafari at openjdk.org> wrote:

>> Johan Sjölen has updated the pull request incrementally with two additional commits since the last revision:
>> 
>>  - Remove faulty condition after removing merging
>>  - Add failing test case
>
> src/hotspot/share/nmt/vmatree.cpp line 188:
> 
>> 186:     // LEQ_A - A - B - GEQ_B
>> 187:     auto& rescom = diff.flag[NMTUtil::flag_to_index(LEQ_A.flag_out())];
>> 188:     if (LEQ_A.state.out.type() == StateType::Reserved) {
> 
> Would be nice to have wrappers that allow us write these as:
> `LEQ_A.is_out_reserved()`
> or
> `LEQ_A.is_out_committed()`

I prefer showing the explicit comparison being done, instead of hiding it under a function or method. Just to make it obvious that there's no extra logic being performed. I did shorten the code by adding `out()` and `in()` accessors.

> src/hotspot/share/nmt/vmatree.cpp line 242:
> 
>> 240:   }
>> 241:   return diff;
>> 242: }
> 
> Would be nice if we can break this function into some smaller sub-functions. It is 200+ line now and little hard to track the logic. Thanks!

Sure, I think there are a couple of cases which are actual functions (taking input, producing output, nothing else), those can be converted.

> src/hotspot/share/nmt/vmatree.hpp line 56:
> 
>> 54: 
>> 55:   // Each point has some stack and a flag associated with it.
>> 56:   struct Metadata {
> 
> `State` and `Metadata` are attributes of a Node and not to be in VMATree.

Sorry, could you expand on what you mean here?

> src/hotspot/share/nmt/vmatree.hpp line 63:
> 
>> 61:       : stack_idx(),
>> 62:         flag(mtNone) {
>> 63:     }
> 
> can fit in 1 line.

Fixed

> src/hotspot/share/nmt/vmatree.hpp line 70:
> 
>> 68:     static bool equals(const Metadata& a, const Metadata& b) {
>> 69:       return NativeCallStackStorage::StackIndex::equals(a.stack_idx, b.stack_idx) &&
>> 70:              a.flag == b.flag;
> 
> `a.flag == b.flag` can be left-hand of `&&` to be more efficient.

Fixed

> src/hotspot/share/nmt/vmatree.hpp line 135:
> 
>> 133:   SummaryDiff register_mapping(size_t A, size_t B, StateType state, Metadata& metadata);
>> 134: 
>> 135:   SummaryDiff reserve_mapping(size_t from, size_t sz, Metadata& metadata) {
> 
> If we use `reserve_mapping` for `uncommit_memory`, we need to set a `StackIndex` and a `MEMFLAGS` to pass as a `Metadata`. If we use `mtNone` for example, all the uncommitted amount would be accounted for `mtNone`.
> Would you please provide a `uncommit_mapping(address, size)` to handle these issues properly?

Let's wait with this until we actually port over the `VirtualMemoryTracker` to use `VMATree`.

> src/hotspot/share/nmt/vmatree.hpp line 145:
> 
>> 143:   SummaryDiff release_mapping(size_t from, size_t sz) {
>> 144:     Metadata empty;
>> 145:     return register_mapping(from, from + sz, StateType::Released, empty);
> 
> `return register_mapping(from, from + sz, StateType::Released, Metadata{});`

Can't be done, `register_mapping` takes a reference and not a value.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1579237985
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1579239062
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1579246503
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1579242029
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1579242086
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1579243316
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1579244295