8293972: runtime/NMT/NMTInitializationTest.java#default_long-off failed with "Suspiciously long bucket chains in lookup table."

src/hotspot/share/runtime/arguments.cpp

@@ -58,6 +58,7 @@
 #include "runtime/synchronizer.hpp"
 #include "runtime/vm_version.hpp"
 #include "services/management.hpp"
+#include "services/memTracker.hpp"
 #include "services/nmtCommon.hpp"
 #include "utilities/align.hpp"
 #include "utilities/debug.hpp"
@@ -333,6 +334,29 @@ bool Arguments::is_internal_module_property(const char* property) {
   return false;
 }
 
+ccstr Arguments::process_nmt_property(JavaVMInitArgs* args) {
+  ccstr nmt = NativeMemoryTracking;
+
+  // Find out user NMT setting
+  for (int index = 0; index < args->nOptions; index++) {
+    const JavaVMOption* option = args->options + index;
+    const char* tail;
+    if (match_option(option, "-XX:NativeMemoryTracking=", &tail)) {
+      nmt = tail;
+    }
+  }
+
+  // Verify NMT arguments
+  const NMT_TrackingLevel level = NMTUtil::parse_tracking_level(nmt);
+  if (level == NMT_unknown) {
+    jio_fprintf(defaultStream::error_stream(),
+                "Syntax error, expecting -XX:NativeMemoryTracking=[off|summary|detail]", nullptr);
+    return nullptr;
+  }
+
+  return nmt;
+}
+
 // Process java launcher properties.
 void Arguments::process_sun_java_launcher_properties(JavaVMInitArgs* args) {
   // See if sun.java.launcher or sun.java.launcher.is_altjvm is defined.

src/hotspot/share/runtime/arguments.hpp

@@ -449,6 +449,9 @@ class Arguments : AllStatic {
   static const char* GetSharedArchivePath() { return SharedArchivePath; }
   static const char* GetSharedDynamicArchivePath() { return SharedDynamicArchivePath; }
   static size_t default_SharedBaseAddress() { return _default_SharedBaseAddress; }
+
+  static ccstr process_nmt_property(JavaVMInitArgs* args);
+
   // Java launcher properties
   static void process_sun_java_launcher_properties(JavaVMInitArgs* args);
 

src/hotspot/share/runtime/os.cpp

@@ -674,12 +674,7 @@ void* os::realloc(void *memblock, size_t size, MEMFLAGS flags) {
 }
 
 void* os::realloc(void *memblock, size_t size, MEMFLAGS memflags, const NativeCallStack& stack) {
-
-  // Special handling for NMT preinit phase before arguments are parsed
   void* rc = nullptr;
-  if (NMTPreInit::handle_realloc(&rc, memblock, size, memflags)) {
-    return rc;
-  }
 
   if (memblock == nullptr) {
     return os::malloc(size, memflags, stack);

src/hotspot/share/runtime/threads.cpp

@@ -415,6 +415,11 @@ jint Threads::create_vm(JavaVMInitArgs* args, bool* canTryAgain) {
   // Check version
   if (!is_supported_jni_version(args->version)) return JNI_EVERSION;
 
+  // Initialize the NMT memory tracking as early as possible
+  ccstr nmt = Arguments::process_nmt_property(args);
+  if (nmt == nullptr) return JNI_ERR;
+  MemTracker::initialize(nmt);
+
   // Initialize library-based TLS
   ThreadLocalStorage::init();
 
@@ -450,8 +455,8 @@ jint Threads::create_vm(JavaVMInitArgs* args, bool* canTryAgain) {
   jint parse_result = Arguments::parse(args);
   if (parse_result != JNI_OK) return parse_result;
 
-  // Initialize NMT right after argument parsing to keep the pre-NMT-init window small.
-  MemTracker::initialize();
+  // Verify NMT flag(s) and log the initial state to the ouput
+  MemTracker::post_initialize();
 
   os::init_before_ergo();
 

src/hotspot/share/services/mallocTracker.cpp

@@ -83,7 +83,6 @@ void MallocMemorySummary::initialize() {
   assert(sizeof(_snapshot) >= sizeof(MallocMemorySnapshot), "Sanity Check");
   // Uses placement new operator to initialize static area.
   ::new ((void*)_snapshot)MallocMemorySnapshot();
-  MallocLimitHandler::initialize(MallocLimit);
 }
 
 bool MallocMemorySummary::total_limit_reached(size_t s, size_t so_far, const malloclimit* limit) {

src/hotspot/share/services/memTracker.cpp

@@ -52,14 +52,14 @@ NMT_TrackingLevel MemTracker::_tracking_level = NMT_unknown;
 
 MemBaseline MemTracker::_baseline;
 
-void MemTracker::initialize() {
+void MemTracker::initialize(ccstr value) {
   bool rc = true;
   assert(_tracking_level == NMT_unknown, "only call once");
 
-  NMT_TrackingLevel level = NMTUtil::parse_tracking_level(NativeMemoryTracking);
+  NMT_TrackingLevel level = NMTUtil::parse_tracking_level(value);
   // Should have been validated before in arguments.cpp
   assert(level == NMT_off || level == NMT_summary || level == NMT_detail,
-         "Invalid setting for NativeMemoryTracking (%s)", NativeMemoryTracking);
+         "Invalid setting for NativeMemoryTracking (%s)", value);
 
   // Memory type is encoded into tracking header as a byte field,
   // make sure that we don't overflow it.
@@ -74,15 +74,21 @@ void MemTracker::initialize() {
       log_warning(nmt)("NMT initialization failed. NMT disabled.");
       return;
     }
-  } else {
-    if (MallocLimit != nullptr) {
-      warning("MallocLimit will be ignored since NMT is disabled.");
-    }
   }
 
   NMTPreInit::pre_to_post(level == NMT_off);
 
   _tracking_level = level;
+}
+
+void MemTracker::post_initialize() {
+  if (_tracking_level == NMT_off) {
+    if (MallocLimit != nullptr) {
+      warning("MallocLimit will be ignored since NMT is disabled.");
+    }
+  } else {
+    MallocLimitHandler::initialize(MallocLimit);
+  }
 
   // Log state right after NMT initialization
   if (log_is_enabled(Info, nmt)) {

src/hotspot/share/services/memTracker.hpp

@@ -73,7 +73,8 @@ class MemTracker : AllStatic {
   // Initializes NMT to whatever -XX:NativeMemoryTracking says.
   //  - Can only be called once.
   //  - NativeMemoryTracking must be validated beforehand.
-  static void initialize();
+  static void initialize(ccstr value);
+  static void post_initialize();
 
   // Returns true if NMT had been initialized.
   static bool is_initialized()  {

src/hotspot/share/services/nmtPreInit.cpp

@@ -34,7 +34,6 @@
 // Obviously we cannot use os::malloc for any dynamic allocation during pre-NMT-init, so we must use
 // raw malloc; to make this very clear, wrap them.
 static void* raw_malloc(size_t s)               { ALLOW_C_FUNCTION(::malloc, return ::malloc(s);) }
-static void* raw_realloc(void* old, size_t s)   { ALLOW_C_FUNCTION(::realloc, return ::realloc(old, s);) }
 static void  raw_free(void* p)                  { ALLOW_C_FUNCTION(::free, ::free(p);) }
 
 // To keep matters simple we just raise a fatal error on OOM. Since preinit allocation
@@ -49,14 +48,6 @@ static void* raw_checked_malloc(size_t s) {
   return p;
 }
 
-static void* raw_checked_realloc(void* old, size_t s) {
-  void* p = raw_realloc(old, s);
-  if (p == nullptr) {
-    vm_exit_out_of_memory(s, OOM_MALLOC_ERROR, "VM early initialization phase");
-  }
-  return p;
-}
-
 // --------- NMTPreInitAllocation --------------
 
 void* NMTPreInitAllocation::operator new(size_t count) {
@@ -73,14 +64,6 @@ NMTPreInitAllocation* NMTPreInitAllocation::do_alloc(size_t payload_size) {
   return a;
 }
 
-NMTPreInitAllocation* NMTPreInitAllocation::do_reallocate(NMTPreInitAllocation* a, size_t new_payload_size) {
-  assert(a->next == nullptr, "unhang from map first");
-  void* new_payload = raw_checked_realloc(a->payload, new_payload_size);
-  NMTPreInitAllocation* a2 = new NMTPreInitAllocation(new_payload_size, new_payload);
-  delete a;
-  return a2;
-}
-
 void NMTPreInitAllocation::do_free(NMTPreInitAllocation* a) {
   assert(a->next == nullptr, "unhang from map first");
   raw_free(a->payload);
@@ -150,12 +133,6 @@ void NMTPreInitAllocationTable::print_map(outputStream* st) const {
 }
 
 void NMTPreInitAllocationTable::verify() const {
-  // This verifies the buildup of the lookup table, including the load and the chain lengths.
-  // We should see chain lens of 0-1 under normal conditions. Under artificial conditions
-  // (20000 VM args) we should see maybe 6-7. From a certain length on we can be sure something
-  // is broken.
-  const int longest_acceptable_chain_len = 30;
-  int num_chains_too_long = 0;
   for (index_t i = 0; i < table_size; i++) {
     int len = 0;
     for (const NMTPreInitAllocation* a = _entries[i]; a != nullptr; a = a->next) {
@@ -172,13 +149,6 @@ void NMTPreInitAllocationTable::verify() const {
         }
       }
     }
-    if (len > longest_acceptable_chain_len) {
-      num_chains_too_long++;
-    }
-  }
-  if (num_chains_too_long > 0) {
-    assert(false, "NMT preinit lookup table degenerated (%d/%d chains longer than %d)",
-                  num_chains_too_long, table_size, longest_acceptable_chain_len);
   }
 }
 #endif // ASSERT
@@ -189,7 +159,6 @@ NMTPreInitAllocationTable* NMTPreInit::_table = nullptr;
 
 // Some statistics
 unsigned NMTPreInit::_num_mallocs_pre = 0;
-unsigned NMTPreInit::_num_reallocs_pre = 0;
 unsigned NMTPreInit::_num_frees_pre = 0;
 
 void NMTPreInit::create_table() {
@@ -214,7 +183,7 @@ void NMTPreInit::pre_to_post(bool nmt_off) {
     // can be handled directly by os::realloc or os::free.
     // We also can get rid of the lookup table.
     // Note that we deliberately leak the headers (NMTPreInitAllocation) in order to speed up startup.
-    // That may leak about 12KB of memory for ~500 surviving pre-init allocations, which is a typical
+    // That may leak about 64 bytes of memory for 2 surviving pre-init allocations, which is a typical
     // number. This is a compromise to keep the coding simple and startup time short. It could very
     // easily improved by keeping a header pool, similar to metaspace ChunkHeaderPool. But since NMTPreInit
     // had been critizised as "too complicated", I try to keep things short and simple.
@@ -228,8 +197,7 @@ void NMTPreInit::verify() {
   if (_table != nullptr) {
     _table->verify();
   }
-  assert(_num_reallocs_pre <= _num_mallocs_pre &&
-         _num_frees_pre <= _num_mallocs_pre, "stats are off");
+  assert(_num_frees_pre <= _num_mallocs_pre, "stats are off");
 }
 #endif // ASSERT
 
@@ -238,6 +206,5 @@ void NMTPreInit::print_state(outputStream* st) {
     _table->print_state(st);
     st->cr();
   }
-  st->print_cr("pre-init mallocs: %u, pre-init reallocs: %u, pre-init frees: %u",
-               _num_mallocs_pre, _num_reallocs_pre, _num_frees_pre);
+  st->print_cr("pre-init mallocs: %u, pre-init frees: %u", _num_mallocs_pre, _num_frees_pre);
 }