RFR: 8337217: Port VirtualMemoryTracker to use VMATree

[Afshin Zafari]

On Fri, 23 Aug 2024 21:02:25 GMT, Gerard Ziemski <gziemski at openjdk.org> wrote:

> Is the plan to check-in the fix with both paths? Or are we going to remove the linked-list based one after the review?

The plan is to have both versions available at run-time. In this plan, we will add JVM options to let the user to select either of them. This can be used for: 1️⃣ stepping back if new version fails in some use-cases, and also for 2️⃣ running benchmarks and comparing the results of both.

> I still haven't run the benchmarks to verify the speed increase promise. Ideally, I would like to do this with my mechanism, but will only do it, if I can manage it without getting sucked into working on the mechanism itself.
> 
> I will definitively want to run the provided microbenchmarks though.

In `test_vmt_with_tree.cpp`, two versions are compared. One of the tests (`PerformanceComparison`) is comparing the time they take for the same operation. This test is skipped for now, because its expectation of time-improvement fails. Until the time we still did not merge all other related changes (like the `copy_flag` or `use_tag_inplace`, [#20330](https://github.com/openjdk/jdk/pull/20330)) to this PR we cannot expect the time to be improved.

> src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 279:
> 
>> 277:                                 "Cannot commit verification bitmap memory");
>> 278:     }
>> 279:     MemTracker::record_virtual_memory_type(verify_bitmap.base(), verify_bitmap..size(), mtGC);
> 
> Does this even compile?

No it doesn't, you're right. Shenandoah is not in the main build.
I thought that GHA builds do this but seems not.
Fixed. Thanks.

> src/hotspot/share/nmt/memReporter.cpp line 440:
> 
>> 438:   if (all_committed) {
>> 439:     if (MemTracker::is_using_sorted_link_list()) {
>> 440:     CommittedRegionIterator itr = reserved_rgn->iterate_committed_regions();
> 
> Indentation.

Done.

> src/hotspot/share/nmt/nmtNativeCallStackStorage.hpp line 94:
> 
>> 92:     if (si._stack_index < 0 || si._stack_index >= _stacks.length()) {
>> 93:       return _fake_stack;
>> 94:     }
> 
> Is that a leftover from debugging?
> 
> Shouldn't this be an `assert` in final code?

You are right. Since the next lines check the `-1` as special case; this check should be either an `assert` or logged. I wait for @jdksjolen, as this is his code.

> src/hotspot/share/nmt/nmtTreap.hpp line 219:
> 
>> 217:       }
>> 218:       last_seen = node;
>> 219:       return true;
> 
> Why are we returning a `bool` value in `void` function?

That is returning from the lambda function.
In `visit_in_order(F func)`, when function `func` (in our case the lambda) returns `false` it means the iteration should stop. Otherwise, if it returns `true` it means to continue iterating.

> src/hotspot/share/nmt/nmtTreap.hpp line 320:
> 
>> 318:       }
>> 319:       head = to_visit.pop();
>> 320:       if(!f(head))
> 
> Needs space `if (!f(head))`

Fixed.

> src/hotspot/share/nmt/nmtTreap.hpp line 348:
> 
>> 346:       const int cmp_to = COMPARATOR::cmp(head->key(), to);
>> 347:       if (cmp_from >= 0 && cmp_to < 0) {
>> 348:         if(!f(head))
> 
> Needs space `if (!f(head))`

Done.

> src/hotspot/share/nmt/nmtTreap.hpp line 356:
> 
>> 354:         head = nullptr;
>> 355:       }
>> 356: 
> 
> Remove.

Done

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

PR Comment: https://git.openjdk.org/jdk/pull/20425#issuecomment-2309507439
PR Comment: https://git.openjdk.org/jdk/pull/20425#issuecomment-2378923225
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1778342931
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1713348422
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1722829439
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1778361486
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1722821927
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1722822141
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1722822343