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

[Thomas Stuefe]

On Mon, 13 May 2024 12:17:42 GMT, Johan Sjölen <jsjolen at openjdk.org> wrote:

>> src/hotspot/share/nmt/vmatree.hpp line 67:
>> 
>>> 65: 
>>> 66:     Metadata(NativeCallStackStorage::StackIndex stack_idx, MEMFLAGS flag)
>>> 67:     : stack_idx(stack_idx), flag(flag) {}
>> 
>> I would assert here that with state=released, we only ever want to see mtNone.
>
> We don't have the state here, do you mean in `IntervalState` ctr? I'm doing `!(type == Released) || flag == mtNone` instead of `!=` to make the usage of `~P v Q equiv. P => Q` obvious.

Sure, works for me

>> src/hotspot/share/nmt/vmatree.hpp line 91:
>> 
>>> 89:     StateType type() const {
>>> 90:       return static_cast<StateType>(type_flag[0]);
>>> 91:     }
>> 
>> Proposal: provide `is_reserved()` and `is_committed()` and replace manual comparisons with the state enum with those. Easier on the eye.
>
> Afshin mentioned this too, I believe, but I want to push back here. I prefer showing what we're doing here (simple comparison) rather than hiding it behind a utility function. Requires less jumping around when reading unknown code.

Tiny utility functions like this are effectively resolved in modern C++ IDEs like CDS. (In stark contrast to templates, which make IDEs very confused). And a clear name provides safety against accidental typos.

I leave this up to you.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1598420951
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1598426270