heap: Clean up conservative stack scanning prototype

This CL cleans up the existing experimental implementation of
conservative stack scanning. It retains the object start bitmap, to
evaluate it as a mechanism for resolving inner pointers, and the
conservative stack scanning visitor (which is currently not used).

The flag v8_enable_conservative_stack_scanning is kept and will be
used for experimental purposes. It currently does not imply any
other flag.

Bug: v8:10614
Bug: v8:12851

Change-Id: Id0ae0f437ed2601eed9ec634d2d1dd2f030d814e
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3602516Reviewed-by: Omer Katz <omerkatz@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Jakob Linke <jgruber@chromium.org>
Commit-Queue: Nikolaos Papaspyrou <nikolaos@chromium.org>
Cr-Commit-Position: refs/heads/main@{#80365}

BUILD.gn

@@ -567,9 +567,6 @@ if (v8_enable_single_generation == true) {
       "Requires unconditional write barriers or none (which disables incremental marking)")
 }
 
-assert(!v8_enable_conservative_stack_scanning || v8_enable_single_generation,
-       "Conservative stack scanning requires single generation")
-
 if (v8_fuchsia_use_vmex_resource) {
   assert(target_os == "fuchsia", "VMEX resource only available on Fuchsia")
 }

src/execution/isolate.cc

@@ -23,6 +23,7 @@
 #include "src/base/logging.h"
 #include "src/base/platform/mutex.h"
 #include "src/base/platform/platform.h"
+#include "src/base/platform/wrappers.h"
 #include "src/base/sys-info.h"
 #include "src/base/utils/random-number-generator.h"
 #include "src/baseline/baseline-batch-compiler.h"
@@ -132,11 +133,6 @@
 #include "src/diagnostics/unwinding-info-win64.h"
 #endif  // V8_OS_WIN64
 
-#ifdef V8_ENABLE_CONSERVATIVE_STACK_SCANNING
-#include "src/base/platform/wrappers.h"
-#include "src/heap/conservative-stack-visitor.h"
-#endif
-
 #if USE_SIMULATOR
 #include "src/execution/simulator-base.h"
 #endif
@@ -570,11 +566,6 @@ void Isolate::Iterate(RootVisitor* v, ThreadLocalTop* thread) {
         FullObjectSlot(reinterpret_cast<Address>(&(block->message_obj_))));
   }
 
-#ifdef V8_ENABLE_CONSERVATIVE_STACK_SCANNING
-  ConservativeStackVisitor stack_visitor(this, v);
-  thread_local_top()->stack_.IteratePointers(&stack_visitor);
-#endif
-
   // Iterate over pointers on native execution stack.
 #if V8_ENABLE_WEBASSEMBLY
   wasm::WasmCodeRefScope wasm_code_ref_scope;

src/execution/thread-local-top.cc

