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

[Johan Sjölen]

On Fri, 24 May 2024 06:13:54 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>>> We claim that:
>>> 
>>> > Our Treap-based approach in this patch gives a performance boost such that we see 25x better performance in a benchmark.
>>> 
>>> May I ask how you ran it? I would like to be able to reproduce our claim.
>> 
>> Sure, it was a while since I ran the benchmark. You're going to have to do a bit of work here, to get it working.
>> 
>> You take this file: https://github.com/tstuefe/jdk/blob/6be830cd2e90a009effb016fbda2e92e1fca8247/test/hotspot/gtest/nmt/test_nmtvmadict.cpp#L1
>> 
>> And you port it to the VMATree instead of VMADict (or whatever it's called). Then you run it and look at output. You could also take one of the stress tests that I made, remove the verification calls, and run the same stress test for VirtualMemoryTracker.
>
>> > We claim that:
>> > > Our Treap-based approach in this patch gives a performance boost such that we see 25x better performance in a benchmark.
>> > 
>> > 
>> > May I ask how you ran it? I would like to be able to reproduce our claim.
>> 
>> Sure, it was a while since I ran the benchmark. You're going to have to do a bit of work here, to get it working.
>> 
>> You take this file: https://github.com/tstuefe/jdk/blob/6be830cd2e90a009effb016fbda2e92e1fca8247/test/hotspot/gtest/nmt/test_nmtvmadict.cpp#L1
>> 
>> And you port it to the VMATree instead of VMADict (or whatever it's called). Then you run it and look at output. You could also take one of the stress tests that I made, remove the verification calls, and run the same stress test for VirtualMemoryTracker.
> 
> The claim makes also sense if you think about it. A binary tree will always grossly outperform a linked list for sorted insert/delete.

Hi @tstuefe, @gerard-ziemski, @afshin-zafari 

What do we think is necessary to have this PR merged in?

Right now, I know that Thomas has some gripes with the private/public API and visibility. I agree, it can be cleaned up, but can't this wait until after the PR is merged? I believe that there are multiple small clean ups and fixes that gets rid of some ugliness, but the actual functionality of this PR is over all well-tested.

I see the following points as needing attention before merging:

1. NativeCallStackStorage -- needs some testing for both summary and detailed mode. *Maybe* get the `bool is_detailed` out of there, but to me this is optional, it receives the info from `MemTracker` anyway, just through the constructor.
2. The locking and reporting mechanisms. Is locking the MemoryFileTracker structures for the duration of the JCMD call acceptable? This means potential stalling of the VM, no?
3. Run through some better/deeper testing than just GHA

Is there anything that I am missing? This will have limited rollout to the subset of users using both ZGC and NMT.

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

PR Comment: https://git.openjdk.org/jdk/pull/18289#issuecomment-2135213186