Move untrackSymbol to IdentifierTable::freeUnmarkedSymbols

Riley Dulin · facebook-github-bot · commit ef8367349635 · 2020-12-04T13:04:49.000-08:00
Summary:
This diff fixes two problems:
* Hades never untracked symbols for heap snapshot purposes
* GenGC and Malloc accidentally untracked "permanent" symbols, or ones that aren't actually freed

To avoid the latter problem from occurring, have `IdentifierTable` itself be the one who calls
`untrackSymbol`.

An alternative would be to have IdentifierTable modify and return a new bitset. That might be
cleaner (more separation of concerns), but also seems like extra cost.

Reviewed By: neildhar

Differential Revision: D25291784

fbshipit-source-id: 46be72f1b7e21da640002f7306b2a09a94abad10
diff --git a/include/hermes/VM/GCBase.h b/include/hermes/VM/GCBase.h
@@ -543,10 +543,9 @@ class GCBase {
     /// tracking working set small.
     inline void untrackObject(const void *cell);
 
-    /// For a symbol index \p i, \p markedSymbols[i] indicates whether
-    /// the symbol was reachable in a GC.  Untrack all symbols whose
-    /// index is not marked.
-    void untrackUnmarkedSymbols(const llvh::BitVector &markedSymbols);
+    /// Remove the symbol from being tracked. This needs to be done to allow
+    /// symbols to be re-used.
+    inline void untrackSymbol(uint32_t symIdx);
 
     /// Remove the native memory from being tracked. This should be done to keep
     /// the tracking working set small. It is also required to be done when
@@ -1667,6 +1666,11 @@ inline void GCBase::IDTracker::untrackNative(const void *mem) {
   untrackObject(mem);
 }
 
+inline void GCBase::IDTracker::untrackSymbol(uint32_t symIdx) {
+  std::lock_guard<Mutex> lk{mtx_};
+  symbolIDMap_.erase(symIdx);
+}
+
 inline const GCExecTrace &GCBase::getGCExecTrace() const {
   return execTrace_;
 }
diff --git a/include/hermes/VM/IdentifierTable.h b/include/hermes/VM/IdentifierTable.h
@@ -165,7 +165,9 @@ class IdentifierTable {
   void unmarkSymbols();
 
   /// Invoked at the end of a GC to free all unmarked symbols.
-  void freeUnmarkedSymbols(const llvh::BitVector &markedSymbols);
+  void freeUnmarkedSymbols(
+      const llvh::BitVector &markedSymbols,
+      GC::IDTracker &gc);
 
 #ifdef HERMES_SLOW_DEBUG
   /// \return true if the given symbol is a live entry in the identifier
diff --git a/lib/VM/GCBase.cpp b/lib/VM/GCBase.cpp
@@ -916,20 +916,6 @@ void GCBase::IDTracker::deserialize(Deserializer &d) {
 }
 #endif
 
-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)) {
-      toUntrack.push_back(pair.first);
-    }
-  }
-  for (uint32_t symIdx : toUntrack) {
-    symbolIDMap_.erase(symIdx);
-  }
-}
-
 HeapSnapshot::NodeID GCBase::IDTracker::getNumberID(double num) {
   std::lock_guard<Mutex> lk{mtx_};
   auto &numberRef = numberIDMap_[num];
diff --git a/lib/VM/IdentifierTable.cpp b/lib/VM/IdentifierTable.cpp
@@ -518,7 +518,8 @@ void IdentifierTable::unmarkSymbols() {
 }
 
 void IdentifierTable::freeUnmarkedSymbols(
-    const llvh::BitVector &markedSymbols) {
+    const llvh::BitVector &markedSymbols,
+    GC::IDTracker &tracker) {
   assert(
       markedSymbols.size() <= lookupVector_.size() &&
       "Size of markedSymbols must be less than the current lookupVector");
@@ -529,6 +530,7 @@ void IdentifierTable::freeUnmarkedSymbols(
   // Flip and find set bits, which will correspond to symbols that weren't
   // marked.
   markedSymbols_.flip();
+  const bool isTrackingIDs = tracker.isTrackingIDs();
   const uint32_t markedSymbolsSize = markedSymbols.size();
   for (const uint32_t i : markedSymbols_.set_bits()) {
     // Don't check any bits after the passed-in bits, which represent the number
@@ -538,8 +540,12 @@ void IdentifierTable::freeUnmarkedSymbols(
     }
     // We never free StringPrimitives that are materialized from a lazy
     // identifier.
-    if (lookupVector_[i].isNonLazyStringPrim())
+    if (lookupVector_[i].isNonLazyStringPrim()) {
+      if (isTrackingIDs) {
+        tracker.untrackSymbol(i);
+      }
       freeSymbol(i);
+    }
   }
   markedSymbols_.reset();
 }
diff --git a/lib/VM/Runtime.cpp b/lib/VM/Runtime.cpp
@@ -753,7 +753,7 @@ void Runtime::unmarkSymbols() {
 }
 
 void Runtime::freeSymbols(const llvh::BitVector &markedSymbols) {
-  identifierTable_.freeUnmarkedSymbols(markedSymbols);
+  identifierTable_.freeUnmarkedSymbols(markedSymbols, heap_.getIDTracker());
 }
 
 #ifdef HERMES_SLOW_DEBUG
diff --git a/lib/VM/gcs/GenGCNC.cpp b/lib/VM/gcs/GenGCNC.cpp
@@ -377,9 +377,6 @@ void GenGC::collect(std::string cause, bool canEffectiveOOM) {
         /* youngGenIsEmpty */ youngGen_.usedDirect() == 0);
 
     gcCallbacks_->freeSymbols(markedSymbols_);
-    if (idTracker_.isTrackingIDs()) {
-      idTracker_.untrackUnmarkedSymbols(markedSymbols_);
-    }
 
     // Update the exponential weighted average of live size, which we'll
     // consult if we need to shrink the heap.
diff --git a/lib/VM/gcs/MallocGC.cpp b/lib/VM/gcs/MallocGC.cpp
@@ -292,9 +292,6 @@ void MallocGC::collect(std::string cause) {
     resetWeakReferences();
     // Free the unused symbols.
     gcCallbacks_->freeSymbols(acceptor.markedSymbols_);
-    if (idTracker_.isTrackingIDs()) {
-      idTracker_.untrackUnmarkedSymbols(acceptor.markedSymbols_);
-    }
     // By the end of the marking loop, all pointers left in pointers_ are dead.
     for (CellHeader *header : pointers_) {
 #ifndef HERMESVM_SANITIZE_HANDLES

Original file line number	Diff line number	Diff line change
`@@ -753,7 +753,7 @@ void Runtime::unmarkSymbols() {`
`753`	`753`	`}`
`754`	`754`
`755`	`755`	`void Runtime::freeSymbols(const llvh::BitVector &markedSymbols) {`
`756`		`- identifierTable_.freeUnmarkedSymbols(markedSymbols);`
	`756`	`+ identifierTable_.freeUnmarkedSymbols(markedSymbols, heap_.getIDTracker());`
`757`	`757`	`}`
`758`	`758`
`759`	`759`	`#ifdef HERMES_SLOW_DEBUG`