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

[Johan Sjölen]

On Thu, 9 May 2024 10:38:14 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> Johan Sjölen has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Some style
>
> src/hotspot/share/nmt/nmtTreap.hpp line 203:
> 
>> 201:     TreapNode* last_seen = nullptr;
>> 202:     bool failed = false;
>> 203:     this->visit_in_order([&](TreapNode* node) {
> 
> Here, and in other places: what's with the this-> ?

Just being very explicit :-). No need for it.

> src/hotspot/share/nmt/nmtTreap.hpp line 240:
> 
>> 238:     DEBUG_ONLY(_node_count++;)
>> 239:     // Doesn't exist, make node
>> 240:     void* node_place = ALLOCATOR::allocate(sizeof(TreapNode));
> 
> Please make it explicit in the class definition that the ALLOCATOR must be checking for oom and exit or whatever.

Done.

> src/hotspot/share/nmt/nmtTreap.hpp line 254:
> 
>> 252: 
>> 253:     // (LEQ_k, GT_k)
>> 254:     node_pair fst_split = split(this->_root, k, LEQ);
> 
> Can we afford some more letters please? :-) fst : first snd : second
> But since you use left and right in other places, I'd use that too here.

Left/right are a bit confusing here, first and second do not correspond to each other in a left/right fashion while the resulting pair of a split does. It is true that `merge(second.left, second.right) == first.left`.

> src/hotspot/share/nmt/nmtTreap.hpp line 307:
> 
>> 305:   }
>> 306: 
>> 307:   TreapNode* closest_leq(const K& key) {
> 
> I don't understand the naming of the variables. What is A? _n? _r? 
> And "_head" is somewhat misleading. I would have named head=pos or current, leqA_n = best or found or candidate or best_so_far... any of these

That's fair

> src/hotspot/share/nmt/virtualMemoryTracker.hpp line 33:
> 
>> 31: #include "nmt/allocationSite.hpp"
>> 32: #include "nmt/nmtCommon.hpp"
>> 33: #include "runtime/atomic.hpp"
> 
> Please add includes only where needed, directly. Let's not rely on indirect includes. Unless this is a remnant from some earlier version, then pls just remove it.

L71:

```c++
  inline size_t peak_size() const {
    return Atomic::load(&_peak_size);
  }


We probably were relying on an indirect include previously. Can we keep this even though it's irrelevant to the PR :-)?

> src/hotspot/share/nmt/vmatree.hpp line 117:
> 
>> 115:     }
>> 116:   };
>> 117: 
> 
> Do `IntervalState` and `IntervalChange` need to be exposed in the header?

No, they do not. I've found it difficult to clean up so that we have the minimal amount of private/public switches in the class, so I'll do the easiest change instead of the cleanest change.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1598277413
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1598276045
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1598274713
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1598269216
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1598251906
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1598256008