Add a mutex to AllocationLocationTracker functions

Riley Dulin · facebook-github-bot · commit f58d99df560a · 2020-12-04T13:04:49.000-08:00
Summary:
In concurrent mode Hades needs to have some extra synchronization
when enabling/disabling the memory profiler, and when adding new allocs.

Add a `Mutex` used to protect the `stackMap_` member. The `enabled_` member
is protected by ensuring that no background GC is running while enabling/disabling.

The cost of acquiring mutexes is only paid when the profiler is on, which is only in
debug modes. This should have no effect on release performance.

Reviewed By: neildhar

Differential Revision: D25284428

fbshipit-source-id: 0964c070f2ad2cbe01cf004d5c5e1b9d9c814f61
diff --git a/include/hermes/VM/GCBase.h b/include/hermes/VM/GCBase.h
@@ -419,6 +419,11 @@ class GCBase {
     /// Flush out heap profiler data to the callback after a new kFlushThreshold
     /// bytes are allocated.
     static constexpr uint64_t kFlushThreshold = 128 * (1 << 10);
+    /// This mutex protects stackMap_. Specifically does not protect enabled_,
+    /// because enabled_ should only be changed while the GC isn't running
+    /// anyway. Also doesn't protect fragments_ because only the allocator
+    /// modifies fragments_.
+    Mutex mtx_;
     /// Associates allocations at their current location with their stack trace
     /// data.
     llvh::DenseMap<const void *, StackTracesTreeNode *> stackMap_;
@@ -580,6 +585,11 @@ class GCBase {
     /// the Chrome snapshot viewer.
     static constexpr HeapSnapshot::NodeID kIDStep = 2;
 
+    /// This mutex protects objectIDMap_, symbolIDMap_, and numberIDMap_.
+    /// Specifically does not protect lastID_, since there's only one allocator
+    /// at a time, and only new allocations affect lastID_.
+    Mutex mtx_;
+
     /// The last ID assigned to a non-native object. Object IDs are not
     /// recycled so that snapshots don't confuse two objects with each other.
     /// NOTE: Reserved guarantees that this is an odd number.
@@ -747,6 +757,21 @@ class GCBase {
   virtual void createSnapshot(llvh::raw_ostream &os) = 0;
   void createSnapshot(GC *gc, llvh::raw_ostream &os);
 
+  /// Turn on the heap profiler, which will track when allocations are made and
+  /// the stack trace of when they were created.
+  virtual void enableHeapProfiler(
+      std::function<void(
+          uint64_t,
+          std::chrono::microseconds,
+          std::vector<GCBase::AllocationLocationTracker::HeapStatsUpdate>)>
+          fragmentCallback);
+
+  /// Turn off the heap profiler, which will stop tracking new allocations and
+  /// not record any stack traces.
+  /// Disabling will not forget any objects that are still alive. This way
+  /// re-enabling later will still remember earlier objects.
+  virtual void disableHeapProfiler();
+
 #ifdef HERMESVM_SERIALIZE
   /// Serialize WeakRefs.
   virtual void serializeWeakRefs(Serializer &s) = 0;
@@ -1564,6 +1589,7 @@ inline bool GCBase::IDTracker::isTrackingIDs() const {
 }
 
 inline HeapSnapshot::NodeID GCBase::IDTracker::getObjectID(const void *cell) {
+  std::lock_guard<Mutex> lk{mtx_};
   auto iter = objectIDMap_.find(cell);
   if (iter != objectIDMap_.end()) {
     return iter->second;
@@ -1576,12 +1602,14 @@ inline HeapSnapshot::NodeID GCBase::IDTracker::getObjectID(const void *cell) {
 
 inline HeapSnapshot::NodeID GCBase::IDTracker::getObjectIDMustExist(
     const void *cell) {
+  std::lock_guard<Mutex> lk{mtx_};
   auto iter = objectIDMap_.find(cell);
   assert(iter != objectIDMap_.end() && "cell must already have an ID");
   return iter->second;
 }
 
 inline HeapSnapshot::NodeID GCBase::IDTracker::getObjectID(uint32_t symIdx) {
+  std::lock_guard<Mutex> lk{mtx_};
   auto iter = symbolIDMap_.find(symIdx);
   if (iter != symbolIDMap_.end()) {
     return iter->second;
@@ -1593,6 +1621,7 @@ inline HeapSnapshot::NodeID GCBase::IDTracker::getObjectID(uint32_t symIdx) {
 }
 
 inline HeapSnapshot::NodeID GCBase::IDTracker::getNativeID(const void *mem) {
+  std::lock_guard<Mutex> lk{mtx_};
   auto iter = objectIDMap_.find(mem);
   if (iter != objectIDMap_.end()) {
     return iter->second;
@@ -1612,6 +1641,7 @@ inline void GCBase::IDTracker::moveObject(
     // happen in old generations where it is compacted to the same location.
     return;
   }
+  std::lock_guard<Mutex> lk{mtx_};
   auto old = objectIDMap_.find(oldLocation);
   if (old == objectIDMap_.end()) {
     // Avoid making new keys for objects that don't need to be tracked.
@@ -1627,6 +1657,7 @@ inline void GCBase::IDTracker::moveObject(
 }
 
 inline void GCBase::IDTracker::untrackObject(const void *cell) {
+  std::lock_guard<Mutex> lk{mtx_};
   objectIDMap_.erase(cell);
 }
 
@@ -1642,6 +1673,7 @@ inline const GCExecTrace &GCBase::getGCExecTrace() const {
 
 template <typename F>
 inline void GCBase::IDTracker::forEachID(F callback) {
+  std::lock_guard<Mutex> lk{mtx_};
   for (auto &p : objectIDMap_) {
     callback(p.first, p.second);
   }
diff --git a/include/hermes/VM/HadesGC.h b/include/hermes/VM/HadesGC.h
@@ -84,6 +84,13 @@ class HadesGC final : public GCBase {
   void getHeapInfoWithMallocSize(HeapInfo &info) override;
   void getCrashManagerHeapInfo(CrashManager::HeapInformation &info) override;
   void createSnapshot(llvh::raw_ostream &os) override;
+  void enableHeapProfiler(
+      std::function<void(
+          uint64_t,
+          std::chrono::microseconds,
+          std::vector<GCBase::AllocationLocationTracker::HeapStatsUpdate>)>
+          fragmentCallback) override;
+  void disableHeapProfiler() override;
   void printStats(JSONEmitter &json) override;
 
   /// \}
diff --git a/lib/VM/GCBase.cpp b/lib/VM/GCBase.cpp
@@ -536,6 +536,19 @@ void GCBase::createSnapshot(GC *gc, llvh::raw_ostream &os) {
   snap.endSection(HeapSnapshot::Section::Locations);
 }
 
+void GCBase::enableHeapProfiler(
+    std::function<void(
+        uint64_t,
+        std::chrono::microseconds,
+        std::vector<GCBase::AllocationLocationTracker::HeapStatsUpdate>)>
+        fragmentCallback) {
+  getAllocationLocationTracker().enable(std::move(fragmentCallback));
+}
+
+void GCBase::disableHeapProfiler() {
+  getAllocationLocationTracker().disable();
+}
+
 void GCBase::checkTripwire(size_t dataSize) {
   if (LLVM_LIKELY(!tripwireCallback_) ||
       LLVM_LIKELY(dataSize < tripwireLimit_) || tripwireCalled_) {
@@ -905,6 +918,7 @@ void GCBase::IDTracker::deserialize(Deserializer &d) {
 
 void GCBase::IDTracker::untrackUnmarkedSymbols(
     const llvh::BitVector &markedSymbols) {
+  std::lock_guard<Mutex> lk{mtx_};
   std::vector<uint32_t> toUntrack;
   for (const auto &pair : symbolIDMap_) {
     if (!markedSymbols.test(pair.first)) {
@@ -917,6 +931,7 @@ void GCBase::IDTracker::untrackUnmarkedSymbols(
 }
 
 HeapSnapshot::NodeID GCBase::IDTracker::getNumberID(double num) {
+  std::lock_guard<Mutex> lk{mtx_};
   auto &numberRef = numberIDMap_[num];
   // If the entry didn't exist, the value was initialized to 0.
   if (numberRef != 0) {
@@ -992,6 +1007,8 @@ void GCBase::AllocationLocationTracker::newAlloc(const void *ptr, uint32_t sz) {
       flushCallback();
     }
     if (auto node = gc_->gcCallbacks_->getCurrentStackTracesTreeNode(ip)) {
+      // Hold a lock while modifying stackMap_.
+      std::lock_guard<Mutex> lk{mtx_};
       auto itAndDidInsert = stackMap_.try_emplace(ptr, node);
       assert(itAndDidInsert.second && "Failed to create a new node");
       (void)itAndDidInsert;
@@ -1006,6 +1023,9 @@ void GCBase::AllocationLocationTracker::moveAlloc(
     // This can happen in old generations when compacting to the same location.
     return;
   }
+  // Hold a lock in case concurrent Hades tries to free an alloc at the same
+  // time.
+  std::lock_guard<Mutex> lk{mtx_};
   auto oldIt = stackMap_.find(oldPtr);
   if (oldIt == stackMap_.end()) {
     // This can happen if the tracker is turned on between collections, and
@@ -1023,6 +1043,9 @@ void GCBase::AllocationLocationTracker::moveAlloc(
 void GCBase::AllocationLocationTracker::freeAlloc(
     const void *ptr,
     uint32_t sz) {
+  // Hold a lock during freeAlloc because concurrent Hades might be creating an
+  // alloc (newAlloc) at the same time.
+  std::lock_guard<Mutex> lk{mtx_};
   stackMap_.erase(ptr);
   if (!enabled_) {
     // Fragments won't exist if the heap profiler isn't enabled.
diff --git a/lib/VM/Runtime.cpp b/lib/VM/Runtime.cpp
@@ -2242,11 +2242,11 @@ void Runtime::enableAllocationLocationTracker(
     stackTracesTree_ = make_unique<StackTracesTree>();
   }
   stackTracesTree_->syncWithRuntimeStack(this);
-  heap_.getAllocationLocationTracker().enable(std::move(fragmentCallback));
+  heap_.enableHeapProfiler(std::move(fragmentCallback));
 }
 
 void Runtime::disableAllocationLocationTracker(bool clearExistingTree) {
-  heap_.getAllocationLocationTracker().disable();
+  heap_.disableHeapProfiler();
   if (clearExistingTree) {
     stackTracesTree_.reset();
   }
diff --git a/lib/VM/gcs/HadesGC.cpp b/lib/VM/gcs/HadesGC.cpp
@@ -900,10 +900,6 @@ bool HadesGC::OldGen::sweepNext() {
     // Cell is dead, run its finalizer first if it has one.
     cell->getVT()->finalizeIfExists(cell, gc_);
     if (isTracking) {
-      // FIXME: There could be a race condition here if newAlloc is
-      // being called at the same time and using a shared data structure
-      // with freeAlloc. freeAlloc relies on the ID, so call it before
-      // untrackObject.
       gc_->getAllocationLocationTracker().freeAlloc(cell, sz);
       gc_->getIDTracker().untrackObject(cell);
     }
@@ -1085,6 +1081,25 @@ void HadesGC::createSnapshot(llvh::raw_ostream &os) {
   }
 }
 
+void HadesGC::enableHeapProfiler(
+    std::function<void(
+        uint64_t,
+        std::chrono::microseconds,
+        std::vector<GCBase::AllocationLocationTracker::HeapStatsUpdate>)>
+        fragmentCallback) {
+  std::lock_guard<Mutex> lk{gcMutex_};
+  // Let any existing collections complete before enabling the profiler.
+  waitForCollectionToFinish();
+  GCBase::enableHeapProfiler(std::move(fragmentCallback));
+}
+
+void HadesGC::disableHeapProfiler() {
+  std::lock_guard<Mutex> lk{gcMutex_};
+  // Let any existing collections complete before disabling the profiler.
+  waitForCollectionToFinish();
+  GCBase::disableHeapProfiler();
+}
+
 void HadesGC::printStats(JSONEmitter &json) {
   GCBase::printStats(json);
   json.emitKey("specific");
diff --git a/unittests/VMRuntime/StackTracesTreeTest.cpp b/unittests/VMRuntime/StackTracesTreeTest.cpp
@@ -7,8 +7,7 @@
 
 #include "hermes/VM/StackTracesTree-NoRuntime.h"
 
-#if defined(HERMES_ENABLE_ALLOCATION_LOCATION_TRACES) and \
-    !defined(HERMESVM_GC_HADES)
+#if defined(HERMES_ENABLE_ALLOCATION_LOCATION_TRACES)
 
 #include "TestHelpers.h"
 
@@ -61,7 +60,7 @@ struct StackTracesTreeTest : public RuntimeTestFixtureBase {
     std::string res;
     llvh::raw_string_ostream resStream(res);
     auto stringTable = runtime->getStackTracesTree()->getStringTable();
-    auto allocationLocationTracker =
+    auto &allocationLocationTracker =
         runtime->getHeap().getAllocationLocationTracker();
     auto node = allocationLocationTracker.getStackTracesTreeNodeForAlloc(
         runRes->getPointer());

Original file line number	Diff line number	Diff line change
`@@ -2242,11 +2242,11 @@ void Runtime::enableAllocationLocationTracker(`
`2242`	`2242`	`stackTracesTree_ = make_unique<StackTracesTree>();`
`2243`	`2243`	`}`
`2244`	`2244`	`stackTracesTree_->syncWithRuntimeStack(this);`
`2245`		`- heap_.getAllocationLocationTracker().enable(std::move(fragmentCallback));`
	`2245`	`+ heap_.enableHeapProfiler(std::move(fragmentCallback));`
`2246`	`2246`	`}`
`2247`	`2247`
`2248`	`2248`	`void Runtime::disableAllocationLocationTracker(bool clearExistingTree) {`
`2249`		`- heap_.getAllocationLocationTracker().disable();`
	`2249`	`+ heap_.disableHeapProfiler();`
`2250`	`2250`	`if (clearExistingTree) {`
`2251`	`2251`	`stackTracesTree_.reset();`
`2252`	`2252`	`}`