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

[Thomas Stuefe]

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

>> src/hotspot/share/nmt/nmtNativeCallStackStorage.hpp line 30:
>> 
>>> 28: #include "utilities/growableArray.hpp"
>>> 29: #include "utilities/nativeCallStack.hpp"
>>> 30: 
>> 
>> Here, I would love it if we had smaller than pointer-sized callstack IDs. 
>> 
>> The simplest way would be to copy callstacks into a growable area, and use their numerical indices as ID. We can get by with 16 bits, or 32 bits if we think 64K callstacks are not enough (they should be enough).
>> 
>> Those can then be combined very efficiently with MEMFLAG and VMAState in the VMATree nodes.
>> 
>> The hashmap for reverse id lookup could be a standard hashmap of key=callstack, value=ID.
>> 
>> As a future possible improvement, that could then replace the MallocSiteTable which does almost the same thing. (only have to avoid using malloc then, because of recursivities).
>
> I still don't get how a growable array is supposed to work for open addressing while having the indices staying immutable :-).
> 
> How does this work:
> 
> ```c++
>   HTable ht; // Open-addressed GrowableArray with linear probing
>   Index oldidx = ht.put(4);
>   // A bunch of puts, leading to a resize of the array
>   Index newidx = ht.put(4);
>   assert(oldidx == newidx, "how is this ensured?");
> 
> 
> How does this work?

Huh? Should the indexes not be stable across resize? Unless you shrink, which we would not need to do?

The base address of the array may change, but the relative order of the items in it hopefully not.

>> src/hotspot/share/nmt/nmtNativeCallStackStorage.hpp line 84:
>> 
>>> 82:       return *_stack;
>>> 83:     }
>>> 84:   };
>> 
>> What is the point of the StackIndex class?
>
> The `StackIndex` used to be a 4 byte encoding which required the `NativeCallStackStorage` to be able to be dereferenced. This method should at the very least be deleted and replaced with `NativeCallStackStorage::get`.

Not sure I follow you. But if you follow my proposal above, StackIndex could be just a 4byte index:

On store, place callstack into array. Possibly resize array, if full. Return index. index is now uniquely identifying the stack.

If you want to keep the linked list - after all, this is just a performance- and memory-optimization - why not just return a const NativeCallStack* instead of an index?

>> src/hotspot/share/nmt/vmatree.hpp line 46:
>> 
>>> 44: 
>>> 45:   // Each node has some stack and a flag associated with it.
>>> 46:   struct Metadata {
>> 
>> all members const?
>
> Can't be const unless we want merge to be a function.

Okay!

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1584309014
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1584312388
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1584190386