RFR: 8337217: Port VirtualMemoryTracker to use VMATree

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.

Hi,

Great work! I'm slowly working through the code, let's take our time with this. My PR got too stressful for everyone involved :-/.

I've reviewed some of the code. Could you go a bit more in-depth on what the results of this work was? Did it improve performance, for example?

>All tier1 tests pass except one that expects a 50% increase in committed memory *but it does not happen*.

~~I don't understand what this means, could you expand on this?~~ Edit: Re-read it again now and I get it, which test is this?

Thanks!

Changes requested by jsjolen (Reviewer).

This code generally over-uses pointers. Please have a go through this code and change to references when applicable.

Another round of review comments :).

What is the consensus on having two implementations alive at the same time? I'd like to see us delete the old VirtualMemoryTracker and only have the tree implementation left as part of this PR. What is the status of our testing? Is the tree version equivalent now and passes all tests?

I'd also like to see the tty printing code to either be deleted or if it's useful for mainline be adjusted so that it is using UL instead.

Some more reviews here along with some questions. I see that you've merged in my PR 8340103. That's good, let's wait until that PR is integrated before this is integrated.

> Is there a concrete advantage here for using lambda expression when iterating committed regions? I'm asking because personally I find
> 
> ```c++
>     while ((committed_rgn = itr.next()) != nullptr) {
>       print_committed_rgn(committed_rgn);
>     }
> ```
> 
> simpler and more compact, hence easier to understand, than
> 
> ```c++
>     CommittedMemoryRegion cmr;
>     VirtualMemoryTrackerWithTree::tree()->visit_committed_regions(reserved_rgn, &cmr, [&](CommittedMemoryRegion* crgn) {
>       print_committed_rgn(crgn);
>       return true;
>     });
> ```

Hi Gerard, I'm the one who prefers passing in a lambda and introduced that style, sorry :-).

First off, I think the lambda code should be simplified, it should be:

```c++
VMWT::tree().visit_committed_regions_of(*reserved_rgn, [](const CommittedMemoryRegion& crgn) {
  print_committed_region(crgn));
  return true;
});

I don't think that's too bad, right? The `return true` is a bit unfortunate. It's for being able to stop early, which I agree that regular iterators do better by just performing a `break;`.

I'll go ahead and say one nice thing too: If the body of the lambda is a bit more complicated, then we can look at the capture list (the `[]` above) to see what the lambda can alter in the outside scope. With a while-loop, you really have to read the whole thing.

**The reason** that I write these kinds of iterators is that they're much simpler to implement and maintain and, *to me*, STL-style iterators aren't easier to read, it's about the same level of complexity.

I'll admit that your style of iterators (which are *not* STL) is about the same complexity, though I find it unfortunate that we have to write an entire class for each iterator.

With the simplifications I made, is this style of iterator acceptable?

It seems like there's a bug with the virtual memory walker. Essentially, the thread stack walker extends the reserved memory region that's passed in, but we don't save the results of that addition. This is missed because the `SnapshotThreadStackWalker` casts away the `const` of the `const ReservedMemoryRegion*`.

```c++
// All code is in vmtCommon.cpp
void VirtualMemoryTracker::Instance::snapshot_thread_stacks() {
  SnapshotThreadStackWalker walker;
  walk_virtual_memory(&walker);
}
// ...
       while (itr.next_committed(committed_start, committed_size)) {
        assert(committed_start != nullptr, "Should not be null");
        assert(committed_size > 0, "Should not be 0");
        // unaligned stack_size case: correct the region to fit the actual stack_size
        if (stack_bottom + stack_size < committed_start + committed_size) {
          committed_size = stack_bottom + stack_size - committed_start;
        }
        region->add_committed_region(committed_start, committed_size, ncs); // <-- LOST!
        DEBUG_ONLY(found_stack = true;)
      }

This particular walker should probably be re-written to only use the `VMATree` facilities. Note that we can't do the insertion in the iteration loop, as our iterator will be invalidated. They must be accumulated first, and then inserted all at once.

Is this part of the code under-tested as we didn't receive a test failure regarding this?

src/hotspot/share/memory/metaspace/virtualSpaceNode.cpp line 262:

> 260:     vm_exit_out_of_memory(word_size * BytesPerWord, OOM_MMAP_ERROR, "Failed to reserve memory for metaspace");
> 261:   }
> 262:   assert(rs.size() == word_size * BytesPerWord, " must be");

" must be" -> "must be"

src/hotspot/share/nmt/memReporter.cpp line 442:

