RFR: 8342730: Get rid of SummaryDiff in VMATree

Paul Hübner phubner at openjdk.org
Wed Aug 27 13:04:46 UTC 2025


On Wed, 13 Aug 2025 13:33:36 GMT, Afshin Zafari <azafari at openjdk.org> wrote:

> `SummaryDiff` result of any `register_mapping()` call, is passed into the function rather than returned. This enables avoiding copying the structure to/from the stack.
> 
> Tests:
> local linux-x64-debug: NMT gtests and JTREGs passed.

Thanks for the work, Afshin. I just have one small question and a few nitpicks, but other than that it looks in good shape to be merged.

src/hotspot/share/nmt/vmatree.cpp line 248:

> 246:                                                const RegionData& metadata, VMATree::SummaryDiff& diff, bool use_tag_inplace) {
> 247: 
> 248:   diff.clear();

I'm not sure I follow why this is introduced. Is there a situation where we/it makes sense to re-use `SumaryDiff`s?

test/hotspot/gtest/nmt/test_regions_tree.cpp line 102:

> 100:   NativeCallStack ncs;
> 101:   VMATree::RegionData rd = rt.make_region_data(ncs, mtTest);
> 102:   VMATree::SummaryDiff diff;

Nit: it's unused in the actual test/assertions right? Would call it `not_used` to be consistent with above test code then.

test/hotspot/gtest/nmt/test_regions_tree.cpp line 121:

> 119:   NativeCallStack ncs;
> 120:   VMATree::RegionData rd = rt.make_region_data(ncs, mtTest);
> 121:   VMATree::SummaryDiff diff;

Same naming nit here.

test/hotspot/gtest/nmt/test_regions_tree.cpp line 137:

> 135:   NativeCallStack ncs;
> 136:   VMATree::RegionData rd = rt.make_region_data(ncs, mtTest);
> 137:   VMATree::SummaryDiff diff;

Same naming nit here.

test/hotspot/gtest/nmt/test_vmatree.cpp line 1112:

> 1110:   {// Check committing into a reserved region inherits the call stacks
> 1111:     Tree tree;
> 1112:     VMATree::SummaryDiff diff;

Same nitpick here as in the other places in this file, I think it's more clear if we used `not_used` for cases where we discard the result. I'll stop pointing out every place this happens, consider this comment for the whole file.

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

PR Review: https://git.openjdk.org/jdk/pull/26761#pullrequestreview-3159649505
PR Review Comment: https://git.openjdk.org/jdk/pull/26761#discussion_r2303829869
PR Review Comment: https://git.openjdk.org/jdk/pull/26761#discussion_r2303845475
PR Review Comment: https://git.openjdk.org/jdk/pull/26761#discussion_r2303847970
PR Review Comment: https://git.openjdk.org/jdk/pull/26761#discussion_r2303848336
PR Review Comment: https://git.openjdk.org/jdk/pull/26761#discussion_r2303861190


More information about the hotspot-runtime-dev mailing list