[heap] Add memory fence after mark-bit range update operations.

The fence ensures that the concurrent marker observes consistent state
of mark-bits for newly allocated objects.

The patch also moves Bitmap functions to cc file and removes non-atomic
versions of SetRange and ClearRange.

BUG=chromium:694255

Change-Id: I466bef654f3d4a21b7aaebdfd6d5a39ddb5f2a0a
Reviewed-on: https://chromium-review.googlesource.com/530367Reviewed-by: Hannes Payer <hpayer@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Commit-Queue: Ulan Degenbaev <ulan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#45868}

BUILD.gn

@@ -1629,6 +1629,7 @@ v8_source_set("v8_base") {
     "src/heap/mark-compact-inl.h",
     "src/heap/mark-compact.cc",
     "src/heap/mark-compact.h",
+    "src/heap/marking.cc",
     "src/heap/marking.h",
     "src/heap/memory-reducer.cc",
     "src/heap/memory-reducer.h",

src/heap/marking.cc

+// Copyright 2017 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "src/heap/marking.h"
+
+namespace v8 {
+namespace internal {
+
+void Bitmap::Clear() {
+  base::Atomic32* cell_base = reinterpret_cast<base::Atomic32*>(cells());
+  for (int i = 0; i < CellsCount(); i++) {
+    base::Relaxed_Store(cell_base + i, 0);
+  }
+  // This fence prevents re-ordering of publishing stores with the mark-bit
+  // clearing stores.
+  base::MemoryFence();
+}
+
+void Bitmap::SetRange(uint32_t start_index, uint32_t end_index) {
+  unsigned int start_cell_index = start_index >> Bitmap::kBitsPerCellLog2;
+  MarkBit::CellType start_index_mask = 1u << Bitmap::IndexInCell(start_index);
+  unsigned int end_cell_index = end_index >> Bitmap::kBitsPerCellLog2;
+  MarkBit::CellType end_index_mask = 1u << Bitmap::IndexInCell(end_index);
+  if (start_cell_index != end_cell_index) {
+    // Firstly, fill all bits from the start address to the end of the first
+    // cell with 1s.
+    SetBitsInCell<MarkBit::ATOMIC>(start_cell_index, ~(start_index_mask - 1));
+    // Then fill all in between cells with 1s.
+    base::Atomic32* cell_base = reinterpret_cast<base::Atomic32*>(cells());
+    for (unsigned int i = start_cell_index + 1; i < end_cell_index; i++) {
+      base::Relaxed_Store(cell_base + i, ~0u);
+    }
+    // Finally, fill all bits until the end address in the last cell with 1s.
+    SetBitsInCell<MarkBit::ATOMIC>(end_cell_index, (end_index_mask - 1));
+  } else {
+    SetBitsInCell<MarkBit::ATOMIC>(start_cell_index,
+                                   end_index_mask - start_index_mask);
+  }
+  // This fence prevents re-ordering of publishing stores with the mark-
+  // bit setting stores.
+  base::MemoryFence();
+}
+
+void Bitmap::ClearRange(uint32_t start_index, uint32_t end_index) {
+  unsigned int start_cell_index = start_index >> Bitmap::kBitsPerCellLog2;
+  MarkBit::CellType start_index_mask = 1u << Bitmap::IndexInCell(start_index);
+
+  unsigned int end_cell_index = end_index >> Bitmap::kBitsPerCellLog2;
+  MarkBit::CellType end_index_mask = 1u << Bitmap::IndexInCell(end_index);
+
+  if (start_cell_index != end_cell_index) {
+    // Firstly, fill all bits from the start address to the end of the first
+    // cell with 0s.
+    ClearBitsInCell<MarkBit::ATOMIC>(start_cell_index, ~(start_index_mask - 1));
+    // Then fill all in between cells with 0s.
+    base::Atomic32* cell_base = reinterpret_cast<base::Atomic32*>(cells());
+    for (unsigned int i = start_cell_index + 1; i < end_cell_index; i++) {
+      base::Relaxed_Store(cell_base + i, 0);
+    }
+    // Finally, set all bits until the end address in the last cell with 0s.
+    ClearBitsInCell<MarkBit::ATOMIC>(end_cell_index, (end_index_mask - 1));
+  } else {
+    ClearBitsInCell<MarkBit::ATOMIC>(start_cell_index,
+                                     (end_index_mask - start_index_mask));
+  }
+  // This fence prevents re-ordering of publishing stores with the mark-
+  // bit clearing stores.
+  base::MemoryFence();
+}
+
+bool Bitmap::AllBitsSetInRange(uint32_t start_index, uint32_t end_index) {
+  unsigned int start_cell_index = start_index >> Bitmap::kBitsPerCellLog2;
+  MarkBit::CellType start_index_mask = 1u << Bitmap::IndexInCell(start_index);
+
+  unsigned int end_cell_index = end_index >> Bitmap::kBitsPerCellLog2;
+  MarkBit::CellType end_index_mask = 1u << Bitmap::IndexInCell(end_index);
+
+  MarkBit::CellType matching_mask;
+  if (start_cell_index != end_cell_index) {
+    matching_mask = ~(start_index_mask - 1);
+    if ((cells()[start_cell_index] & matching_mask) != matching_mask) {
+      return false;
+    }
+    for (unsigned int i = start_cell_index + 1; i < end_cell_index; i++) {
+      if (cells()[i] != ~0u) return false;
+    }
+    matching_mask = (end_index_mask - 1);
+    // Check against a mask of 0 to avoid dereferencing the cell after the
+    // end of the bitmap.
+    return (matching_mask == 0) ||
+           ((cells()[end_cell_index] & matching_mask) == matching_mask);
+  } else {
+    matching_mask = end_index_mask - start_index_mask;
+    // Check against a mask of 0 to avoid dereferencing the cell after the
+    // end of the bitmap.
+    return (matching_mask == 0) ||
+           (cells()[end_cell_index] & matching_mask) == matching_mask;
+  }
+}
+
+bool Bitmap::AllBitsClearInRange(uint32_t start_index, uint32_t end_index) {
+  unsigned int start_cell_index = start_index >> Bitmap::kBitsPerCellLog2;
+  MarkBit::CellType start_index_mask = 1u << Bitmap::IndexInCell(start_index);
+
+  unsigned int end_cell_index = end_index >> Bitmap::kBitsPerCellLog2;
+  MarkBit::CellType end_index_mask = 1u << Bitmap::IndexInCell(end_index);
+
+  MarkBit::CellType matching_mask;
+  if (start_cell_index != end_cell_index) {
+    matching_mask = ~(start_index_mask - 1);
+    if ((cells()[start_cell_index] & matching_mask)) return false;
+    for (unsigned int i = start_cell_index + 1; i < end_cell_index; i++) {
+      if (cells()[i]) return false;
+    }
+    matching_mask = (end_index_mask - 1);
+    // Check against a mask of 0 to avoid dereferencing the cell after the
+    // end of the bitmap.
+    return (matching_mask == 0) || !(cells()[end_cell_index] & matching_mask);
+  } else {
+    matching_mask = end_index_mask - start_index_mask;
+    // Check against a mask of 0 to avoid dereferencing the cell after the
+    // end of the bitmap.
+    return (matching_mask == 0) || !(cells()[end_cell_index] & matching_mask);
+  }
+}
+
+namespace {
+
+void PrintWord(uint32_t word, uint32_t himask = 0) {
+  for (uint32_t mask = 1; mask != 0; mask <<= 1) {
+    if ((mask & himask) != 0) PrintF("[");
+    PrintF((mask & word) ? "1" : "0");
+    if ((mask & himask) != 0) PrintF("]");
+  }
+}
+
+class CellPrinter {
+ public:
+  CellPrinter() : seq_start(0), seq_type(0), seq_length(0) {}
+
+  void Print(uint32_t pos, uint32_t cell) {
+    if (cell == seq_type) {
+      seq_length++;
+      return;
+    }
+
+    Flush();
+
+    if (IsSeq(cell)) {
+      seq_start = pos;
+      seq_length = 0;
+      seq_type = cell;
+      return;
+    }
+
+    PrintF("%d: ", pos);
+    PrintWord(cell);
+    PrintF("\n");
+  }
+
+  void Flush() {
+    if (seq_length > 0) {
+      PrintF("%d: %dx%d\n", seq_start, seq_type == 0 ? 0 : 1,
+             seq_length * Bitmap::kBitsPerCell);
+      seq_length = 0;
+    }
+  }
+
+  static bool IsSeq(uint32_t cell) { return cell == 0 || cell == 0xFFFFFFFF; }
+
+ private:
+  uint32_t seq_start;
+  uint32_t seq_type;
+  uint32_t seq_length;
+};
+
+}  // anonymous namespace
+
+void Bitmap::Print() {
+  CellPrinter printer;
+  for (int i = 0; i < CellsCount(); i++) {
+    printer.Print(i, cells()[i]);
+  }
+  printer.Flush();
+  PrintF("\n");
+}
+
+bool Bitmap::IsClean() {
+  for (int i = 0; i < CellsCount(); i++) {
+    if (cells()[i] != 0) {
+      return false;
+    }
+  }
+  return true;
+}
+
+}  // namespace internal
+}  // namespace v8

src/heap/marking.h

@@ -110,7 +110,7 @@ inline bool MarkBit::Clear<MarkBit::ATOMIC>() {
 }
 
 // Bitmap is a sequence of cells each containing fixed number of bits.
-class Bitmap {
+class V8_EXPORT_PRIVATE Bitmap {
  public:
   static const uint32_t kBitsPerCell = 32;
   static const uint32_t kBitsPerCellLog2 = 5;
@@ -165,9 +165,7 @@ class Bitmap {
     return MarkBit(reinterpret_cast<base::Atomic32*>(cell), mask);
   }
 
-  void Clear() {
-    for (int i = 0; i < CellsCount(); i++) cells()[i] = 0;
-  }
+  void Clear();
 
   // Clears bits in the given cell. The mask specifies bits to clear: if a
   // bit is set in the mask then the corresponding bit is cleared in the cell.
@@ -179,188 +177,25 @@ class Bitmap {
   template <MarkBit::AccessMode mode = MarkBit::NON_ATOMIC>
   void SetBitsInCell(uint32_t cell_index, uint32_t mask);
 
-  // Sets all bits in the range [start_index, end_index).
-  // If the access mode is ATOMIC then the cells at the boundary of the range
-  // are updated with atomic compare and swap operation. The inner cells are
-  // updated with ordinary write, so the caller must ensure exclusive access
-  // to the inner cells.
-  template <MarkBit::AccessMode mode = MarkBit::NON_ATOMIC>
-  void SetRange(uint32_t start_index, uint32_t end_index) {
-    unsigned int start_cell_index = start_index >> Bitmap::kBitsPerCellLog2;
-    MarkBit::CellType start_index_mask = 1u << Bitmap::IndexInCell(start_index);
-
-    unsigned int end_cell_index = end_index >> Bitmap::kBitsPerCellLog2;
-    MarkBit::CellType end_index_mask = 1u << Bitmap::IndexInCell(end_index);
-
-    if (start_cell_index != end_cell_index) {
-      // Firstly, fill all bits from the start address to the end of the first
-      // cell with 1s.
-      SetBitsInCell<mode>(start_cell_index, ~(start_index_mask - 1));
-      // Then fill all in between cells with 1s.
-      base::Atomic32* cell_base = reinterpret_cast<base::Atomic32*>(cells());
-      for (unsigned int i = start_cell_index + 1; i < end_cell_index; i++) {
-        base::Relaxed_Store(cell_base + i, ~0u);
-      }
-      // Finally, fill all bits until the end address in the last cell with 1s.
-      SetBitsInCell<mode>(end_cell_index, (end_index_mask - 1));
-    } else {
-      SetBitsInCell<mode>(start_cell_index, end_index_mask - start_index_mask);
-    }
-  }
+  // Sets all bits in the range [start_index, end_index). The cells at the
+  // boundary of the range are updated with atomic compare and swap operation.
+  // The inner cells are updated with relaxed write.
+  void SetRange(uint32_t start_index, uint32_t end_index);
 
-  // Clears all bits in the range [start_index, end_index).
-  // If the access mode is ATOMIC then the cells at the boundary of the range
-  // are updated with atomic compare and swap operation. The inner cells are
-  // updated with ordinary write, so the caller must ensure exclusive access
-  // to the inner cells.
-  template <MarkBit::AccessMode mode = MarkBit::NON_ATOMIC>
-  void ClearRange(uint32_t start_index, uint32_t end_index) {
-    unsigned int start_cell_index = start_index >> Bitmap::kBitsPerCellLog2;
-    MarkBit::CellType start_index_mask = 1u << Bitmap::IndexInCell(start_index);
-
-    unsigned int end_cell_index = end_index >> Bitmap::kBitsPerCellLog2;
-    MarkBit::CellType end_index_mask = 1u << Bitmap::IndexInCell(end_index);
-
-    if (start_cell_index != end_cell_index) {
-      // Firstly, fill all bits from the start address to the end of the first
-      // cell with 0s.
-      ClearBitsInCell<mode>(start_cell_index, ~(start_index_mask - 1));
-      // Then fill all in between cells with 0s.
-      base::Atomic32* cell_base = reinterpret_cast<base::Atomic32*>(cells());
-      for (unsigned int i = start_cell_index + 1; i < end_cell_index; i++) {
-        base::Relaxed_Store(cell_base + i, 0);
-      }
-      // Finally, set all bits until the end address in the last cell with 0s.
-      ClearBitsInCell<mode>(end_cell_index, (end_index_mask - 1));
-    } else {
-      ClearBitsInCell<mode>(start_cell_index,
-                            (end_index_mask - start_index_mask));
-    }
-  }
+  // Clears all bits in the range [start_index, end_index). The cells at the
+  // boundary of the range are updated with atomic compare and swap operation.
+  // The inner cells are updated with relaxed write.
+  void ClearRange(uint32_t start_index, uint32_t end_index);
 
   // Returns true if all bits in the range [start_index, end_index) are set.
-  bool AllBitsSetInRange(uint32_t start_index, uint32_t end_index) {
-    unsigned int start_cell_index = start_index >> Bitmap::kBitsPerCellLog2;
-    MarkBit::CellType start_index_mask = 1u << Bitmap::IndexInCell(start_index);
-
-    unsigned int end_cell_index = end_index >> Bitmap::kBitsPerCellLog2;
-    MarkBit::CellType end_index_mask = 1u << Bitmap::IndexInCell(end_index);
-
-    MarkBit::CellType matching_mask;
-    if (start_cell_index != end_cell_index) {
-      matching_mask = ~(start_index_mask - 1);
-      if ((cells()[start_cell_index] & matching_mask) != matching_mask) {
-        return false;
-      }
-      for (unsigned int i = start_cell_index + 1; i < end_cell_index; i++) {
-        if (cells()[i] != ~0u) return false;
-      }
-      matching_mask = (end_index_mask - 1);
-      // Check against a mask of 0 to avoid dereferencing the cell after the
-      // end of the bitmap.
-      return (matching_mask == 0) ||
-             ((cells()[end_cell_index] & matching_mask) == matching_mask);
-    } else {
-      matching_mask = end_index_mask - start_index_mask;
-      // Check against a mask of 0 to avoid dereferencing the cell after the
-      // end of the bitmap.
-      return (matching_mask == 0) ||
-             (cells()[end_cell_index] & matching_mask) == matching_mask;
-    }
-  }
+  bool AllBitsSetInRange(uint32_t start_index, uint32_t end_index);
 
   // Returns true if all bits in the range [start_index, end_index) are cleared.
-  bool AllBitsClearInRange(uint32_t start_index, uint32_t end_index) {
-    unsigned int start_cell_index = start_index >> Bitmap::kBitsPerCellLog2;
-    MarkBit::CellType start_index_mask = 1u << Bitmap::IndexInCell(start_index);
-
-    unsigned int end_cell_index = end_index >> Bitmap::kBitsPerCellLog2;
-    MarkBit::CellType end_index_mask = 1u << Bitmap::IndexInCell(end_index);
-
-    MarkBit::CellType matching_mask;
-    if (start_cell_index != end_cell_index) {
-      matching_mask = ~(start_index_mask - 1);
-      if ((cells()[start_cell_index] & matching_mask)) return false;
-      for (unsigned int i = start_cell_index + 1; i < end_cell_index; i++) {
-        if (cells()[i]) return false;
-      }
-      matching_mask = (end_index_mask - 1);
-      // Check against a mask of 0 to avoid dereferencing the cell after the
-      // end of the bitmap.
-      return (matching_mask == 0) || !(cells()[end_cell_index] & matching_mask);
-    } else {
-      matching_mask = end_index_mask - start_index_mask;
-      // Check against a mask of 0 to avoid dereferencing the cell after the
-      // end of the bitmap.
-      return (matching_mask == 0) || !(cells()[end_cell_index] & matching_mask);
-    }
-  }
+  bool AllBitsClearInRange(uint32_t start_index, uint32_t end_index);
 
-  static void PrintWord(uint32_t word, uint32_t himask = 0) {
-    for (uint32_t mask = 1; mask != 0; mask <<= 1) {
-      if ((mask & himask) != 0) PrintF("[");
-      PrintF((mask & word) ? "1" : "0");
-      if ((mask & himask) != 0) PrintF("]");
-    }
-  }
+  void Print();
 
-  class CellPrinter {
-   public:
-    CellPrinter() : seq_start(0), seq_type(0), seq_length(0) {}
-
-    void Print(uint32_t pos, uint32_t cell) {
-      if (cell == seq_type) {
-        seq_length++;
-        return;
-      }
-
-      Flush();
-
-      if (IsSeq(cell)) {
-        seq_start = pos;
-        seq_length = 0;
-        seq_type = cell;
-        return;
-      }
-
-      PrintF("%d: ", pos);
-      PrintWord(cell);
-      PrintF("\n");
-    }
-
-    void Flush() {
-      if (seq_length > 0) {
-        PrintF("%d: %dx%d\n", seq_start, seq_type == 0 ? 0 : 1,
-               seq_length * kBitsPerCell);
-        seq_length = 0;
-      }
-    }
-
-    static bool IsSeq(uint32_t cell) { return cell == 0 || cell == 0xFFFFFFFF; }
-
-   private:
-    uint32_t seq_start;
-    uint32_t seq_type;
-    uint32_t seq_length;
-  };
-
-  void Print() {
-    CellPrinter printer;
-    for (int i = 0; i < CellsCount(); i++) {
-      printer.Print(i, cells()[i]);
-    }
-    printer.Flush();
-    PrintF("\n");
-  }
-
-  bool IsClean() {
-    for (int i = 0; i < CellsCount(); i++) {
-      if (cells()[i] != 0) {
-        return false;
-      }
-    }
-    return true;
-  }
+  bool IsClean();
 };
 
 template <>

src/heap/spaces.cc

@@ -554,7 +554,6 @@ MemoryChunk* MemoryChunk::Initialize(Heap* heap, Address base, size_t size,
   if (reservation != nullptr) {
     chunk->reservation_.TakeControl(reservation);
   }
-
   return chunk;
 }
 
@@ -848,10 +847,8 @@ void Page::CreateBlackArea(Address start, Address end) {
   DCHECK_EQ(Page::FromAddress(start), this);
   DCHECK_NE(start, end);
   DCHECK_EQ(Page::FromAddress(end - 1), this);
-  MarkingState::Internal(this)
-      .bitmap()
-      ->SetRange<IncrementalMarking::kAtomicity>(AddressToMarkbitIndex(start),
-                                                 AddressToMarkbitIndex(end));
+  MarkingState::Internal(this).bitmap()->SetRange(AddressToMarkbitIndex(start),
+                                                  AddressToMarkbitIndex(end));
   MarkingState::Internal(this)
       .IncrementLiveBytes<IncrementalMarking::kAtomicity>(
           static_cast<int>(end - start));
@@ -862,10 +859,8 @@ void Page::DestroyBlackArea(Address start, Address end) {
   DCHECK_EQ(Page::FromAddress(start), this);
   DCHECK_NE(start, end);
   DCHECK_EQ(Page::FromAddress(end - 1), this);
-  MarkingState::Internal(this)
-      .bitmap()
-      ->ClearRange<IncrementalMarking::kAtomicity>(AddressToMarkbitIndex(start),
-                                                   AddressToMarkbitIndex(end));
+  MarkingState::Internal(this).bitmap()->ClearRange(
+      AddressToMarkbitIndex(start), AddressToMarkbitIndex(end));
   MarkingState::Internal(this)
       .IncrementLiveBytes<IncrementalMarking::kAtomicity>(
           -static_cast<int>(end - start));
@@ -1504,11 +1499,9 @@ void PagedSpace::EmptyAllocationInfo() {
 
     // Clear the bits in the unused black area.
     if (current_top != current_limit) {
-      MarkingState::Internal(page)
-          .bitmap()
-          ->ClearRange<IncrementalMarking::kAtomicity>(
-              page->AddressToMarkbitIndex(current_top),
-              page->AddressToMarkbitIndex(current_limit));
+      MarkingState::Internal(page).bitmap()->ClearRange(
+          page->AddressToMarkbitIndex(current_top),
+          page->AddressToMarkbitIndex(current_limit));
       MarkingState::Internal(page)
           .IncrementLiveBytes<IncrementalMarking::kAtomicity>(
               -static_cast<int>(current_limit - current_top));

src/v8.gyp

@@ -1091,6 +1091,7 @@
         'heap/mark-compact-inl.h',
         'heap/mark-compact.cc',
         'heap/mark-compact.h',
+        'heap/marking.cc',
         'heap/marking.h',
         'heap/object-stats.cc',
         'heap/object-stats.h',

test/unittests/heap/marking-unittest.cc

@@ -76,20 +76,6 @@ TEST(Marking, SetAndClearRange) {
   free(bitmap);
 }
 
-TEST(Marking, SetAndClearRangeAtomic) {
-  Bitmap* bitmap = reinterpret_cast<Bitmap*>(
-      calloc(Bitmap::kSize / kPointerSize, kPointerSize));
-  for (int i = 0; i < 3; i++) {
-    bitmap->SetRange<MarkBit::ATOMIC>(i, Bitmap::kBitsPerCell + i);
-    CHECK_EQ(reinterpret_cast<uint32_t*>(bitmap)[0], 0xffffffffu << i);
-    CHECK_EQ(reinterpret_cast<uint32_t*>(bitmap)[1], (1u << i) - 1);
-    bitmap->ClearRange<MarkBit::ATOMIC>(i, Bitmap::kBitsPerCell + i);
-    CHECK_EQ(reinterpret_cast<uint32_t*>(bitmap)[0], 0x0u);
-    CHECK_EQ(reinterpret_cast<uint32_t*>(bitmap)[1], 0x0u);
-  }
-  free(bitmap);
-}
-
 TEST(Marking, ClearMultipleRanges) {
   Bitmap* bitmap = reinterpret_cast<Bitmap*>(
       calloc(Bitmap::kSize / kPointerSize, kPointerSize));