> 440:   if (all_committed) {
> 441:     bool reserved_and_committed = false;
> 442:     VirtualMemoryTracker::Instance::tree()->visit_committed_regions(*reserved_rgn,

Change the signature of `visit_committed_regions` to taking `(position start, size size)` instead of the `ReservedMemoryRegion`.

src/hotspot/share/nmt/memReporter.cpp line 458:

> 456:   }
> 457: 
> 458:   auto print_committed_rgn = [&](CommittedMemoryRegion* crgn) {

`crgn` can be const reference instead of pointer.

src/hotspot/share/nmt/memTracker.hpp line 291:

> 289:   // Query lock
> 290:   static Mutex*           _query_lock;
> 291: 

Remove

src/hotspot/share/nmt/nmtNativeCallStackStorage.hpp line 93:

> 91: 
> 92:   const inline NativeCallStack& get(StackIndex si) {
> 93:     if (is_invalid(si) || si >= _stacks.length()) {

I don't think this should be here?

src/hotspot/share/nmt/regionsTree.cpp line 33:

> 31:         log_debug(nmt)("trc base: " INTPTR_FORMAT " , trc end: " INTPTR_FORMAT,
> 32:                       p2i(region_in_tree.base()), p2i(region_in_tree.end()));
> 33:       }

Remove `with_trace` and improve the logging message. This should log unconditionally, probably under a `(nmt,vmtracker)` tagset.

src/hotspot/share/nmt/regionsTree.cpp line 50:

> 48:   ReservedMemoryRegion rgn = find_reserved_region(addr);
> 49:   return reserve_mapping((VMATree::position)addr, size, make_region_data(NativeCallStack::empty_stack(), rgn.mem_tag()));
> 50: }

Can now be fixed.

src/hotspot/share/nmt/regionsTree.cpp line 52:

> 50:     rgn = find_reserved_region(addr, true);
> 51:     ShouldNotReachHere();
> 52:   }

TODO: Remove this when 8335091 is merged

src/hotspot/share/nmt/regionsTree.cpp line 54:

> 52:   }
> 53:   return commit_mapping((VMATree::position)addr, size, make_region_data(stack, rgn.flag()));
> 54: 

Style: Extra space

src/hotspot/share/nmt/regionsTree.cpp line 64:

> 62:     rgn = find_reserved_region(addr, true);
> 63:     ShouldNotReachHere();
> 64:   }

TODO: Remove this when https://github.com/openjdk/jdk/commit/8335091a5dd9a0654d9a616addcfdcc1863747ca is merged

src/hotspot/share/nmt/regionsTree.hpp line 31:

> 29: #include "nmt/vmatree.hpp"
> 30: 
> 31: class RegionsTree : public VMATree {

Is it necessary for this to be a superclass? Seems like this class contains utilities for working with a `VMATree`. Could it instead be `AllStatic` and take a tree as its first argument? I'd like to avoid inheritance unless necessary (typically for the usage of virtual functions).

src/hotspot/share/nmt/regionsTree.hpp line 33:

> 31: class RegionsTree : public VMATree {
> 32:  private:
> 33:   NativeCallStackStorage* _ncs_storage;

No need for this to be a pointer.

src/hotspot/share/nmt/regionsTree.hpp line 38:

> 36:  public:
> 37:   RegionsTree(bool with_storage) : VMATree() , _ncs_storage(with_storage) { }
> 38: 

Whitespace

src/hotspot/share/nmt/regionsTree.hpp line 46:

> 44:   using Node = VMATree::TreapNode;
> 45: 
> 46:   class NodeHelper : public Node {

This shouldn't inherit from `Node` and then have each instance be cast into `NodeHelper`. Make into `class Utils : public AllStatic`.

src/hotspot/share/nmt/regionsTree.hpp line 47:

> 45: 
> 46:   class NodeHelper {
> 47:       Node *_node;

Style: `*` should hug type, not name.

src/hotspot/share/nmt/regionsTree.hpp line 50:

> 48:       inline bool is_before(VMATree::position addr) { return VMATree::PositionComparator::cmp(key(), addr) < 0; }
> 49:       inline bool is_after(VMATree::position addr) { return VMATree::PositionComparator::cmp(key(), addr) > 0; }
> 50:       inline bool is_region_begin() { return is_reserved_begin()|| is_committed_begin(); }

Not used?

src/hotspot/share/nmt/regionsTree.hpp line 52:

> 50:       inline bool is_region_begin() { return is_reserved_begin()|| is_committed_begin(); }
> 51:       inline bool is_region_end(NodeHelper* node) { return is_released_begin() || (node != nullptr && node->out_flag() != out_flag()); }
> 52:       inline bool is_end_only(NodeHelper* node) { return !is_region_begin() && is_region_end(node); }

Only used for debugging?

src/hotspot/share/nmt/regionsTree.hpp line 53:

> 51:       inline bool is_region_end(NodeHelper* node) { return is_released_begin() || (node != nullptr && node->out_flag() != out_flag()); }
> 52:       inline bool is_end_only(NodeHelper* node) { return !is_region_begin() && is_region_end(node); }
> 53:       inline bool is_joint(NodeHelper* node) { return is_region_begin() && is_region_end(node); }

Only used for debugging?

src/hotspot/share/nmt/regionsTree.hpp line 58:

> 56:       // inline bool is_region_end(NodeHelper* node) { return is_released_begin() || (node != nullptr && node->out_flag() != out_flag()); }
> 57:       // inline bool is_end_only(NodeHelper* node) { return !is_region_begin() && is_region_end(node); }
> 58:       // inline bool is_joint(NodeHelper* node) { return is_region_begin() && is_region_end(node); }

No commented out code like this please.

src/hotspot/share/nmt/regionsTree.hpp line 61:

> 59:       inline VMATree::StateType out_state() { return val().out.type(); }
> 60:       inline size_t distance_from(NodeHelper* other) { return key() - other->key(); }
> 61:       inline const NativeCallStack& stack(NativeCallStackStorage* ncss) { return ncss->get(val().in.stack()); }

Remove this

src/hotspot/share/nmt/regionsTree.hpp line 66:

> 64:       inline VMATree::StateType out_state() { return _node->val().out.type(); }
> 65:       inline size_t distance_from(NodeHelper& other) { return position() - other.position(); }
> 66:       inline const NativeCallStackStorage::StackIndex out_stack_index() { return _node->val().out.stack(); }

Unnecessary const in return type.

src/hotspot/share/nmt/regionsTree.hpp line 71:

> 69:       inline void set_in_flag(MEMFLAGS flag) { _node->val().in.set_flag(flag); }
> 70:       inline void set_out_flag(MEMFLAGS flag) { _node->val().out.set_flag(flag); }
> 71:       inline void dump(outputStream* st) {

`dump` is typically called `print_on` in HotSpot.

src/hotspot/share/nmt/regionsTree.hpp line 91:

> 89: 
> 90:   template<typename F> // F == bool(*)(CommittedMemoryRegion*)
> 91:   void visit_committed_regions(const ReservedMemoryRegion* rgn, CommittedMemoryRegion* cmr, F func) {

Why does it take the `ReservedMemoryRegion` as a pointer instead of a reference? There's no need to take a pointer to the `CommittedMemoryRegion`, just construct it in the function and pass it to `f`.

src/hotspot/share/nmt/regionsTree.hpp line 91:

> 89:   template<typename F> // F == bool(*)(CommittedMemoryRegion&)
> 90:   void visit_committed_regions(position start, size_t size, F func) {
> 91:     size_t end = start + size + 1;

It's not clear to me why `+ 1` is needed. If we have a range `[start, size+start)`, will it be missed by `visit_range_in_order`?

src/hotspot/share/nmt/regionsTree.hpp line 95:

> 93:     size_t end = (size_t)rgn->end();
> 94:     size_t comm_size = 0;
> 95:     size_t base = (size_t)rgn->base();

Why casting these?

src/hotspot/share/nmt/regionsTree.hpp line 108:

> 106:             CommittedMemoryRegion cmr((address)base, comm_size, st);
> 107:             comm_size = 0;
> 108:             prev.clear_node();

This is unneeded.

src/hotspot/share/nmt/regionsTree.hpp line 140:

> 138:         if (r_size != rgn_size) {
> 139:           log_debug(nmt)("----------------- size differ, distance: " SIZE_FORMAT " size: " SIZE_FORMAT, r_size, rgn_size);
> 140:         }

Not needed anymore?

src/hotspot/share/nmt/regionsTree.hpp line 143:

> 141:     visit_in_order([&](Node* node) {
> 142:       NodeHelper* curr = (NodeHelper*)node;
> 143:       /*

Delete

src/hotspot/share/nmt/regionsTree.hpp line 172:

> 170:   inline const NativeCallStack stack(NodeHelper& node) {
> 171:     NativeCallStackStorage::StackIndex si = node.out_stack_index();
> 172:     if (!si.is_invalid()) {

Change check to `NativeCallStackStorage::is_invalid`.

src/hotspot/share/nmt/virtualMemoryTrackerWithTree.cpp line 101:

> 99: }
> 100: 
> 101: void VirtualMemoryTrackerWithTree::apply_summary_diff(VMATree::SummaryDiff diff) {

Applying a summary diff in the MemoryFileTracker is this:

```c++
  for (int i = 0; i < mt_number_of_types; i++) {
    VirtualMemory* summary = file->_summary.by_type(NMTUtil::index_to_flag(i));
    summary->reserve_memory(diff.flag[i].commit);
    summary->commit_memory(diff.flag[i].commit);
  }

This is much simpler and doesn't require looking at signs and so on.

src/hotspot/share/nmt/virtualMemoryTrackerWithTree.cpp line 109:

> 107:       return true;
> 108:     });
> 109: }

TODO: This should be replaced with the implementation in 8340103

Will happen when that PR has been reviewed and integrated.

src/hotspot/share/nmt/virtualMemoryTrackerWithTree.cpp line 115:

> 113:                       " vms-committed: " SIZE_FORMAT,
> 114:                       str, NMTUtil::flag_to_name(flag), r, c, reserved, committed);
> 115:     };

Move this up to before the loop.

src/hotspot/share/nmt/virtualMemoryTrackerWithTree.hpp line 34:

> 32: #include "utilities/ostream.hpp"
> 33: 
> 34: class VirtualMemoryTrackerWithTree : AllStatic {

Can this class be redone to use the same pattern as `MemoryFileTracker` with an outer class that's not static and an inner class that's stack, storing an instance of the outer class? That makes testing the class by creating an instance of it possible, unlike our old tests.

src/hotspot/share/nmt/virtualMemoryTrackerWithTree.hpp line 41:

> 39:   VirtualMemoryTrackerWithTree(bool is_detailed_mode) : _tree(is_detailed_mode) { }
> 40: 
> 41:   bool add_reserved_region (address base_addr, size_t size, const NativeCallStack& stack, MEMFLAGS flag = mtNone);

Style: Align open paren with other functions.

src/hotspot/share/nmt/virtualMemoryTrackerWithTree.hpp line 64:

> 62:   void apply_summary_diff(VMATree::SummaryDiff diff);
> 63:   RegionsTree* tree() { return &_tree; }
> 64:   class Instance : public AllStatic {

Style: Newline to separate the new class to the getter above.

src/hotspot/share/nmt/virtualMemoryTrackerWithTree.hpp line 67:

> 65:     friend class VirtualMemoryTrackerTest;
> 66:     friend class CommittedVirtualMemoryTest;
> 67:     friend class ReservedMemoryRegion;

Why is this necessary?

src/hotspot/share/nmt/virtualMemoryTrackerWithTree.hpp line 69:

> 67: 
> 68:  private:
> 69:   static RegionsTree* _tree;

If you do the instance/static pattern then you can switch this into a regular member, no pointer necessary.

src/hotspot/share/nmt/virtualMemoryTrackerWithTree.hpp line 69:

> 67:     friend class ReservedMemoryRegion;
> 68: 
> 69:    private:

Unnecessary, remove.

src/hotspot/share/nmt/virtualMemoryTrackerWithTree.hpp line 70:

> 68:  private:
> 69:   static RegionsTree* _tree;
> 70:   static NativeCallStackStorage* _ncs_storage;

Doesn't seem like this is ever initialized?

src/hotspot/share/nmt/virtualMemoryTrackerWithTree.hpp line 77:

> 75: 
> 76:     static bool add_reserved_region (address base_addr, size_t size, const NativeCallStack& stack, MEMFLAGS flag = mtNone);
> 77: 

Style: Extra space

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

> 31: 
> 32: const char* VMATree::statetype_strings[4] = {
> 33:   "released","reserved", "'only-committed", "committed",

Extra quote in `"only-committed"`. Why is this new state required by the way?

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

> 34: };
> 35: 
> 36: void VMATree::put_if_absent(position A, position B, StateType state, const RegionData& region_data) {

Unused.

src/hotspot/share/nmt/vmatree.hpp line 40:

> 38: // or from committed memory of a certain MEMFLAGS to committed memory of a different MEMFLAGS.
> 39: // The set of points is stored in a balanced binary tree for efficient querying and updating.
> 40: class VMATree : public AnyObj {

Why is it AnyObj now?

src/hotspot/share/nmt/vmatree.hpp line 41:

> 39: // or from committed memory of a certain MemTag to committed memory of a different MemTag.
> 40: // The set of points is stored in a balanced binary tree for efficient querying and updating.
> 41: class VMATree : public CHeapObj<mtNMT> {

Is this still needed?

src/hotspot/share/nmt/vmatree.hpp line 57:

> 55:   };
> 56: 
> 57:   enum class StateType : uint8_t { Reserved = 1, Committed = 3, Released = 0, COUNT = 3 };

Why not the following?

```c++
  enum class StateType : uint8_t { Released, Reserved, Committed, COUNT };

If this particular ordering is necessary then it should be documented why.

src/hotspot/share/nmt/vmatree.hpp line 59:

> 57:   // Bit fields view: bit 0 for Reserved, bit 1 for Committed.
> 58:   // Setting a region as Committed preserves the Reserved state.
> 59:   enum class StateType : uint8_t { Reserved = 1, Committed = 3, Released = 0, COUNT = 4 };

Why do we now consider `StateType` to be a bit field?

src/hotspot/share/nmt/vmatree.hpp line 234:

> 232:   }
> 233: 
> 234:   void put_if_absent(position A, position B, StateType state, const RegionData& region_data);

Not used.

src/hotspot/share/nmt/vmtCommon.cpp line 1:

> 1: /*

Is this all of the code that is still required by the reporter to be able to function?

src/hotspot/share/nmt/vmtCommon.hpp line 40:

> 38: 
> 39: 
> 40: 

A lot of excessive spaces.

src/java.base/share/classes/java/lang/String.java line 2990:

> 2988:             return this;
> 2989:         }
> 2990:         return StringConcatHelper.doConcat(this, str == null ? "null" : str);

Remove

test/hotspot/gtest/nmt/test_nmt_treap.cpp line 168:

> 166:     tty->print_cr("d1: %lf, d2: %lf, d2/d1: %lf", d1, d2, d2 / d1);
> 167:     EXPECT_LT((int)(d2 / d1), N2 / N1);
> 168:   }

1000 and 10_000 elements are both quite small, isn't one measurement prone to being slower than expected due to unforeseen circumstances? For example, if `malloc` performs a heavy allocation (by mapping in a few new  pages) during the 10_000 elements insertion then that may stall enough that the `EXPECT_LT` fails. We are also bound to only performing the test once.

Is your thinking that these perf tests should be kept in, or should they be removed before integrating?

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

> 366: }
> 367: 
> 368: TEST_VM_F(NMTVMATreeTest, SummaryAccounting_dup) {

Better name for the test, please avoid underscores in test names. I know, we have underscores in places in Hotspot, but it's a good rule for GTest naming.

test/hotspot/jtreg/runtime/NMT/VirtualAllocCommitMerge.java line 123:

> 121:             checkCommitted(output, addrC, commitSize, "128KB");
> 122:             checkCommitted(output, addrE, commitSize, "128KB");
> 123:             System.out.println(output.getStdout());

Shouldn't be here?

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

PR Review: https://git.openjdk.org/jdk/pull/20425#pullrequestreview-2214987478
PR Review: https://git.openjdk.org/jdk/pull/20425#pullrequestreview-2216996817
PR Review: https://git.openjdk.org/jdk/pull/20425#pullrequestreview-2218354564
PR Review: https://git.openjdk.org/jdk/pull/20425#pullrequestreview-2229844713
PR Review: https://git.openjdk.org/jdk/pull/20425#pullrequestreview-2309488897
PR Review: https://git.openjdk.org/jdk/pull/20425#pullrequestreview-2330835910
PR Review: https://git.openjdk.org/jdk/pull/20425#pullrequestreview-2363917957
PR Comment: https://git.openjdk.org/jdk/pull/20425#issuecomment-2277599150
PR Comment: https://git.openjdk.org/jdk/pull/20425#issuecomment-2410656300
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1776822998
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1797708571
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1797709609
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1797709829
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1776830627
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1797710529
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1802524769
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1763180620
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1763179219
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1763181328
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1702620226
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1702613618
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1797710838
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1711201076
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1763121326
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1711202054
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1711202833
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1711203437
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1763122189
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1711204473
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1763087825
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1763114280
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1711194217
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1803020903
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1711195818
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1797710967
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1797711165
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1711102998
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1763084724
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1701512262
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1763130736
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1701507820
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1702621499
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1763078198
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1763079618
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1763080836
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1703756244
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1763081303
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1702615789
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1763078450
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1797711711
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1797711641
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1701493261
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1797711401
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1703759119
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1763128004
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1797711940
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1797712152
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1797712349
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1701486398
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1701503355
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1701513351
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1797712911