8320061: [nmt] Multiple issues with peak accounting

Reviewed-by: jsjolen, mbaesken

Author: tstuefe

src/hotspot/share/nmt/memReporter.cpp

@@ -55,7 +55,7 @@ void MemReporterBase::print_total(size_t reserved, size_t committed, size_t peak
   output()->print("reserved=" SIZE_FORMAT "%s, committed=" SIZE_FORMAT "%s",
     amount_in_current_scale(reserved), scale, amount_in_current_scale(committed), scale);
   if (peak != 0) {
-    output()->print(", largest_committed=" SIZE_FORMAT "%s", amount_in_current_scale(peak), scale);
+    output()->print(", peak=" SIZE_FORMAT "%s", amount_in_current_scale(peak), scale);
   }
 }
 
@@ -93,9 +93,15 @@ void MemReporterBase::print_malloc(const MemoryCounter* c, MEMFLAGS flag) const
 }
 
 void MemReporterBase::print_virtual_memory(size_t reserved, size_t committed, size_t peak) const {
+  outputStream* out = output();
   const char* scale = current_scale();
-  output()->print("(mmap: reserved=" SIZE_FORMAT "%s, committed=" SIZE_FORMAT "%s, largest_committed=" SIZE_FORMAT "%s)",
-    amount_in_current_scale(reserved), scale, amount_in_current_scale(committed), scale, amount_in_current_scale(peak), scale);
+  out->print("(mmap: reserved=" SIZE_FORMAT "%s, committed=" SIZE_FORMAT "%s, ",
+    amount_in_current_scale(reserved), scale, amount_in_current_scale(committed), scale);
+  if (peak == committed) {
+    out->print_raw("at peak)");
+  } else {
+    out->print("peak=" SIZE_FORMAT "%s)", amount_in_current_scale(peak), scale);
+  }
 }
 
 void MemReporterBase::print_malloc_line(const MemoryCounter* c) const {
@@ -204,74 +210,79 @@ void MemSummaryReporter::report_summary_of_type(MEMFLAGS flag,
     committed_amount += _malloc_snapshot->malloc_overhead();
   }
 
-  if (amount_in_current_scale(reserved_amount) > 0) {
-    outputStream* out   = output();
-    const char*   scale = current_scale();
-    out->print("-%26s (", NMTUtil::flag_to_name(flag));
-    print_total(reserved_amount, committed_amount);
+  // Omit printing if the current reserved value as well as all historical peaks (malloc, mmap committed, arena)
+  // fall below scale threshold
+  const size_t pk_vm = virtual_memory->peak_size();
+  const size_t pk_malloc = malloc_memory->malloc_peak_size();
+  const size_t pk_arena = malloc_memory->arena_peak_size();
+
+  if (amount_in_current_scale(MAX4(reserved_amount, pk_vm, pk_malloc, pk_arena)) == 0) {
+    return;
+  }
+
+  outputStream* out   = output();
+  const char*   scale = current_scale();
+  out->print("-%26s (", NMTUtil::flag_to_name(flag));
+  print_total(reserved_amount, committed_amount);
 #if INCLUDE_CDS
-    if (flag == mtClassShared) {
-        size_t read_only_bytes = FileMapInfo::readonly_total();
-      output()->print(", readonly=" SIZE_FORMAT "%s",
-                      amount_in_current_scale(read_only_bytes), scale);
-    }
+  if (flag == mtClassShared) {
+      size_t read_only_bytes = FileMapInfo::readonly_total();
+    output()->print(", readonly=" SIZE_FORMAT "%s",
+                    amount_in_current_scale(read_only_bytes), scale);
+  }
 #endif
-    out->print_cr(")");
+  out->print_cr(")");
 
-    if (flag == mtClass) {
-      // report class count
-      out->print_cr("%27s (classes #" SIZE_FORMAT ")",
-        " ", (_instance_class_count + _array_class_count));
-      out->print_cr("%27s (  instance classes #" SIZE_FORMAT ", array classes #" SIZE_FORMAT ")",
-        " ", _instance_class_count, _array_class_count);
-    } else if (flag == mtThread) {
-      if (ThreadStackTracker::track_as_vm()) {
-        const VirtualMemory* thread_stack_usage =
-         _vm_snapshot->by_type(mtThreadStack);
-        // report thread count
-        out->print_cr("%27s (threads #" SIZE_FORMAT ")", " ", ThreadStackTracker::thread_count());
-        out->print("%27s (stack: ", " ");
-        print_total(thread_stack_usage->reserved(), thread_stack_usage->committed(), thread_stack_usage->peak_size());
-      } else {
-        MallocMemory* thread_stack_memory = _malloc_snapshot->by_type(mtThreadStack);
-        const char* scale = current_scale();
-        // report thread count
-        out->print_cr("%27s (threads #" SIZE_FORMAT ")", " ", thread_stack_memory->malloc_count());
-        out->print("%27s (Stack: " SIZE_FORMAT "%s", " ",
-          amount_in_current_scale(thread_stack_memory->malloc_size()), scale);
-      }
-      out->print_cr(")");
+  if (flag == mtClass) {
+    // report class count
+    out->print_cr("%27s (classes #" SIZE_FORMAT ")",
+      " ", (_instance_class_count + _array_class_count));
+    out->print_cr("%27s (  instance classes #" SIZE_FORMAT ", array classes #" SIZE_FORMAT ")",
+      " ", _instance_class_count, _array_class_count);
+  } else if (flag == mtThread) {
+    if (ThreadStackTracker::track_as_vm()) {
+      const VirtualMemory* thread_stack_usage =
+       _vm_snapshot->by_type(mtThreadStack);
+      // report thread count
+      out->print_cr("%27s (threads #" SIZE_FORMAT ")", " ", ThreadStackTracker::thread_count());
+      out->print("%27s (stack: ", " ");
+      print_total(thread_stack_usage->reserved(), thread_stack_usage->committed(), thread_stack_usage->peak_size());
+    } else {
+      MallocMemory* thread_stack_memory = _malloc_snapshot->by_type(mtThreadStack);
+      const char* scale = current_scale();
+      // report thread count
+      out->print_cr("%27s (threads #" SIZE_FORMAT ")", " ", thread_stack_memory->malloc_count());
+      out->print("%27s (Stack: " SIZE_FORMAT "%s", " ",
+        amount_in_current_scale(thread_stack_memory->malloc_size()), scale);
     }
+    out->print_cr(")");
+  }
 
-     // report malloc'd memory
-    if (amount_in_current_scale(malloc_memory->malloc_size()) > 0
-        || amount_in_current_scale(malloc_memory->malloc_peak_size()) > 0) {
-      print_malloc_line(malloc_memory->malloc_counter());
-    }
+   // report malloc'd memory
+  if (amount_in_current_scale(MAX2(malloc_memory->malloc_size(), pk_malloc)) > 0) {
+    print_malloc_line(malloc_memory->malloc_counter());
+  }
 
-    if (amount_in_current_scale(virtual_memory->reserved()) > 0
-        DEBUG_ONLY(|| amount_in_current_scale(virtual_memory->peak_size()) > 0)) {
-      print_virtual_memory_line(virtual_memory->reserved(), virtual_memory->committed(), virtual_memory->peak_size());
-    }
+  if (amount_in_current_scale(MAX2(virtual_memory->reserved(), pk_vm)) > 0) {
+    print_virtual_memory_line(virtual_memory->reserved(), virtual_memory->committed(), virtual_memory->peak_size());
+  }
 
-    if (amount_in_current_scale(malloc_memory->arena_size()) > 0
-        DEBUG_ONLY(|| amount_in_current_scale(malloc_memory->arena_peak_size()) > 0)) {
-      print_arena_line(malloc_memory->arena_counter());
-    }
+  if (amount_in_current_scale(MAX2(malloc_memory->arena_size(), pk_arena)) > 0) {
+    print_arena_line(malloc_memory->arena_counter());
+  }
 
-    if (flag == mtNMT &&
-      amount_in_current_scale(_malloc_snapshot->malloc_overhead()) > 0) {
-      out->print_cr("%27s (tracking overhead=" SIZE_FORMAT "%s)", " ",
-        amount_in_current_scale(_malloc_snapshot->malloc_overhead()), scale);
-    } else if (flag == mtClass) {
-      // Metadata information
-      report_metadata(Metaspace::NonClassType);
-      if (Metaspace::using_class_space()) {
-        report_metadata(Metaspace::ClassType);
-      }
+  if (flag == mtNMT &&
+    amount_in_current_scale(_malloc_snapshot->malloc_overhead()) > 0) {
+    out->print_cr("%27s (tracking overhead=" SIZE_FORMAT "%s)", " ",
+      amount_in_current_scale(_malloc_snapshot->malloc_overhead()), scale);
+  } else if (flag == mtClass) {
+    // Metadata information
+    report_metadata(Metaspace::NonClassType);
+    if (Metaspace::using_class_space()) {
+      report_metadata(Metaspace::ClassType);
     }
-    out->print_cr(" ");
   }
+  out->print_cr(" ");
 }
 
 void MemSummaryReporter::report_metadata(Metaspace::MetadataType type) const {
@@ -321,9 +332,8 @@ int MemDetailReporter::report_malloc_sites() {
   const MallocSite* malloc_site;
   int num_omitted = 0;
   while ((malloc_site = malloc_itr.next()) != nullptr) {
-    // Don't report if site has never allocated less than one unit of whatever our scale is
-    if (scale() > 1 && amount_in_current_scale(malloc_site->size()) == 0
-                       DEBUG_ONLY(&& amount_in_current_scale(malloc_site->peak_size()) == 0)) {
+    // Omit printing if the current value and the historic peak value both fall below the reporting scale threshold
+    if (amount_in_current_scale(MAX2(malloc_site->size(), malloc_site->peak_size())) == 0) {
       num_omitted ++;
       continue;
     }
@@ -353,8 +363,10 @@ int MemDetailReporter::report_virtual_memory_allocation_sites()  {
     if (virtual_memory_site->reserved() == 0) {
       continue;
     }
-    // Don't report if site has reserved less than one unit of whatever our scale is
-    if (scale() > 1 && amount_in_current_scale(virtual_memory_site->reserved()) == 0) {
+    // Omit printing if the current value and the historic peak value both fall below the
+    // reporting scale threshold
+    if (amount_in_current_scale(MAX2(virtual_memory_site->reserved(),
+                                     virtual_memory_site->peak_size())) == 0) {
       num_omitted++;
       continue;
     }
@@ -386,7 +398,16 @@ void MemDetailReporter::report_virtual_memory_map() {
 void MemDetailReporter::report_virtual_memory_region(const ReservedMemoryRegion* reserved_rgn) {
   assert(reserved_rgn != nullptr, "null pointer");
 
-  // Don't report if size is too small
+  // We don't bother about reporting peaks here.
+  // That is because peaks - in the context of virtual memory, peak of committed areas - make little sense
+  // when we report *by region*, which are identified by their location in memory. There is a philosophical
+  // question about identity here: e.g. a committed region that has been split into three regions by
+  // uncommitting a middle section of it, should that still count as "having peaked" before the split? If
+  // yes, which of the three new regions would be the spiritual successor? Rather than introducing more
+  // complexity, we avoid printing peaks altogether. Note that peaks should still be printed when reporting
+  // usage *by callsite*.
+
+  // Don't report if size is too small.
   if (amount_in_current_scale(reserved_rgn->size()) == 0) return;
 
   outputStream* out = output();

src/hotspot/share/nmt/virtualMemoryTracker.cpp

@@ -34,7 +34,6 @@
 
 size_t VirtualMemorySummary::_snapshot[CALC_OBJ_SIZE_IN_TYPE(VirtualMemorySnapshot, size_t)];
 
-#ifdef ASSERT
 void VirtualMemory::update_peak(size_t size) {
   size_t peak_sz = peak_size();
   while (peak_sz < size) {
@@ -46,7 +45,6 @@ void VirtualMemory::update_peak(size_t size) {
     }
   }
 }
-#endif // ASSERT
 
 void VirtualMemorySummary::initialize() {
   assert(sizeof(_snapshot) >= sizeof(VirtualMemorySnapshot), "Sanity Check");

src/hotspot/share/nmt/virtualMemoryTracker.hpp

@@ -42,21 +42,17 @@ class VirtualMemory {
   size_t     _reserved;
   size_t     _committed;
 
-#ifdef ASSERT
   volatile size_t _peak_size;
   void update_peak(size_t size);
-#endif // ASSERT
 
  public:
-  VirtualMemory() : _reserved(0), _committed(0) {
-    DEBUG_ONLY(_peak_size  = 0;)
-  }
+  VirtualMemory() : _reserved(0), _committed(0), _peak_size(0) {}
 
   inline void reserve_memory(size_t sz) { _reserved += sz; }
   inline void commit_memory (size_t sz) {
     _committed += sz;
-    DEBUG_ONLY(update_peak(sz);)
     assert(_committed <= _reserved, "Sanity check");
+    update_peak(_committed);
   }
 
   inline void release_memory (size_t sz) {
@@ -72,7 +68,7 @@ class VirtualMemory {
   inline size_t reserved()  const { return _reserved;  }
   inline size_t committed() const { return _committed; }
   inline size_t peak_size() const {
-    return DEBUG_ONLY(Atomic::load(&_peak_size)) NOT_DEBUG(0);
+    return Atomic::load(&_peak_size);
   }
 };
 
@@ -85,10 +81,9 @@ class VirtualMemoryAllocationSite : public AllocationSite {
 
   inline void reserve_memory(size_t sz)  { _c.reserve_memory(sz);  }
   inline void commit_memory (size_t sz)  { _c.commit_memory(sz);   }
-  inline void uncommit_memory(size_t sz) { _c.uncommit_memory(sz); }
-  inline void release_memory(size_t sz)  { _c.release_memory(sz);  }
   inline size_t reserved() const  { return _c.reserved(); }
   inline size_t committed() const { return _c.committed(); }
+  inline size_t peak_size() const { return _c.peak_size(); }
 };
 
 class VirtualMemorySummary;

test/hotspot/jtreg/runtime/NMT/HugeArenaTracking.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2019, 2020, Red Hat, Inc. All rights reserved.
+ * Copyright (c) 2019, 2023, Red Hat, Inc. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -31,59 +31,81 @@
  *          java.management
  * @build jdk.test.whitebox.WhiteBox
  * @run driver jdk.test.lib.helpers.ClassFileInstaller jdk.test.whitebox.WhiteBox
- * @run main/othervm -Xbootclasspath/a:. -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI -XX:NativeMemoryTracking=detail HugeArenaTracking
+ * @run main/othervm -Xbootclasspath/a:. -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI -XX:NativeMemoryTracking=summary HugeArenaTracking
  */
 
 import java.util.Random;
 import jdk.test.lib.process.ProcessTools;
 import jdk.test.lib.process.OutputAnalyzer;
-import jdk.test.lib.JDKToolFinder;
 import jdk.test.lib.Utils;
 import jdk.test.whitebox.WhiteBox;
 
 public class HugeArenaTracking {
-  private static final long GB = 1024 * 1024 * 1024;
+  private static final long MB = 1024 * 1024;
+  private static final long GB = MB * 1024;
 
   public static void main(String args[]) throws Exception {
-    OutputAnalyzer output;
     final WhiteBox wb = WhiteBox.getWhiteBox();
 
-    // Grab my own PID
-    String pid = Long.toString(ProcessTools.getProcessId());
-    ProcessBuilder pb = new ProcessBuilder();
-
     long arena1 = wb.NMTNewArena(1024);
     long arena2 = wb.NMTNewArena(1024);
 
-    // Run 'jcmd <pid> VM.native_memory summary'
-    pb.command(new String[] { JDKToolFinder.getJDKTool("jcmd"), pid, "VM.native_memory", "summary"});
-    output = new OutputAnalyzer(pb.start());
-    output.shouldContain("Test (reserved=2KB, committed=2KB)");
+    NMTTestUtils.runJcmdSummaryReportAndCheckOutput(
+            new String[] { "scale=K" },
+            new String[] { "Test (reserved=2KB, committed=2KB)",
+                           "(arena=2KB #2) (at peak)" });
 
     Random rand = Utils.getRandomInstance();
 
     // Allocate 2GB+ from arena
     long total = 0;
     while (total < 2 * GB) {
-      // Cap to 10M
-      long inc = rand.nextInt(10 * 1024 * 1024);
-      wb.NMTArenaMalloc(arena1, inc);
-      total += inc;
+      wb.NMTArenaMalloc(arena1, MB);
+      total += MB;
     }
 
-    ProcessBuilder pb2 = new ProcessBuilder();
-    // Run 'jcmd <pid> VM.native_memory summary'
-    pb2.command(new String[] { JDKToolFinder.getJDKTool("jcmd"), pid, "VM.native_memory", "summary", "scale=GB"});
-    output = new OutputAnalyzer(pb2.start());
-    output.shouldContain("Test (reserved=2GB, committed=2GB)");
+    // run a report at GB level. We should see our allocations; since they are rounded
+    // to GB, we expect an exact output match
+    NMTTestUtils.runJcmdSummaryReportAndCheckOutput(
+            new String[] { "scale=G" },
+            new String[] { "Test (reserved=2GB, committed=2GB)",
+                           "(arena=2GB #2) (at peak)" });
+
+    // Repeat at MB level; we expect the same behavior
+    NMTTestUtils.runJcmdSummaryReportAndCheckOutput(
+            new String[] { "scale=M" },
+            new String[] { "Test (reserved=2048MB, committed=2048MB)",
+                           "(arena=2048MB #2) (at peak)" });
 
     wb.NMTFreeArena(arena1);
 
-    output = new OutputAnalyzer(pb.start());
-    output.shouldContain("Test (reserved=1KB, committed=1KB)");
+    // Repeat report at GB level. Reserved should be 0 now. Current usage is 1KB, since arena2 is left, but that
+    // is below GB scale threshold, so should show up as 0.
+    NMTTestUtils.runJcmdSummaryReportAndCheckOutput(
+            new String[] { "scale=G" },
+            new String[] { "Test (reserved=0GB, committed=0GB)",
+                           "(arena=0GB #1) (peak=2GB #2)" });
+
+    // Same, for MB scale
+    NMTTestUtils.runJcmdSummaryReportAndCheckOutput(
+            new String[] { "scale=M" },
+            new String[] { "Test (reserved=0MB, committed=0MB)",
+                           "(arena=0MB #1) (peak=2048MB #2)" });
+
+    // At KB level we should see the remaining 1KB. Note that we refrain from testing peak here
+    // since the number gets fuzzy: it depends on the size of the initially allocated chunk. At MB
+    // and GB scale, these differences don't matter.
+    NMTTestUtils.runJcmdSummaryReportAndCheckOutput(
+            new String[] { "scale=K" },
+            new String[] { "Test (reserved=1KB, committed=1KB)",
+                           "(arena=1KB #1) (peak=" });
+
     wb.NMTFreeArena(arena2);
 
-    output = new OutputAnalyzer(pb.start());
-    output.shouldNotContain("Test (reserved");
+    // Everything free'd, current usage 0, peak should be preserved.
+    NMTTestUtils.runJcmdSummaryReportAndCheckOutput(
+            new String[] { "scale=G" },
+            new String[] { "Test (reserved=0GB, committed=0GB)",
+                           "(arena=0GB #0) (peak=2GB #2)" });
   }
 }