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

[Gerard Ziemski]

On Wed, 29 May 2024 13:09:46 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.
>> 
>> 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?
> 
> All tests green, and Oracle having run this through their CI.
> 
>> 
>> 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?
> 
> Yes
> 
>> 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?
> 
> Well, its not worse than what we do now for VirtualMemoryTracker, no? That said, when I am careful I usually try to separate output (writing to an opaque outputStream* that can be god knows what) from querying information. Simplest way is to query info from MemoryFileTracker under lock protection and write report to a stringStream first, than dump that one outside of lock protection to the real output stream.
> 
>> 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.
> 
> If you want to be super carefull, give us a diagnostic option that can switch off the new VMATree feed if needed. That way we won't see ZGC footprint in the output, but in case there are problems, we have a quick solution. If we don't see anything bad happening, we can remove that switch again.
> 
> --
> 
> I will take a last look at it start of next week. Tomorrow is holiday, and I am busy with some other things.

> 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.

I wanted to run some benchmarks, and gain a deeper understanding of how the code works. I will not block this PR moving forward, however, at this point I don't feel like I have enough understanding of the code to give a proper review, yet.

I can always catch up on it later after it's checked in.

If you do decide to check this in now, would you mind removing me as the reviewer (if you know how to this)? I don't think I earned a reviewer credit on this one yet

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

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