8071277

Hi all,

  please review this change to G1 commit/uncommit to merge commit/uncommit calls to the operating system as much as possible. This significantly improves startup performanc

Performance results before/after on Zen3 processor for a "HelloWorld" application:

```
 $ hyperfine -w 10 -r 10 "baseline/bin/java -XX:+UseG1GC -Xms128m -Xmx128m Hello"
Benchmark 1: baseline/bin/java -XX:+UseG1GC -Xms128m -Xmx128m Hello
  Time (mean ± σ): 16.4 ms ± 0.4 ms [User: 11.0 ms, System: 14.4 ms]
  Range (min … max): 15.7 ms … 16.8 ms 10 runs

$ hyperfine -w 10 -r 10 "baseline/bin/java -XX:+UseG1GC -Xms2g -Xmx2g Hello"
Benchmark 1: baseline/bin/java -XX:+UseG1GC -Xms2g -Xmx2g Hello
  Time (mean ± σ): 24.7 ms ± 0.4 ms [User: 10.7 ms, System: 22.7 ms]
  Range (min … max): 24.2 ms … 25.4 ms 10 runs

My reimplementation of JDK-8071277 does show improvements here:

$ hyperfine -w 10 -r 10 "changes/bin/java -XX:+UseG1GC -Xms128m -Xmx128m Hello"
Benchmark 1: changes/bin/java -XX:+UseG1GC -Xms128m -Xmx128m Hello
  Time (mean ± σ): 15.9 ms ± 0.4 ms [User: 11.9 ms, System: 13.1 ms]
  Range (min … max): 15.4 ms … 16.7 ms 10 runs

$ hyperfine -w 10 -r 10 "changes/bin/java -XX:+UseG1GC -Xms2g -Xmx2g Hello"
Benchmark 1: changes/bin/java -XX:+UseG1GC -Xms2g -Xmx2g Hello
  Time (mean ± σ): 19.7 ms ± 0.3 ms [User: 11.3 ms, System: 17.4 ms]
  Range (min … max): 19.2 ms … 20.1 ms 10 runs

```

I.e., depending on actually committed heap size (`-Xms`), reduction of startup time by 20% or so in above cases.

Testing: tier1-5, gha

Thanks,
  Thomas

Author: tschatzl

src/hotspot/share/gc/g1/g1NUMA.cpp

