8367231: [BACKOUT] JDK-8366363: MemBaseline accesses VMT without using lock

Reviewed-by: kbarrett, dholmes

Author: jdksjolen

src/hotspot/share/nmt/memBaseline.cpp

@@ -27,7 +27,8 @@
 #include "memory/metaspaceUtils.hpp"
 #include "nmt/memBaseline.hpp"
 #include "nmt/memTracker.hpp"
-#include "nmt/regionsTree.inline.hpp"
+#include "runtime/javaThread.hpp"
+#include "runtime/safepoint.hpp"
 
 /*
  * Sizes are sorted in descenting order for reporting
@@ -103,6 +104,38 @@ class MallocAllocationSiteWalker : public MallocSiteWalker {
   }
 };
 
+// Walk all virtual memory regions for baselining
+class VirtualMemoryAllocationWalker : public VirtualMemoryWalker {
+ private:
+  typedef LinkedListImpl<ReservedMemoryRegion, AnyObj::C_HEAP, mtNMT,
+                         AllocFailStrategy::RETURN_NULL> EntryList;
+  EntryList _virtual_memory_regions;
+  DEBUG_ONLY(address _last_base;)
+ public:
+  VirtualMemoryAllocationWalker() {
+    DEBUG_ONLY(_last_base = nullptr);
+  }
+
+  bool do_allocation_site(const ReservedMemoryRegion* rgn)  {
+    assert(rgn->base() >= _last_base, "region unordered?");
+    DEBUG_ONLY(_last_base = rgn->base());
+    if (rgn->size() > 0) {
+      if (_virtual_memory_regions.add(*rgn) != nullptr) {
+        return true;
+      } else {
+        return false;
+      }
+    } else {
+      // Ignore empty sites.
+      return true;
+    }
+  }
+
+  LinkedList<ReservedMemoryRegion>* virtual_memory_allocations() {
+    return &_virtual_memory_regions;
+  }
+};
+
 void MemBaseline::baseline_summary() {
   MallocMemorySummary::snapshot(&_malloc_memory_snapshot);
   VirtualMemorySummary::snapshot(&_virtual_memory_snapshot);
@@ -125,16 +158,15 @@ bool MemBaseline::baseline_allocation_sites() {
   // The malloc sites are collected in size order
   _malloc_sites_order = by_size;
 
-  assert(_vma_allocations == nullptr, "must");
-
-  {
-    MemTracker::NmtVirtualMemoryLocker locker;
-    _vma_allocations = new (mtNMT, std::nothrow) RegionsTree(*VirtualMemoryTracker::Instance::tree());
-    if (_vma_allocations == nullptr)  {
-      return false;
-    }
+  // Virtual memory allocation sites
+  VirtualMemoryAllocationWalker virtual_memory_walker;
+  if (!VirtualMemoryTracker::Instance::walk_virtual_memory(&virtual_memory_walker)) {
+    return false;
   }
 
+  // Virtual memory allocations are collected in call stack order
+  _virtual_memory_allocations.move(virtual_memory_walker.virtual_memory_allocations());
+
   if (!aggregate_virtual_memory_allocation_sites()) {
     return false;
   }
@@ -170,28 +202,20 @@ int compare_allocation_site(const VirtualMemoryAllocationSite& s1,
 bool MemBaseline::aggregate_virtual_memory_allocation_sites() {
   SortedLinkedList<VirtualMemoryAllocationSite, compare_allocation_site> allocation_sites;
 
+  VirtualMemoryAllocationIterator itr = virtual_memory_allocations();
+  const ReservedMemoryRegion* rgn;
   VirtualMemoryAllocationSite* site;
-  bool failed_oom = false;
-  _vma_allocations->visit_reserved_regions([&](ReservedMemoryRegion& rgn) {
-    VirtualMemoryAllocationSite tmp(*rgn.call_stack(), rgn.mem_tag());
+  while ((rgn = itr.next()) != nullptr) {
+    VirtualMemoryAllocationSite tmp(*rgn->call_stack(), rgn->mem_tag());
     site = allocation_sites.find(tmp);
     if (site == nullptr) {
       LinkedListNode<VirtualMemoryAllocationSite>* node =
         allocation_sites.add(tmp);
-      if (node == nullptr) {
-        failed_oom = true;
-        return false;
-      }
+      if (node == nullptr) return false;
       site = node->data();
     }
-    site->reserve_memory(rgn.size());
-
-    site->commit_memory(_vma_allocations->committed_size(rgn));
-    return true;
-  });
-
-  if (failed_oom) {
-    return false;
+    site->reserve_memory(rgn->size());
+    site->commit_memory(VirtualMemoryTracker::Instance::committed_size(rgn));
   }
 
   _virtual_memory_sites.move(&allocation_sites);

src/hotspot/share/nmt/memBaseline.hpp

@@ -35,6 +35,7 @@
 
 typedef LinkedListIterator<MallocSite>                   MallocSiteIterator;
 typedef LinkedListIterator<VirtualMemoryAllocationSite>  VirtualMemorySiteIterator;
+typedef LinkedListIterator<ReservedMemoryRegion>         VirtualMemoryAllocationIterator;
 
 /*
  * Baseline a memory snapshot
@@ -70,7 +71,7 @@ class MemBaseline {
   LinkedListImpl<MallocSite>                  _malloc_sites;
 
   // All virtual memory allocations
-  RegionsTree* _vma_allocations;
+  LinkedListImpl<ReservedMemoryRegion>        _virtual_memory_allocations;
 
   // Virtual memory allocations by allocation sites, always in by_address
   // order
@@ -85,7 +86,6 @@ class MemBaseline {
   // create a memory baseline
   MemBaseline():
     _instance_class_count(0), _array_class_count(0), _thread_count(0),
-    _vma_allocations(nullptr),
     _baseline_type(Not_baselined) {
   }
 
@@ -110,9 +110,9 @@ class MemBaseline {
 
   // Virtual memory allocation iterator always returns in virtual memory
   // base address order.
-  RegionsTree* virtual_memory_allocations() {
-    assert(_vma_allocations != nullptr, "Not detail baseline");
-    return _vma_allocations;
+  VirtualMemoryAllocationIterator virtual_memory_allocations() {
+    assert(!_virtual_memory_allocations.is_empty(), "Not detail baseline");
+    return VirtualMemoryAllocationIterator(_virtual_memory_allocations.head());
   }
 
   // Total reserved memory = total malloc'd memory + total reserved virtual
@@ -185,8 +185,7 @@ class MemBaseline {
 
     _malloc_sites.clear();
     _virtual_memory_sites.clear();
-    delete _vma_allocations;
-    _vma_allocations = nullptr;
+    _virtual_memory_allocations.clear();
   }
 
  private:

src/hotspot/share/nmt/memReporter.cpp

@@ -394,11 +394,13 @@ int MemDetailReporter::report_virtual_memory_allocation_sites()  {
 
 void MemDetailReporter::report_virtual_memory_map() {
   // Virtual memory map always in base address order
+  VirtualMemoryAllocationIterator itr = _baseline.virtual_memory_allocations();
+  const ReservedMemoryRegion* rgn;
+
   output()->print_cr("Virtual memory map:");
-  _baseline.virtual_memory_allocations()->visit_reserved_regions([&](ReservedMemoryRegion& rgn) {
-    report_virtual_memory_region(&rgn);
-    return true;
-  });
+  while ((rgn = itr.next()) != nullptr) {
+    report_virtual_memory_region(rgn);
+  }
 }
 
 void MemDetailReporter::report_virtual_memory_region(const ReservedMemoryRegion* reserved_rgn) {

src/hotspot/share/nmt/nmtNativeCallStackStorage.cpp

@@ -57,21 +57,3 @@ NativeCallStackStorage::NativeCallStackStorage(bool is_detailed_mode, int table_
 NativeCallStackStorage::~NativeCallStackStorage() {
   FREE_C_HEAP_ARRAY(LinkPtr, _table);
 }
-
-NativeCallStackStorage::NativeCallStackStorage(const NativeCallStackStorage& other)
-  : _table_size(other._table_size),
-    _table(nullptr),
-    _stacks(),
-    _is_detailed_mode(other._is_detailed_mode),
-    _fake_stack(other._fake_stack) {
-  if (_is_detailed_mode) {
-    _table = NEW_C_HEAP_ARRAY(TableEntryIndex, _table_size, mtNMT);
-    for (int i = 0; i < _table_size; i++) {
-      _table[i] = other._table[i];
-    }
-  }
-  _stacks.reserve(other._stacks.length());
-  for (int i = 0; i < other._stacks.length(); i++) {
-    _stacks.at_grow(i) = other._stacks.at(i);
-  }
-}

src/hotspot/share/nmt/nmtNativeCallStackStorage.hpp

@@ -95,8 +95,7 @@ class NativeCallStackStorage : public CHeapObjBase {
   }
 
   NativeCallStackStorage(bool is_detailed_mode, int table_size = default_table_size);
-  NativeCallStackStorage(const NativeCallStackStorage& other);
-  NativeCallStackStorage& operator=(const NativeCallStackStorage& other) = delete;
+
   ~NativeCallStackStorage();
 };
 

src/hotspot/share/nmt/regionsTree.cpp

@@ -22,8 +22,6 @@
  *
  */
 #include "nmt/regionsTree.hpp"
