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