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

[Thomas Stuefe]

On Fri, 22 Mar 2024 16:29:30 GMT, Johan Sjölen <jsjolen at openjdk.org> wrote:

>> src/hotspot/share/nmt/vmatree.hpp line 63:
>> 
>>> 61:     }
>>> 62:   };
>>> 63: 
>> 
>> In my original proposal, a `MappingState` (https://github.com/tstuefe/jdk/blob/6be830cd2e90a009effb016fbda2e92e1fca8247/src/hotspot/share/nmt/vmaTree.cpp#L106C7-L114) is the combination of ( MEMFLAGS + VMAstate ).
>> 
>> A `MappingStateChange` https://github.com/tstuefe/jdk/blob/6be830cd2e90a009effb016fbda2e92e1fca8247/src/hotspot/share/nmt/vmaTree.cpp#L38-L65 is the combination of two `MappingState`. Its "is_noop()" method compares the combination of both ( MEMFLAGS + VMAstate ) tuples.
>> 
>> This `State` here is neither. It contains only the incoming VMAState, and carries the full information only for the outgoing state. Its ` is_noop` only compares VMAState. This renders `is_noop()` pretty useless since it alone is not sufficient when checking if areas for mergeability. Consequently, you then tree-search the adjacent node whenever is_noop is used ( e.g. line 161, `if (is_noop(stA) && Metadata::equals(stA.metadata, leqA_n->val().metadata)) { ` .
>> 
>> I would suggest following my proposal here and carrying the full information for both incoming and outgoing states. Advantages would be:
>> - it removes the need for tree searches whenever we check whether neighboring areas can be folded.
>> - it would also make the code easier to read and arguably more robust
>> - it is arguably more logical - semantically, there is no difference between VMAState and the rest of the state information, all of them are part of the total region state that determines if regions are mergeable.
>> 
>> If you are worried about data size: all the information we need can still be encoded in a 64-bit value, as suggested above. And compared with a single cmpq. 16 bits for MEMFLAGS, 2 bits for VMAState, and 32 bits are more than enough as a callstack id (in fact, 16 bits should be enough).
>
> Hi, just to be clear: We don't perform a tree search whenever `is_noop` is used, the `leqA_n` is also present in your draft. Still, I think you might be right that having the full state for both in and out is the best way to go, I'll try it out as a simplification.

Ah, you are of course right.

I remember trying your approach first too: only keeping one state per node (also the outgoing state). Because, like you, I thought that the redundant data are unnecessary. But I ran into problem with rare corner cases, so I switched to the in-out-model. 

Unfortunately I blank out when trying to remember what the issue actually was. Figures :)

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

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