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

[Johan Sjölen]

On Wed, 17 Apr 2024 06:01:07 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:
>> 
>>   Style, copyright
>
> src/hotspot/share/nmt/nmtTreap.hpp line 142:
> 
>> 140:     }
>> 141:   }
>> 142: 
> 
> Allocating:
> 
> Why do we need to pass in allocators and deallocators as arguments? The only reason I can see is to have per-instance allocators that use tree-local variables. But why does it need access to the tree instance it works for at all? The only thing I see it accesses from the treap is the random seed. That should be using a global seed var, and os::next_random. A global seed var would be perfectly fine in this case, I think. The treap is only accessed under lock protection, right? Even if not, os::random() (the variant using a global seed with CAS) would be a valid alternative.
> 
> (Arguably, initializing the rng seed is also not the job of an allocator, but of the function calling the allocator)
> 
> So, a simplification would be to have the allocator and deallocator as class template parameters for the treapnode or for the trea. That would also get of the many duplicate definitions of free functions that all call os::free, for instance.
> 
> A further simplification would be to have just one Allocator template parameter giving me an allocation and a deallocation function, since they come and work in pairs.
> 
> A further simplification would be to have these actually be part of the tree, not the tree node: I prefer the individual nodes in a tree or list or similar to be dumb data holders, and for the containing tree to hold the tree related logic of merging, inserting etc. I think that is the more natural distribution of responsibilities. It also simplifies things that are done on a per-tree basis, like accounting. It avoids having to marshall a lot of calls (e.g. Tree->upsert just delegates to TreeNode->upsert)

Sure, the allocator isn't responsible for seeding the nodes. It was simply convenient that the class which does allocation also happens to hold the seed, so I could have them do both together.

I came to the opposite conclusion that you did: I didn't want to have a global shared state seed as that makes me have to think about the behavior of the RNG in the presence of multiple threads and instances of the treap. 

>A further simplification would be to have these actually be part of the tree, not the tree node: I prefer the individual nodes in a tree or list or similar to be dumb data holders, and for the containing tree to hold the tree related logic of merging, inserting etc. I think that is the more natural distribution of responsibilities. It also simplifies things that are done on a per-tree basis, like accounting. It avoids having to marshall a lot of calls (e.g. Tree->upsert just delegates to TreeNode->upsert)

Right, I could do that. I'll look into this.

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

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