RFR: 8337217: Port VirtualMemoryTracker to use VMATree

Afshin Zafari azafari at openjdk.org
Fri Nov 8 10:51:33 UTC 2024


On Thu, 1 Aug 2024 15:44:32 GMT, Afshin Zafari <azafari at openjdk.org> wrote:

> - `VMATree` is used instead of `SortedLinkList` in new class `VirtualMemoryTrackerWithTree`.
>  -  A wrapper/helper `RegionTree` is made around VMATree to make some calls easier.
>  -  Both old and new versions exist in the code and can be selected via `MemTracker::set_version()`
>  - `find_reserved_region()` is used in 4 cases, it will be removed in further PRs.
>  - All tier1 tests pass except one ~that expects a 50% increase in committed memory but it does not happen~  https://bugs.openjdk.org/browse/JDK-8335167.
>  - Adding a runtime flag for selecting the old or new version can be added later.
>  - Some performance tests are added for new version, VMATree and Treap, to show the idea and should be improved later. Based on the results of comparing speed of VMATree and VMT, VMATree shows ~40x faster response time.

Thanks for your comments. They are replied or applied.

More fixes and questions.

Review comments applied.

The comments are applied.

Performance test runs insert/remove operations for SortedLinkList (SLL in the code) and Treap and expects that Treap be faster. Both tests pass with a factor of 70+ faster for 10000 elements.

Dear reviewers in recent commit the following can be considered: 
### Changes made to the code
- Instance/Static implemented.
- Using of Reference instead of pointers, as much as possible.
- Removed extra un-needed args from functions
- `NodeHelper` is implemented using composition approach.
- Non-used methods are removed from `NodeHelper`

### Still open conversations from previous reviews
- Two alternatives for implementing `apply_summary_diff`
- Is readability improved after latest changes?
- Are `lambda` usages acceptable now? Any improvement suggestion?
- Which pointers still remained to be replaced by references?

GHA failures are for some tests that have already a related JBS issue, not for this PR.

> ```c++
> region->add_committed_region(committed_start, committed_size, ncs); // <-- LOST!
> ```

The `region` is not a VMATree::node, it is a `ReservedMemoryRegion*`.

Dear @tstuefe, this PR has been reviewed couple of rounds. Would you please, give your feedback? 
Thanks.

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

PR Review: https://git.openjdk.org/jdk/pull/20425#pullrequestreview-2217791061
PR Review: https://git.openjdk.org/jdk/pull/20425#pullrequestreview-2219547262
PR Review: https://git.openjdk.org/jdk/pull/20425#pullrequestreview-2333278135
PR Review: https://git.openjdk.org/jdk/pull/20425#pullrequestreview-2365877947
PR Comment: https://git.openjdk.org/jdk/pull/20425#issuecomment-2273208292
PR Comment: https://git.openjdk.org/jdk/pull/20425#issuecomment-2283953888
PR Comment: https://git.openjdk.org/jdk/pull/20425#issuecomment-2380017488
PR Comment: https://git.openjdk.org/jdk/pull/20425#issuecomment-2410734124
PR Comment: https://git.openjdk.org/jdk/pull/20425#issuecomment-2462154991


More information about the core-libs-dev mailing list