@@ -203,9 +203,7 @@ uint G1NUMA::index_for_region(G1HeapRegion* hr) const {
 //      * G1HeapRegion #: |-#0-||-#1-||-#2-||-#3-||-#4-||-#5-||-#6-||-#7-||-#8-||-#9-||#10-||#11-||#12-||#13-||#14-||#15-|
 //      * NUMA node #:    |----#0----||----#1----||----#2----||----#3----||----#0----||----#1----||----#2----||----#3----|
 void G1NUMA::request_memory_on_node(void* aligned_address, size_t size_in_bytes, uint region_index) {
-  if (!is_enabled()) {
-    return;
-  }
+  assert(is_enabled(), "must be, check before");
 
   if (size_in_bytes == 0) {
     return;

src/hotspot/share/gc/g1/g1RegionToSpaceMapper.cpp

@@ -96,13 +96,15 @@ class G1RegionsLargerThanCommitSizeMapper : public G1RegionToSpaceMapper {
     const size_t start_page = (size_t)start_idx * _pages_per_region;
     const size_t size_in_pages = num_regions * _pages_per_region;
     bool zero_filled = _storage.commit(start_page, size_in_pages);
-    if (_memory_tag == mtJavaHeap) {
+
+    if (should_distribute_across_numa_nodes()) {
       for (uint region_index = start_idx; region_index < start_idx + num_regions; region_index++ ) {
         void* address = _storage.page_start(region_index * _pages_per_region);
         size_t size_in_bytes = _storage.page_size() * _pages_per_region;
         G1NUMA::numa()->request_memory_on_node(address, size_in_bytes, region_index);
       }
     }
+
     if (AlwaysPreTouch) {
       _storage.pretouch(start_page, size_in_pages, pretouch_workers);
     }
@@ -148,15 +150,62 @@ class G1RegionsSmallerThanCommitSizeMapper : public G1RegionToSpaceMapper {
     return _region_commit_map.find_first_set_bit(region, region_limit) != region_limit;
   }
 
-  void numa_request_on_node(size_t page_idx) {
-    if (_memory_tag == mtJavaHeap) {
-      uint region = (uint)(page_idx * _regions_per_page);
-      void* address = _storage.page_start(page_idx);
-      size_t size_in_bytes = _storage.page_size();
-      G1NUMA::numa()->request_memory_on_node(address, size_in_bytes, region);
+  bool commit_pages(size_t start_page, size_t size_in_pages) {
+    bool result = _storage.commit(start_page, size_in_pages);
+
+    if (should_distribute_across_numa_nodes()) {
+      for (size_t page = start_page; page < start_page + size_in_pages; page++) {
+        uint region = checked_cast<uint>(page * _regions_per_page);
+        void* address = _storage.page_start(page);
+        size_t size_in_bytes = _storage.page_size();
+        G1NUMA::numa()->request_memory_on_node(address, size_in_bytes, region);
+      }
     }
+    return result;
   }
 
+  // Given increasing integers, applies the method OnNewRange(size_t range_start, size_t range_size)
+  // in the constructor all ranges of consecutive indexes. Consecutive means that
+  // the difference between integers is 1.
+  // Call apply_last() to cover the last range.
+  template <typename OnNewRange>
+  class RangeApplicator {
+    size_t const NoIndex = SIZE_MAX;
+
+    size_t _cur_start;
+    size_t _cur_end;
+
+    OnNewRange _on_new_range;
+
+  public:
+    RangeApplicator(OnNewRange on_new_range) : _cur_start(NoIndex), _cur_end(NoIndex), _on_new_range(on_new_range) { }
+    ~RangeApplicator() {
+      assert(_cur_start == NoIndex, "must be, missing application of lambda");
+      assert(_cur_end == NoIndex, "must be, missing application of lambda");
+    }
+
+    void next_value(size_t index) {
+      assert(_cur_end == NoIndex || _cur_end < index, "Given indexes must be ascending");
+
+      if (_cur_start == NoIndex) {
+        _cur_start = _cur_end = index;
+      } else if (_cur_end + 1 != index) {
+        _on_new_range(_cur_start, _cur_end - _cur_start + 1);
+        _cur_start = _cur_end = index;
+      } else {
+        _cur_end = index;
+      }
+    }
+
+    void apply_last() {
+      if (_cur_start != NoIndex) {
+        assert(_cur_end != NoIndex, "must be");
+        _on_new_range(_cur_start, _cur_end - _cur_start + 1);
+        _cur_start = _cur_end = NoIndex;
+      }
+    }
+  };
+
  public:
   G1RegionsSmallerThanCommitSizeMapper(ReservedSpace rs,
                                        size_t actual_size,
@@ -190,6 +239,13 @@ class G1RegionsSmallerThanCommitSizeMapper : public G1RegionToSpaceMapper {
     // Concurrent operations might operate on regions sharing the same
     // underlying OS page. See lock declaration for more details.
     {
+      auto commit_range = [&](size_t page_start, size_t size_in_pages) {
+        if (!commit_pages(page_start, size_in_pages)) {
+          all_zero_filled = false;
+        }
+      };
+      RangeApplicator range(commit_range);
+
       MutexLocker ml(&_lock, Mutex::_no_safepoint_check_flag);
       for (size_t page = start_page; page <= end_page; page++) {
         if (!is_page_committed(page)) {
@@ -199,18 +255,13 @@ class G1RegionsSmallerThanCommitSizeMapper : public G1RegionToSpaceMapper {
           }
           num_committed++;
 
-          if (!_storage.commit(page, 1)) {
-            // Found dirty region during commit.
-            all_zero_filled = false;
-          }
-
-          // Move memory to correct NUMA node for the heap.
-          numa_request_on_node(page);
+          range.next_value(page);
         } else {
           // Page already committed.
           all_zero_filled = false;
         }
       }
+      range.apply_last();
 
       // Update the commit map for the given range. Not using the par_set_range
       // since updates to _region_commit_map for this mapper is protected by _lock.
@@ -233,6 +284,11 @@ class G1RegionsSmallerThanCommitSizeMapper : public G1RegionToSpaceMapper {
     size_t start_page = region_idx_to_page_idx(start_idx);
     size_t end_page = region_idx_to_page_idx(region_limit - 1);
 
+    auto uncommit_range = [&](size_t page_start, size_t size_in_pages) {
+      _storage.uncommit(page_start, size_in_pages);
+    };
+    RangeApplicator r(uncommit_range);
+
     // Concurrent operations might operate on regions sharing the same
     // underlying OS page. See lock declaration for more details.
     MutexLocker ml(&_lock, Mutex::_no_safepoint_check_flag);
@@ -245,9 +301,10 @@ class G1RegionsSmallerThanCommitSizeMapper : public G1RegionToSpaceMapper {
       // the page is still marked as committed after the clear we should
       // not uncommit it.
       if (!is_page_committed(page)) {
-        _storage.uncommit(page, 1);
+        r.next_value(page);
       }
     }
+    r.apply_last();
   }
 };
 
@@ -257,6 +314,10 @@ void G1RegionToSpaceMapper::fire_on_commit(uint start_idx, size_t num_regions, b
   }
 }
 
+bool G1RegionToSpaceMapper::should_distribute_across_numa_nodes() const {
+  return _memory_tag == mtJavaHeap && G1NUMA::numa()->is_enabled();
+}
+
 G1RegionToSpaceMapper* G1RegionToSpaceMapper::create_mapper(ReservedSpace rs,
                                                             size_t actual_size,
                                                             size_t page_size,

src/hotspot/share/gc/g1/g1RegionToSpaceMapper.hpp

@@ -58,6 +58,8 @@ class G1RegionToSpaceMapper : public CHeapObj<mtGC> {
   G1RegionToSpaceMapper(ReservedSpace rs, size_t used_size, size_t page_size, size_t region_granularity, size_t commit_factor, MemTag mem_tag);
 
   void fire_on_commit(uint start_idx, size_t num_regions, bool zero_filled);
+
+  bool should_distribute_across_numa_nodes() const;
  public:
   MemRegion reserved() { return _storage.reserved(); }