-#include "nmt/regionsTree.inline.hpp"
-#include "nmt/virtualMemoryTracker.hpp"
 
 VMATree::SummaryDiff RegionsTree::commit_region(address addr, size_t size, const NativeCallStack& stack) {
   return commit_mapping((VMATree::position)addr, size, make_region_data(stack, mtNone), /*use tag inplace*/ true);
@@ -56,13 +54,4 @@ void RegionsTree::print_on(outputStream* st) {
     return true;
   });
 }
-#endif
-
-size_t RegionsTree::committed_size(ReservedMemoryRegion& rgn) {
-  size_t result = 0;
-  visit_committed_regions(rgn, [&](CommittedMemoryRegion& crgn) {
-    result += crgn.size();
-    return true;
-  });
-  return result;
-}
+#endif

src/hotspot/share/nmt/regionsTree.hpp

@@ -40,12 +40,6 @@ class RegionsTree : public VMATree {
  public:
   RegionsTree(bool with_storage) : VMATree() , _ncs_storage(with_storage), _with_storage(with_storage) { }
 
-  RegionsTree(const RegionsTree& other)
-  : VMATree(other),
-    _ncs_storage(other._ncs_storage),
-    _with_storage(other._with_storage) {}
-  RegionsTree& operator=(const RegionsTree& other) = delete;
-
   ReservedMemoryRegion find_reserved_region(address addr);
 
   SummaryDiff commit_region(address addr, size_t size, const NativeCallStack& stack);
@@ -97,8 +91,6 @@ class RegionsTree : public VMATree {
     NativeCallStackStorage::StackIndex si = node.out_stack_index();
     return _ncs_storage.get(si);
   }
-
-  size_t committed_size(ReservedMemoryRegion& rgn);
 };
 
