Eliminate compacteeHandleForSweep_

Summary:
If compaction is completed before sweeping, there may be dead objects
in the heap that refer into the now evacuated compactee. To deal with
this, we maintain a `shared_ptr` to the compactee to keep it alive
until sweeping is completed, in case something attempts to read one of
those dangling references.

The only such case is card marking, which can visit unmarked objects in
some cases.

The current system is complicated and difficult to follow, and is only
correct by accident. This is because `compacteeHandleForSweep_` can
currently be freed by the background thread, which is unsafe, because
an `AlignedStorage` should in theory only be destroyed on the mutator.
But it happens to be correct because the relevant data is incidentally
synchronised by `gcMutex_`.

To simplify this, get rid of `compacteeHandleForSweep_`, and always
skip dead objects in card marking.

Reviewed By: lavenzg

Differential Revision: D44585769

fbshipit-source-id: 8904d21f06772462ca63d6b7215a988f3094754d

include/hermes/VM/HadesGC.h

@@ -837,15 +837,6 @@ class HadesGC final : public GCBase {
     std::shared_ptr<HeapSegment> segment;
   } compactee_;
 
-  /// If compaction completes before sweeping, there is a possibility that
-  /// dangling pointers into the now freed compactee may remain in the OG heap
-  /// until sweeping finishes. In certain cases, like when scanning dirty cards,
-  /// this could cause a segfault if you attempt to say, compress a pointer. To
-  /// handle this case, if compaction completes while sweeping is still in
-  /// progress, this shared_ptr will keep the compactee segment alive until the
-  /// end of sweeping.
-  std::shared_ptr<HeapSegment> compacteeHandleForSweep_;
-
   /// The number of compactions this GC has performed.
   size_t numCompactions_{0};
 

lib/VM/gcs/HadesGC.cpp

@@ -1709,7 +1709,6 @@ void HadesGC::incrementalCollect(bool backgroundThread) {
         // Finish any collection bookkeeping.
         ogCollectionStats_->setEndTime();
         ogCollectionStats_->setAfterSize(segmentFootprint());
-        compacteeHandleForSweep_.reset();
         concurrentPhase_ = Phase::None;
         if (!backgroundThread)
           checkTripwireAndSubmitStats();
@@ -1751,7 +1750,6 @@ void HadesGC::prepareCompactee(bool forceCompaction) {
     compactee_.startCP = CompressedPointer::encodeNonNull(
         reinterpret_cast<GCCell *>(compactee_.segment->lowLim()),
         getPointerBase());
-    compacteeHandleForSweep_ = compactee_.segment;
   }
 }
 
@@ -2691,12 +2689,17 @@ void HadesGC::scanDirtyCardsForSegment(
   size_t from = cardTable.addressToIndex(seg.start());
   const size_t to = cardTable.addressToIndex(origSegLevel - 1) + 1;
 
-  // If a compaction is taking place during sweeping, we may scan cards that
-  // contain dead objects which in turn point to dead objects in the compactee.
-  // In order to avoid promoting these dead objects, we should skip unmarked
-  // objects altogether when compaction and sweeping happen at the same time.
-  const bool visitUnmarked =
-      !CompactionEnabled || concurrentPhase_ != Phase::Sweep;
+  // Do not scan unmarked objects in the OG if we are currently sweeping. We do
+  // this for three reasons:
+  // 1. If an object is unmarked during sweeping, that means it is dead, so it
+  //    is wasteful to scan it and promote objects it refers to.
+  // 2. During compaction, these unmarked objects may point to dead objects in
+  //    the compactee. Promoting these objects is dangerous, since they may
+  //    contain references to other dead objects that are or will be freed.
+  // 3. If a compaction was completed during this cycle, unmarked objects may
+  //    contain references into the now freed compactee. These pointers are not
+  //    safe to visit.
+  const bool visitUnmarked = concurrentPhase_ != Phase::Sweep;
 
   while (const auto oiBegin = cardTable.findNextDirtyCard(from, to)) {
     const auto iBegin = *oiBegin;