@@ -37,9 +37,6 @@ void ThreadLocalTop::Clear() {
   current_embedder_state_ = nullptr;
   failed_access_check_callback_ = nullptr;
   thread_in_wasm_flag_address_ = kNullAddress;
-#ifdef V8_ENABLE_CONSERVATIVE_STACK_SCANNING
-  stack_ = ::heap::base::Stack(nullptr);
-#endif
 }
 
 void ThreadLocalTop::Initialize(Isolate* isolate) {
@@ -53,10 +50,6 @@ void ThreadLocalTop::Initialize(Isolate* isolate) {
 #ifdef USE_SIMULATOR
   simulator_ = Simulator::current(isolate);
 #endif
-
-#ifdef V8_ENABLE_CONSERVATIVE_STACK_SCANNING
-  stack_ = ::heap::base::Stack(base::Stack::GetStackStart());
-#endif
 }
 
 void ThreadLocalTop::Free() {}

src/execution/thread-local-top.h

@@ -13,10 +13,6 @@
 #include "src/objects/contexts.h"
 #include "src/utils/utils.h"
 
-#ifdef V8_ENABLE_CONSERVATIVE_STACK_SCANNING
-#include "src/heap/base/stack.h"
-#endif
-
 namespace v8 {
 
 class TryCatch;
@@ -33,11 +29,7 @@ class ThreadLocalTop {
   // TODO(all): This is not particularly beautiful. We should probably
   // refactor this to really consist of just Addresses and 32-bit
   // integer fields.
-#ifdef V8_ENABLE_CONSERVATIVE_STACK_SCANNING
-  static constexpr uint32_t kSizeInBytes = 26 * kSystemPointerSize;
-#else
   static constexpr uint32_t kSizeInBytes = 25 * kSystemPointerSize;
-#endif
 
   // Does early low-level initialization that does not depend on the
   // isolate being present.
@@ -154,10 +146,6 @@ class ThreadLocalTop {
 
   // Address of the thread-local "thread in wasm" flag.
   Address thread_in_wasm_flag_address_;
-
-#ifdef V8_ENABLE_CONSERVATIVE_STACK_SCANNING
-  ::heap::base::Stack stack_;
-#endif
 };
 
 }  // namespace internal

src/heap/conservative-stack-visitor.cc

@@ -37,25 +37,26 @@ bool ConservativeStackVisitor::CheckPage(Address address, MemoryChunk* page) {
     return false;
   }
 
-  // TODO(jakehughes) Pinning is only required for the marking visitor. Other
-  // visitors (such as verify visitor) could work without pining. This should
-  // be moved to delegate_
-  page->SetFlag(BasicMemoryChunk::Flag::PINNED);
-
-  Object ptr = HeapObject::FromAddress(base_ptr);
-  FullObjectSlot root = FullObjectSlot(&ptr);
-  delegate_->VisitRootPointer(Root::kHandleScope, nullptr, root);
-  DCHECK(root == FullObjectSlot(reinterpret_cast<Address>(&base_ptr)));
+  Object root = obj;
+  delegate_->VisitRootPointer(Root::kHandleScope, nullptr,
+                              FullObjectSlot(&root));
+  // Check that the delegate visitor did not modify the root slot.
+  DCHECK_EQ(root, obj);
   return true;
 }
 
 void ConservativeStackVisitor::VisitConservativelyIfPointer(
     const void* pointer) {
   auto address = reinterpret_cast<Address>(pointer);
+  // TODO(v8:12851): Let's figure out what this meant to do...
+  // This condition is always true, as the LAB invariant requires
+  // start <= top <= limit
+#if 0
   if (address > isolate_->heap()->old_space()->top() ||
       address < isolate_->heap()->old_space()->limit()) {
     return;
   }
+#endif
 
   for (Page* page : *isolate_->heap()->old_space()) {
     if (CheckPage(address, page)) {

src/heap/heap.cc

@@ -18,6 +18,7 @@
 #include "src/base/logging.h"
 #include "src/base/once.h"
 #include "src/base/platform/mutex.h"
+#include "src/base/platform/wrappers.h"
 #include "src/base/utils/random-number-generator.h"
 #include "src/builtins/accessors.h"
 #include "src/codegen/assembler-inl.h"
@@ -34,7 +35,6 @@
 #include "src/execution/vm-state-inl.h"
 #include "src/handles/global-handles-inl.h"
 #include "src/heap/array-buffer-sweeper.h"
-#include "src/heap/base/stack.h"
 #include "src/heap/basic-memory-chunk.h"
 #include "src/heap/code-object-registry.h"
 #include "src/heap/code-range.h"
@@ -102,12 +102,6 @@
 #include "src/tracing/trace-event.h"
 #include "src/utils/utils-inl.h"
 #include "src/utils/utils.h"
-
-#ifdef V8_ENABLE_CONSERVATIVE_STACK_SCANNING
-#include "src/heap/conservative-stack-visitor.h"
-#endif
-
-#include "src/base/platform/wrappers.h"
 // Has to be the last include (doesn't have include guards):
 #include "src/objects/object-macros.h"
 
@@ -5062,7 +5056,6 @@ void Heap::IterateRoots(RootVisitor* v, base::EnumSet<SkipRoot> options) {
       v->Synchronize(VisitorSynchronization::kStackRoots);
     }
 
-#ifndef V8_ENABLE_CONSERVATIVE_STACK_SCANNING
     // Iterate over main thread handles in handle scopes.
     if (!options.contains(SkipRoot::kMainThreadHandles)) {
       // Clear main thread handles with stale references to left-trimmed
@@ -5072,7 +5065,6 @@ void Heap::IterateRoots(RootVisitor* v, base::EnumSet<SkipRoot> options) {
 
       isolate_->handle_scope_implementer()->Iterate(v);
     }
-#endif
 
     // Iterate local handles for all local heaps.
     safepoint_->Iterate(v);

src/heap/mark-compact.cc

@@ -868,11 +868,6 @@ void MarkCompactCollector::CollectEvacuationCandidates(PagedSpace* space) {
     } else {
       pages.push_back(std::make_pair(p->allocated_bytes(), p));
     }
-
-    // Unpin pages for the next GC
-    if (p->IsFlagSet(MemoryChunk::PINNED)) {
-      p->ClearFlag(MemoryChunk::PINNED);
-    }
   }
 
   int candidate_count = 0;

src/heap/memory-chunk.cc

@@ -197,7 +197,7 @@ MemoryChunk::MemoryChunk(Heap* heap, BaseSpace* space, size_t chunk_size,
   if (heap->IsShared()) SetFlag(MemoryChunk::IN_SHARED_HEAP);
 
 #ifdef V8_ENABLE_CONSERVATIVE_STACK_SCANNING
-  chunk->object_start_bitmap_ = ObjectStartBitmap(chunk->area_start());
+  object_start_bitmap_ = ObjectStartBitmap(area_start_);
 #endif
 
 #ifdef DEBUG

src/heap/object-start-bitmap.h

@@ -99,45 +99,43 @@ Address ObjectStartBitmap::FindBasePtr(Address maybe_inner_ptr) const {
   DCHECK_GT(object_start_bit_map_.size(), cell_index);
   const size_t bit = object_start_number & kCellMask;
   // check if maybe_inner_ptr is the base pointer
-  uint32_t byte = load(cell_index) & ((1 << (bit + 1)) - 1);
-  while (!byte && cell_index) {
-    DCHECK_LT(0u, cell_index);
+  uint32_t byte =
+      load(cell_index) & static_cast<uint32_t>((1 << (bit + 1)) - 1);
+  while (byte == 0 && cell_index > 0) {
     byte = load(--cell_index);
   }
-  const int leading_zeroes = v8::base::bits::CountLeadingZeros(byte);
-  if (leading_zeroes == kBitsPerCell) {
+  if (byte == 0) {
+    DCHECK_EQ(0, cell_index);
     return kNullAddress;
   }
-
+  const int leading_zeroes = v8::base::bits::CountLeadingZeros(byte);
+  DCHECK_GT(kBitsPerCell, leading_zeroes);
   object_start_number =
       (cell_index * kBitsPerCell) + (kBitsPerCell - 1) - leading_zeroes;
-  Address base_ptr = StartIndexToAddress(object_start_number);
-  return base_ptr;
+  return StartIndexToAddress(object_start_number);
 }
 
 void ObjectStartBitmap::SetBit(Address base_ptr) {
   size_t cell_index, object_bit;
   ObjectStartIndexAndBit(base_ptr, &cell_index, &object_bit);
-  store(cell_index,
-        static_cast<uint32_t>(load(cell_index) | (1 << object_bit)));
+  store(cell_index, load(cell_index) | static_cast<uint32_t>(1 << object_bit));
 }
 
 void ObjectStartBitmap::ClearBit(Address base_ptr) {
   size_t cell_index, object_bit;
   ObjectStartIndexAndBit(base_ptr, &cell_index, &object_bit);
   store(cell_index,
-        static_cast<uint32_t>(load(cell_index) & ~(1 << object_bit)));
+        load(cell_index) & static_cast<uint32_t>(~(1 << object_bit)));
 }
 
 bool ObjectStartBitmap::CheckBit(Address base_ptr) const {
   size_t cell_index, object_bit;
   ObjectStartIndexAndBit(base_ptr, &cell_index, &object_bit);
-  return load(cell_index) & (1 << object_bit);
+  return (load(cell_index) & static_cast<uint32_t>(1 << object_bit)) != 0;
 }
 
 void ObjectStartBitmap::store(size_t cell_index, uint32_t value) {
   object_start_bit_map_[cell_index] = value;
-  return;
 }
 
 uint32_t ObjectStartBitmap::load(size_t cell_index) const {
@@ -165,15 +163,17 @@ Address ObjectStartBitmap::StartIndexToAddress(
 template <typename Callback>
 inline void ObjectStartBitmap::Iterate(Callback callback) const {
   for (size_t cell_index = 0; cell_index < kReservedForBitmap; cell_index++) {
-    uint32_t value = object_start_bit_map_[cell_index];
-    while (value) {
-      const int trailing_zeroes = v8::base::bits::CountTrailingZeros(value);
+    uint32_t value = load(cell_index);
+    while (value != 0) {
+      const int trailing_zeroes =
+          v8::base::bits::CountTrailingZerosNonZero(value);
+      DCHECK_GT(kBitsPerCell, trailing_zeroes);
       const size_t object_start_number =
           (cell_index * kBitsPerCell) + trailing_zeroes;
       const Address object_address = StartIndexToAddress(object_start_number);
       callback(object_address);
       // Clear current object bit in temporary value to advance iteration.
-      value &= ~(1 << (object_start_number & kCellMask));
+      value &= value - 1;
     }
   }
 }

src/heap/paged-spaces.cc

@@ -77,6 +77,7 @@ bool PagedSpaceObjectIterator::AdvanceToNextPage() {
   DCHECK(cur_page->SweepingDone());
   return true;
 }
+
 Page* PagedSpace::InitializePage(MemoryChunk* chunk) {
   Page* page = static_cast<Page*>(chunk);
   DCHECK_EQ(

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

@@ -227,10 +227,7 @@ HEAP_TEST(DoNotEvacuatePinnedPages) {
 }
 
 HEAP_TEST(ObjectStartBitmap) {
-  if (!FLAG_single_generation || !FLAG_conservative_stack_scanning) return;
-
 #if V8_ENABLE_CONSERVATIVE_STACK_SCANNING
-
   CcTest::InitializeVM();
   Isolate* isolate = CcTest::i_isolate();
   v8::HandleScope sc(CcTest::isolate());
@@ -239,28 +236,67 @@ HEAP_TEST(ObjectStartBitmap) {
   heap::SealCurrentObjects(heap);
 
   auto* factory = isolate->factory();
-  HeapObject obj = *factory->NewStringFromStaticChars("hello");
-  HeapObject obj2 = *factory->NewStringFromStaticChars("world");
-  Page* page = Page::FromAddress(obj.ptr());
 
-  CHECK(page->object_start_bitmap()->CheckBit(obj.address()));
-  CHECK(page->object_start_bitmap()->CheckBit(obj2.address()));
+  // TODO(v8:12851): Using handles because conservative stack scanning currently
+  // does not work.
+  Handle<String> h1 = factory->NewStringFromStaticChars("hello");
+  Handle<String> h2 = factory->NewStringFromStaticChars("world");
+
+  HeapObject obj1 = *h1;
+  HeapObject obj2 = *h2;
+  Page* page1 = Page::FromAddress(obj1.ptr());
+  Page* page2 = Page::FromAddress(obj2.ptr());
 
-  Address obj_inner_ptr = obj.ptr() + 2;
-  CHECK(page->object_start_bitmap()->FindBasePtr(obj_inner_ptr) ==
-        obj.address());
+  CHECK(page1->object_start_bitmap()->CheckBit(obj1.address()));
+  CHECK(page2->object_start_bitmap()->CheckBit(obj2.address()));
 
-  Address obj2_inner_ptr = obj2.ptr() + 2;
-  CHECK(page->object_start_bitmap()->FindBasePtr(obj2_inner_ptr) ==
-        obj2.address());
+  for (int k = 0; k < obj1.Size(); ++k) {
+    Address obj1_inner_ptr = obj1.address() + k;
+    CHECK_EQ(obj1.address(),
+             page1->object_start_bitmap()->FindBasePtr(obj1_inner_ptr));
+  }
+  for (int k = 0; k < obj2.Size(); ++k) {
+    Address obj2_inner_ptr = obj2.address() + k;
+    CHECK_EQ(obj2.address(),
+             page2->object_start_bitmap()->FindBasePtr(obj2_inner_ptr));
+  }
 
   CcTest::CollectAllGarbage();
 
-  CHECK((obj).IsString());
-  CHECK((obj2).IsString());
-  CHECK(page->object_start_bitmap()->CheckBit(obj.address()));
-  CHECK(page->object_start_bitmap()->CheckBit(obj2.address()));
+  obj1 = *h1;
+  obj2 = *h2;
+  page1 = Page::FromAddress(obj1.ptr());
+  page2 = Page::FromAddress(obj2.ptr());
+
+  CHECK(obj1.IsString());
+  CHECK(obj2.IsString());
+
+  // TODO(v8:12851): Bits set in the object_start_bitmap are currently not
+  // preserved when objects are evacuated. For this reason, in the following
+  // checks, FindBasePtr is expected to return null after garbage collection.
+  for (int k = 0; k < obj1.Size(); ++k) {
+    Address obj1_inner_ptr = obj1.address() + k;
+    CHECK_EQ(kNullAddress,
+             page1->object_start_bitmap()->FindBasePtr(obj1_inner_ptr));
+  }
+  for (int k = 0; k < obj2.Size(); ++k) {
+    Address obj2_inner_ptr = obj2.address() + k;
+    CHECK_EQ(kNullAddress,
+             page2->object_start_bitmap()->FindBasePtr(obj2_inner_ptr));
+  }
+
+  obj1 = *h1;
+  obj2 = *h2;
+  page1 = Page::FromAddress(obj1.ptr());
+  page2 = Page::FromAddress(obj2.ptr());
 
+  CHECK(obj1.IsString());
+  CHECK(obj2.IsString());
+
+  // TODO(v8:12851): Bits set in the object_start_bitmap are currently not
+  // preserved when objects are evacuated.
+  CHECK(!page1->object_start_bitmap()->CheckBit(obj1.address()));
+  CHECK(!page2->object_start_bitmap()->CheckBit(obj2.address()));
 #endif
 }
 
@@ -270,7 +306,6 @@ static Handle<Map> CreateMap(Isolate* isolate) {
   return isolate->factory()->NewMap(JS_OBJECT_TYPE, JSObject::kHeaderSize);
 }
 
-
 TEST(MapCompact) {
   FLAG_max_map_space_pages = 16;
   CcTest::InitializeVM();