-#endif // NMT_REGIONSTREE_HPP
+#endif // NMT_REGIONSTREE_HPP

src/hotspot/share/nmt/vmatree.cpp

@@ -744,10 +744,3 @@ void VMATree::SummaryDiff::print_on(outputStream* out) {
   }
 }
 #endif
-
-void VMATree::clear() {
-  _tree.remove_all();
-};
-bool VMATree::is_empty() {
-  return _tree.size() == 0;
-};

src/hotspot/share/nmt/vmatree.hpp

@@ -30,7 +30,6 @@
 #include "nmt/nmtNativeCallStackStorage.hpp"
 #include "utilities/globalDefinitions.hpp"
 #include "utilities/ostream.hpp"
-#include "utilities/rbTree.hpp"
 #include "utilities/rbTree.inline.hpp"
 
 #include <cstdint>
@@ -40,7 +39,7 @@
 // For example, the state may go from released memory to committed memory,
 // or from committed memory of a certain MemTag to committed memory of a different MemTag.
 // The set of points is stored in a balanced binary tree for efficient querying and updating.
-class VMATree : public CHeapObjBase {
+class VMATree {
   friend class NMTVMATreeTest;
   friend class VMTWithVMATreeTest;
   // A position in memory.
@@ -66,6 +65,7 @@ class VMATree : public CHeapObjBase {
   static const char* statetype_strings[static_cast<uint8_t>(StateType::st_number_of_states)];
 
 public:
+  NONCOPYABLE(VMATree);
 
   static const char* statetype_to_string(StateType type) {
     assert(type < StateType::st_number_of_states, "must be");
@@ -226,10 +226,6 @@ class VMATree : public CHeapObjBase {
 
 public:
   VMATree() : _tree() {}
-  VMATree(const VMATree& other) : _tree() {
-    assert(other._tree.copy_into(_tree), "VMATree dies on OOM");
-  }
-  VMATree& operator=(VMATree const&) = delete;
 
   struct SingleDiff {
     using delta = int64_t;
@@ -333,8 +329,5 @@ class VMATree : public CHeapObjBase {
     _tree.visit_range_in_order(from, to, f);
   }
   VMARBTree& tree() { return _tree; }
-
-  void clear();
-  bool is_empty();
 };
 #endif

src/hotspot/share/utilities/rbTree.hpp

@@ -428,7 +428,6 @@ class AbstractRBTree {
   template <typename F>
   void visit_in_order(F f);
 
-
   // Visit all RBNodes in ascending order whose keys are in range [from, to], calling f on each node.
   // If f returns `true` the iteration continues, otherwise it is stopped at the current node.
   template <typename F>
@@ -476,7 +475,6 @@ class RBTree : public AbstractRBTree<K, RBNode<K, V>, COMPARATOR> {
 
 public:
   RBTree() : BaseType(), _allocator() {}
-  NONCOPYABLE(RBTree);
   ~RBTree() { remove_all(); }
   RBTree(const RBTree& other) : BaseType(), _allocator() {
     assert(std::is_copy_constructible<V>(), "Value type must be copy-constructible");
@@ -487,8 +485,6 @@ class RBTree : public AbstractRBTree<K, RBNode<K, V>, COMPARATOR> {
   }
   RBTree& operator=(const RBTree& other) = delete;
 
-  bool copy_into(RBTree& other) const;
-
   typedef typename BaseType::Cursor Cursor;
   using BaseType::cursor;
   using BaseType::insert_at_cursor;