heap: Fix and clean up object start bitmap

This CL introduces the following changes to the experimental
implementation of the object start bitmap, that is evaluated as
a mechanism for resolving inner pointers (behind the flag
v8_enable_conservative_stack_scanning):

- Manually iterate through page objects, instead of using the
  PagedSpaceObjectIterator, for performance (avoid calling
  MakeHeapIterable all the time) and to simplify the handling
  of filler objects.
- Clear bits when reusing evacuated pages of the new space.
- Use the cage base to iterate correctly through code objects.
- Introduce a method for verifying the validity of the object
  start bitmap.
- Minor fixes, additional checks and cleanup.

Bug: v8:12851
Change-Id: I245937ffe6f4b53c4c2dcf5126e8836aec4dc79e
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3675099Reviewed-by: Omer Katz <omerkatz@chromium.org>
Reviewed-by: Dominik Inführ <dinfuehr@chromium.org>
Commit-Queue: Nikolaos Papaspyrou <nikolaos@chromium.org>
Cr-Commit-Position: refs/heads/main@{#80869}

src/heap/conservative-stack-visitor.cc

@@ -4,9 +4,9 @@
 
 #include "src/heap/conservative-stack-visitor.h"
 
-#include "src/execution/isolate-utils-inl.h"
 #include "src/heap/large-spaces.h"
 #include "src/heap/paged-spaces-inl.h"
+#include "src/heap/paged-spaces.h"
 
 namespace v8 {
 namespace internal {

src/heap/heap.cc

@@ -1443,6 +1443,10 @@ void Heap::GarbageCollectionEpilogueInSafepoint(GarbageCollector collector) {
     }
     TRACE_GC(tracer(), GCTracer::Scope::HEAP_EPILOGUE_REDUCE_NEW_SPACE);
     ReduceNewSpaceSize();
+
+#ifdef V8_ENABLE_CONSERVATIVE_STACK_SCANNING
+    new_space()->ClearUnusedObjectStartBitmaps();
+#endif  // V8_ENABLE_CONSERVATIVE_STACK_SCANNING
   }
 
   // Ensure that unmapper task isn't running during full GC. We need access to

src/heap/memory-chunk.cc

@@ -131,7 +131,12 @@ MemoryChunk::MemoryChunk(Heap* heap, BaseSpace* space, size_t chunk_size,
                          VirtualMemory reservation, Executability executable,
                          PageSize page_size)
     : BasicMemoryChunk(heap, space, chunk_size, area_start, area_end,
-                       std::move(reservation)) {
+                       std::move(reservation))
+#ifdef V8_ENABLE_CONSERVATIVE_STACK_SCANNING
+      ,
+      object_start_bitmap_(PtrComprCageBase{heap->isolate()}, area_start)
+#endif
+{
   base::AsAtomicPointer::Release_Store(&slot_set_[OLD_TO_NEW], nullptr);
   base::AsAtomicPointer::Release_Store(&slot_set_[OLD_TO_OLD], nullptr);
   base::AsAtomicPointer::Release_Store(&slot_set_[OLD_TO_SHARED], nullptr);
@@ -196,10 +201,6 @@ MemoryChunk::MemoryChunk(Heap* heap, BaseSpace* space, size_t chunk_size,
   // All pages of a shared heap need to be marked with this flag.
   if (heap->IsShared()) SetFlag(MemoryChunk::IN_SHARED_HEAP);
 
-#ifdef V8_ENABLE_CONSERVATIVE_STACK_SCANNING
-  object_start_bitmap_ = ObjectStartBitmap(area_start_);
-#endif
-
 #ifdef DEBUG
   ValidateOffsets(this);
 #endif

src/heap/new-spaces.cc

@@ -565,6 +565,10 @@ void NewSpace::VerifyImpl(Isolate* isolate, const Page* current_page,
     CHECK_EQ(bytes,
              ExternalBackingStoreBytes(ExternalBackingStoreType::kArrayBuffer));
   }
+
+#ifdef V8_ENABLED_CONSERVATIVE_STACK_SCANNING
+  page->object_start_bitmap()->Verify();
+#endif
 }
 #endif
 
@@ -781,6 +785,15 @@ void SemiSpaceNewSpace::Verify(Isolate* isolate) const {
 }
 #endif
 
+#ifdef V8_ENABLE_CONSERVATIVE_STACK_SCANNING
+void SemiSpaceNewSpace::ClearUnusedObjectStartBitmaps() {
+  if (!IsFromSpaceCommitted()) return;
+  for (Page* page : PageRange(from_space().first_page(), nullptr)) {
+    page->object_start_bitmap()->Clear();
+  }
+}
+#endif  // V8_ENABLE_CONSERVATIVE_STACK_SCANNING
+
 bool SemiSpaceNewSpace::ShouldBePromoted(Address address) const {
   Page* page = Page::FromAddress(address);
   Address current_age_mark = age_mark();

src/heap/new-spaces.h

@@ -301,6 +301,10 @@ class NewSpace : NON_EXPORTED_BASE(public SpaceWithLinearArea) {
                   Address current_address) const;
 #endif
 
+#ifdef V8_ENABLE_CONSERVATIVE_STACK_SCANNING
+  virtual void ClearUnusedObjectStartBitmaps() = 0;
+#endif  // V8_ENABLE_CONSERVATIVE_STACK_SCANNING
+
   virtual iterator begin() = 0;
   virtual iterator end() = 0;
 
@@ -473,6 +477,10 @@ class V8_EXPORT_PRIVATE SemiSpaceNewSpace final : public NewSpace {
   void Print() override { to_space_.Print(); }
 #endif
 
+#ifdef V8_ENABLE_CONSERVATIVE_STACK_SCANNING
+  void ClearUnusedObjectStartBitmaps() override;
+#endif  // V8_ENABLE_CONSERVATIVE_STACK_SCANNING
+
   Page* first_page() final { return to_space_.first_page(); }
   Page* last_page() final { return to_space_.last_page(); }
 

src/heap/object-start-bitmap-inl.h

@@ -8,12 +8,9 @@
 #include <limits.h>
 #include <stdint.h>
 
-#include <array>
-
 #include "include/v8-internal.h"
 #include "src/base/bits.h"
 #include "src/base/macros.h"
-#include "src/common/globals.h"
 #include "src/heap/object-start-bitmap.h"
 #include "src/heap/paged-spaces-inl.h"
 #include "src/heap/paged-spaces.h"
@@ -21,7 +18,8 @@
 namespace v8 {
 namespace internal {
 
-ObjectStartBitmap::ObjectStartBitmap(size_t offset) : offset_(offset) {
+ObjectStartBitmap::ObjectStartBitmap(PtrComprCageBase cage_base, size_t offset)
+    : cage_base_(cage_base), offset_(offset) {
   Clear();
 }
 
@@ -50,25 +48,35 @@ Address ObjectStartBitmap::FindBasePtrImpl(Address maybe_inner_ptr) const {
 }
 
 Address ObjectStartBitmap::FindBasePtr(Address maybe_inner_ptr) {
-  Address baseptr = FindBasePtrImpl(maybe_inner_ptr);
-  if (baseptr == maybe_inner_ptr) {
-    DCHECK(CheckBit(baseptr));
-    return baseptr;
+  Address base_ptr = FindBasePtrImpl(maybe_inner_ptr);
+  if (base_ptr == maybe_inner_ptr) {
+    DCHECK(CheckBit(base_ptr));
+    return base_ptr;
   }
   // TODO(v8:12851): If the ObjectStartBitmap implementation stays, this part of
-  // code involving Page and the PagedSpaceObjectIterator is its only connection
-  // with V8 internals. It should be moved to some different abstraction.
-  Page* page = Page::FromAddress(offset_);
+  // code involving the Page and the iteration through its objects is the only
+  // connection with V8 internals. It should be moved to some different
+  // abstraction.
+  const Page* page = Page::FromAddress(offset_);
   DCHECK_EQ(page->area_start(), offset_);
-  if (baseptr == kNullAddress) baseptr = offset_;
-  DCHECK_LE(baseptr, maybe_inner_ptr);
-  PagedSpaceObjectIterator it(
-      page->heap(), static_cast<PagedSpace*>(page->owner()), page, baseptr);
-  for (HeapObject obj = it.Next(); !obj.is_null(); obj = it.Next()) {
-    Address start = obj.address();
-    SetBit(start);
-    if (maybe_inner_ptr < start) break;
-    if (maybe_inner_ptr < start + obj.Size()) return start;
+
+#ifdef VERIFY_HEAP
+  if (FLAG_verify_heap) {
+    Verify();
+  }
+#endif  // VERIFY_HEAP
+
+  if (base_ptr == kNullAddress) base_ptr = offset_;
+  DCHECK_LE(base_ptr, maybe_inner_ptr);
+
+  const Address limit = page->area_end();
+  while (base_ptr < limit) {
+    SetBit(base_ptr);
+    if (maybe_inner_ptr < base_ptr) break;
+    const int size = HeapObject::FromAddress(base_ptr).Size(cage_base());
+    if (maybe_inner_ptr < base_ptr + size) return base_ptr;
+    base_ptr += size;
+    DCHECK_LE(base_ptr, limit);
   }
   return kNullAddress;
 }
@@ -100,8 +108,6 @@ uint32_t ObjectStartBitmap::load(size_t cell_index) const {
   return object_start_bit_map_[cell_index];
 }
 
-Address ObjectStartBitmap::offset() const { return offset_; }
-
 void ObjectStartBitmap::ObjectStartIndexAndBit(Address base_ptr,
                                                size_t* cell_index,
                                                size_t* bit) const {
@@ -140,6 +146,22 @@ void ObjectStartBitmap::Clear() {
   std::fill(object_start_bit_map_.begin(), object_start_bit_map_.end(), 0);
 }
 
+#ifdef VERIFY_HEAP
+void ObjectStartBitmap::Verify() const {
+  Page* page = Page::FromAddress(offset_);
+  DCHECK_EQ(page->area_start(), offset_);
+  Address next_object_in_page = page->area_start();
+  const PtrComprCageBase cage = cage_base();
+  Iterate([&next_object_in_page, cage](Address next_object_in_bitmap) {
+    while (next_object_in_page != next_object_in_bitmap) {
+      DCHECK_LT(next_object_in_page, next_object_in_bitmap);
+      next_object_in_page +=
+          HeapObject::FromAddress(next_object_in_page).Size(cage);
+    }
+  });
+}
+#endif  // VERIFY_HEAP
+
 }  // namespace internal
 }  // namespace v8
 

src/heap/object-start-bitmap.h

@@ -36,7 +36,7 @@ class V8_EXPORT_PRIVATE ObjectStartBitmap {
     return kReservedForBitmap * kBitsPerCell;
   }
 
-  explicit inline ObjectStartBitmap(size_t offset = 0);
+  inline ObjectStartBitmap(PtrComprCageBase cage_base, size_t offset);
 
   // Finds an object header based on a maybe_inner_ptr. If the object start
   // bitmap is not fully populated, this iterates through the objects of the
@@ -59,11 +59,19 @@ class V8_EXPORT_PRIVATE ObjectStartBitmap {
   // Clear the object start bitmap.
   inline void Clear();
 
+#ifdef VERIFY_HEAP
+  // This method verifies that the object start bitmap is consistent with the
+  // page's contents. That is, the bits that are set correspond to existing
+  // objects in the page.
+  inline void Verify() const;
+#endif  // VERIFY_HEAP
+
  private:
   inline void store(size_t cell_index, uint32_t value);
   inline uint32_t load(size_t cell_index) const;
 
-  inline Address offset() const;
+  PtrComprCageBase cage_base() const { return cage_base_; }
+  Address offset() const { return offset_; };
 
   static constexpr size_t kBitsPerCell = sizeof(uint32_t) * CHAR_BIT;
   static constexpr size_t kCellMask = kBitsPerCell - 1;
@@ -83,6 +91,7 @@ class V8_EXPORT_PRIVATE ObjectStartBitmap {
   // pointer of a previous object on the page.
   inline Address FindBasePtrImpl(Address maybe_inner_ptr) const;
 
+  PtrComprCageBase cage_base_;
   size_t offset_;
 
   std::array<uint32_t, kReservedForBitmap> object_start_bit_map_;

src/heap/paged-spaces.cc

@@ -77,6 +77,8 @@ PagedSpaceObjectIterator::PagedSpaceObjectIterator(Heap* heap,
 #endif  // V8_COMPRESS_POINTERS
 {
   heap->MakeHeapIterable();
+  DCHECK_IMPLIES(space->IsInlineAllocationEnabled(),
+                 !page->Contains(space->top()));
   DCHECK(page->Contains(start_address));
   DCHECK(page->SweepingDone());
 }
@@ -819,6 +821,10 @@ void PagedSpaceBase::Verify(Isolate* isolate, ObjectVisitor* visitor) const {
 
     CHECK(!page->IsFlagSet(Page::PAGE_NEW_OLD_PROMOTION));
     CHECK(!page->IsFlagSet(Page::PAGE_NEW_NEW_PROMOTION));
+
+#ifdef V8_ENABLED_CONSERVATIVE_STACK_SCANNING
+    page->object_start_bitmap()->Verify();
+#endif
   }
   for (int i = 0; i < kNumTypes; i++) {
     if (i == ExternalBackingStoreType::kArrayBuffer) continue;

src/heap/scavenger-inl.h

@@ -14,6 +14,10 @@
 #include "src/objects/objects-inl.h"
 #include "src/objects/slots-inl.h"
 
+#ifdef V8_ENABLE_CONSERVATIVE_STACK_SCANNING
+#include "src/heap/object-start-bitmap-inl.h"
+#endif
+
 namespace v8 {
 namespace internal {
 
@@ -107,6 +111,7 @@ bool Scavenger::MigrateObject(Map map, HeapObject source, HeapObject target,
     heap()->incremental_marking()->TransferColor(source, target);
   }
   heap()->UpdateAllocationSite(map, source, &local_pretenuring_feedback_);
+
   return true;
 }
 

test/cctest/heap/test-mark-compact.cc

@@ -227,7 +227,7 @@ HEAP_TEST(DoNotEvacuatePinnedPages) {
 }
 
 HEAP_TEST(ObjectStartBitmap) {
-#if V8_ENABLE_CONSERVATIVE_STACK_SCANNING
+#ifdef V8_ENABLE_CONSERVATIVE_STACK_SCANNING
   CcTest::InitializeVM();
   Isolate* isolate = CcTest::i_isolate();
   v8::HandleScope sc(CcTest::isolate());
@@ -242,8 +242,8 @@ HEAP_TEST(ObjectStartBitmap) {
 
   HeapObject obj1 = *h1;
   HeapObject obj2 = *h2;
-  Page* page1 = Page::FromAddress(obj1.ptr());
-  Page* page2 = Page::FromAddress(obj2.ptr());
+  Page* page1 = Page::FromHeapObject(obj1);
+  Page* page2 = Page::FromHeapObject(obj2);
 
   CHECK(page1->object_start_bitmap()->CheckBit(obj1.address()));
   CHECK(page2->object_start_bitmap()->CheckBit(obj2.address()));
@@ -274,8 +274,8 @@ HEAP_TEST(ObjectStartBitmap) {
 
   obj1 = *h1;
   obj2 = HeapObject::FromAddress(h2->address() - offset);
-  page1 = Page::FromAddress(obj1.ptr());
-  page2 = Page::FromAddress(obj2.ptr());
+  page1 = Page::FromHeapObject(obj1);
+  page2 = Page::FromHeapObject(obj2);
 
   CHECK(obj1.IsString());
   CHECK(obj2.IsString());

test/unittests/heap/object-start-bitmap-unittest.cc

@@ -23,6 +23,7 @@ bool IsEmpty(const ObjectStartBitmap& bitmap) {
 // the base address as getting either of it wrong will result in failed DCHECKs.
 class TestObject {
  public:
+  static PtrComprCageBase kCageBase;
   static Address kBaseOffset;
 
   explicit TestObject(size_t number) : number_(number) {
@@ -41,6 +42,7 @@ class TestObject {
   const size_t number_;
 };
 
+PtrComprCageBase TestObject::kCageBase{0xca6e00000000ul};
 Address TestObject::kBaseOffset = reinterpret_cast<Address>(0x4000ul);
 
 }  // namespace
@@ -51,25 +53,25 @@ TEST(V8ObjectStartBitmapTest, MoreThanZeroEntriesPossible) {
 }
 
 TEST(V8ObjectStartBitmapTest, InitialEmpty) {
-  ObjectStartBitmap bitmap(TestObject::kBaseOffset);
+  ObjectStartBitmap bitmap(TestObject::kCageBase, TestObject::kBaseOffset);
   EXPECT_TRUE(IsEmpty(bitmap));
 }
 
 TEST(V8ObjectStartBitmapTest, SetBitImpliesNonEmpty) {
-  ObjectStartBitmap bitmap(TestObject::kBaseOffset);
+  ObjectStartBitmap bitmap(TestObject::kCageBase, TestObject::kBaseOffset);
   bitmap.SetBit(TestObject(0));
   EXPECT_FALSE(IsEmpty(bitmap));
 }
 
 TEST(V8ObjectStartBitmapTest, SetBitCheckBit) {
-  ObjectStartBitmap bitmap(TestObject::kBaseOffset);
+  ObjectStartBitmap bitmap(TestObject::kCageBase, TestObject::kBaseOffset);
   TestObject object(7);
   bitmap.SetBit(object);
   EXPECT_TRUE(bitmap.CheckBit(object));
 }
 
 TEST(V8ObjectStartBitmapTest, SetBitClearbitCheckBit) {
-  ObjectStartBitmap bitmap(TestObject::kBaseOffset);
+  ObjectStartBitmap bitmap(TestObject::kCageBase, TestObject::kBaseOffset);
   TestObject object(77);
   bitmap.SetBit(object);
   bitmap.ClearBit(object);
@@ -77,7 +79,7 @@ TEST(V8ObjectStartBitmapTest, SetBitClearbitCheckBit) {
 }
 
 TEST(V8ObjectStartBitmapTest, SetBitClearBitImpliesEmpty) {
-  ObjectStartBitmap bitmap(TestObject::kBaseOffset);
+  ObjectStartBitmap bitmap(TestObject::kCageBase, TestObject::kBaseOffset);
   TestObject object(123);
   bitmap.SetBit(object);
   bitmap.ClearBit(object);
@@ -85,7 +87,7 @@ TEST(V8ObjectStartBitmapTest, SetBitClearBitImpliesEmpty) {
 }
 
 TEST(V8ObjectStartBitmapTest, AdjacentObjectsAtBegin) {
-  ObjectStartBitmap bitmap(TestObject::kBaseOffset);
+  ObjectStartBitmap bitmap(TestObject::kCageBase, TestObject::kBaseOffset);
   TestObject object0(0);
   TestObject object1(1);
   bitmap.SetBit(object0);
@@ -104,7 +106,7 @@ TEST(V8ObjectStartBitmapTest, AdjacentObjectsAtBegin) {
 }
 
 TEST(V8ObjectStartBitmapTest, AdjacentObjectsAtEnd) {
-  ObjectStartBitmap bitmap(TestObject::kBaseOffset);
+  ObjectStartBitmap bitmap(TestObject::kCageBase, TestObject::kBaseOffset);
   const size_t last_entry_index = ObjectStartBitmap::MaxEntries() - 1;
   TestObject object0(last_entry_index - 1);
   TestObject object1(last_entry_index);
@@ -124,7 +126,7 @@ TEST(V8ObjectStartBitmapTest, AdjacentObjectsAtEnd) {
 }
 
 TEST(V8ObjectStartBitmapTest, FindBasePtrExact) {
-  ObjectStartBitmap bitmap(TestObject::kBaseOffset);
+  ObjectStartBitmap bitmap(TestObject::kCageBase, TestObject::kBaseOffset);
   TestObject object(654);
   bitmap.SetBit(object);
   EXPECT_EQ(object.base_ptr(), bitmap.FindBasePtrImpl(object.base_ptr()));
@@ -132,7 +134,7 @@ TEST(V8ObjectStartBitmapTest, FindBasePtrExact) {
 
 TEST(V8ObjectStartBitmapTest, FindBasePtrApproximate) {
   const size_t kInternalDelta = 37;
-  ObjectStartBitmap bitmap(TestObject::kBaseOffset);
+  ObjectStartBitmap bitmap(TestObject::kCageBase, TestObject::kBaseOffset);
   TestObject object(654);
   bitmap.SetBit(object);
   EXPECT_EQ(object.base_ptr(),
@@ -141,7 +143,7 @@ TEST(V8ObjectStartBitmapTest, FindBasePtrApproximate) {
 
 TEST(V8ObjectStartBitmapTest, FindBasePtrIteratingWholeBitmap) {
   const size_t kLastWordDelta = ObjectStartBitmap::MaxEntries() - 1;
-  ObjectStartBitmap bitmap(TestObject::kBaseOffset);
+  ObjectStartBitmap bitmap(TestObject::kCageBase, TestObject::kBaseOffset);
   TestObject object_to_find(0);
   bitmap.SetBit(object_to_find);
   Address hint_index = TestObject(kLastWordDelta);
@@ -151,7 +153,7 @@ TEST(V8ObjectStartBitmapTest, FindBasePtrIteratingWholeBitmap) {
 TEST(V8ObjectStartBitmapTest, FindBasePtrNextCell) {
   // This white box test makes use of the fact that cells are of type uint32_t.
   const size_t kCellSize = sizeof(uint32_t);
-  ObjectStartBitmap bitmap(TestObject::kBaseOffset);
+  ObjectStartBitmap bitmap(TestObject::kCageBase, TestObject::kBaseOffset);
   TestObject object_to_find(kCellSize - 1);
   Address hint = TestObject(kCellSize);
   bitmap.SetBit(TestObject(0));
@@ -162,7 +164,7 @@ TEST(V8ObjectStartBitmapTest, FindBasePtrNextCell) {
 TEST(V8ObjectStartBitmapTest, FindBasePtrSameCell) {
   // This white box test makes use of the fact that cells are of type uint32_t.
   const size_t kCellSize = sizeof(uint32_t);
-  ObjectStartBitmap bitmap(TestObject::kBaseOffset);
+  ObjectStartBitmap bitmap(TestObject::kCageBase, TestObject::kBaseOffset);
   TestObject object_to_find(kCellSize - 1);
   Address hint = object_to_find;
   bitmap.SetBit(TestObject(0));