RFR: 8337217: Port VirtualMemoryTracker to use VMATree

[Afshin Zafari]

On Fri, 27 Sep 2024 10:10:37 GMT, Johan Sjölen <jsjolen at openjdk.org> wrote:

> Sure, I can understand that it's nice to have both versions present during development. Right now it seems like we have a broken build, do you have any plans on having a functioning and fully featured build soon?

The new commit is pushed. I will monitor the build results.

> Rather than having 2 implementations, I'd like to see us aiming for integration for JDK-25 after forking 24, so integration in December. That would give us 6 months of ensuring stability of the new implementation before it reaches any customers. During those 6 months it will go through all of our testing multiple times over, and be used by OpenJDK developers. What do you think of this idea?

No objection. We can do this way.

>> It is inherited because it is not a new type. I think that `_tree->visit_reserved_region(par1, par2)` is more readable than `VMATree::visit_reserved_region(_tree, par1, par2)`
>> I could not find any *virtual functions* in these classes, what do you mean by that?
>
> I'm saying that inheritance is mostly useful when we have virtual functions which we can specialize, and that `VMATree` has none.

Done.
I used inheritance for extending a class.

>> How to use the ctor with a `bool` arg, then?
>
> The bool argument is just passed along.
> 
> ```c++
>   RegionsTree(bool with_storage) : VMATree(), _ncs_storage(with_storage) {
>   }

Done.
For my curiosity, what is the advantage?

>> Can we do this change as a separate RFE? It impacts the code a lot.
>
> Then just invert it: Have the outer class be static and the inner class be an instance. We can change the `MemoryFileTracker` to be that, as it's not as large of a change.

It is still a big change. Why not another RFE?

>> Generally, I think it is necessary to have performance tests to verify if any future changes do not degrade the performance.
>> The approach of stress testing can be agreed between us, but the tests have to exist.
>> In this approach, the input size is scaled N times and we expect the execution time grows as O(log(N)) which is much less than N.
>> This test fails and we need to have a justification for it. If approach is invalid or not correct implemented, we fix it. But the expectations remains the same.
>
> Why would the execution time grow logarithmically when we do linearly more work? When we run this with `N2` we will perform `10_000 * log(10_000, 2)` units of work, and for `N1` we will perform `1_000 * log(1_000, 2)` units of work, approximately. A closer approximation is `O(log(1)) + O(log(2)) + ... + O(log(n))` for `n = N2` or `n = N1`.
> 
> Let's calculate that:
> 
> 
>>>> import math
>>>> def time_it(n):
> ...     t = 0
> ...     for i in range(1, n):
> ...             t  = t + math.log(i)
> ...     return [t, 3*t] # Approximate best and worst case
> ... 
>>>> time_it(1000)
> [5905.220423209189, 17715.661269627566]
>>>> time_it(10_000)
> [82099.71749644217, 246299.15248932652]
>>>> def compare(a, b):
> ...     ab, aw = a
> ...     bb, bw = b
> ...     return [ bb / ab, bw / aw ]
> ... 
>>>> compare(time_it(1000), time_it(10_000))
> [13.902904821938064, 13.902904821938066]
>>>> # Ouch, that's outside of our linear bound!
> 
> 
> What I said previously also applies, especially the performance of `malloc` will impact this I suspect.

It is considered that `malloc` or other external events are the same for two cases. If we know that there might be some noise for one or another, we should check and disable them. This is the approach I have talked. How can we avoid noise from `malloc` side?

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

PR Comment: https://git.openjdk.org/jdk/pull/20425#issuecomment-2379615290
PR Comment: https://git.openjdk.org/jdk/pull/20425#issuecomment-2379618102
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1713461169
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1704394160
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1704401031
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1704424029