[heap] Fix --gc-interval command line flag

This CL fixes the --gc-interval command line flag which was
supposed to trigger GCs more frequently. Since this was not really
happening, we might now trigger GCs in more locations and thus
might flush out some pre-existing issues.

In the linked bug --gc-interval was not triggering GCs because
AllocateRawWith() was invoking AllocateRaw() twice without performing
a GC in-between. Assuming allocation_timeout_ was 1 before the first
call to AllocateRaw(), the first call to AllocateRaw() decreased the
counter to 0. AllocateRawWith() then invokes AllocateRaw() again at
the beginning of AllocateRawWithLightRetrySlowPath(). In the second
call allocation_timeout_ is then 0, AllocateRaw() therefore assumes
the allocation timeout is disabled and the second allocation
subsequently succeeds.

This CL fixes this by turning allocation_timeout_ into
base::Optional<int>. With this change both invocations of
AllocateRaw() observe the allocation timeout as enabled and both
invocations let the allocation fail because of this.

Bug: 346197733
Change-Id: Ic5987a9fb83320a145b04271e5631e50410c3572
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5613990
Commit-Queue: Dominik Inführ <dinfuehr@chromium.org>
Reviewed-by: Darius Mercadier <dmercadier@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/main@{#94357}

src/heap/heap-allocator-inl.h

@@ -84,7 +84,8 @@ V8_WARN_UNUSED_RESULT V8_INLINE AllocationResult HeapAllocator::AllocateRaw(
   }
 
 #ifdef V8_ENABLE_ALLOCATION_TIMEOUT
-  if (V8_UNLIKELY(allocation_timeout_ > 0) && ReachedAllocationTimeout()) {
+  if (V8_UNLIKELY(allocation_timeout_.has_value()) &&
+      ReachedAllocationTimeout()) {
     return AllocationResult::Failure();
   }
 #endif  // V8_ENABLE_ALLOCATION_TIMEOUT

src/heap/heap-allocator.cc

@@ -358,39 +358,41 @@ void HeapAllocator::SetAllocationGcInterval(int allocation_gc_interval) {
 std::atomic<int> HeapAllocator::allocation_gc_interval_{-1};
 
 void HeapAllocator::SetAllocationTimeout(int allocation_timeout) {
-  // See `allocation_timeout_` for description. We map negative values to 0 to
-  // avoid underflows as allocation decrements this value as well.
-  allocation_timeout_ = std::max(0, allocation_timeout);
+  if (allocation_timeout > 0) {
+    allocation_timeout_ = allocation_timeout;
+  } else {
+    allocation_timeout_.reset();
+  }
 }
 
 void HeapAllocator::UpdateAllocationTimeout() {
   if (v8_flags.random_gc_interval > 0) {
-    const int new_timeout = allocation_timeout_ <= 0
-                                ? heap_->isolate()->fuzzer_rng()->NextInt(
-                                      v8_flags.random_gc_interval + 1)
-                                : allocation_timeout_;
+    const int new_timeout = heap_->isolate()->fuzzer_rng()->NextInt(
+        v8_flags.random_gc_interval + 1);
     // Reset the allocation timeout, but make sure to allow at least a few
     // allocations after a collection. The reason for this is that we have a lot
     // of allocation sequences and we assume that a garbage collection will
     // allow the subsequent allocation attempts to go through.
     constexpr int kFewAllocationsHeadroom = 6;
-    allocation_timeout_ = std::max(kFewAllocationsHeadroom, new_timeout);
+    int timeout = std::max(kFewAllocationsHeadroom, new_timeout);
+    SetAllocationTimeout(timeout);
+    DCHECK(allocation_timeout_.has_value());
     return;
   }
 
-  int interval = allocation_gc_interval_.load(std::memory_order_relaxed);
-  allocation_timeout_ = std::max(0, interval);
+  int timeout = allocation_gc_interval_.load(std::memory_order_relaxed);
+  SetAllocationTimeout(timeout);
 }
 
 bool HeapAllocator::ReachedAllocationTimeout() {
-  DCHECK_GT(allocation_timeout_, 0);
+  DCHECK(allocation_timeout_.has_value());
 
   if (heap_->always_allocate() || local_heap_->IsRetryOfFailedAllocation()) {
     return false;
   }
 
-  --allocation_timeout_;
-  return allocation_timeout_ <= 0;
+  allocation_timeout_ = std::max(0, allocation_timeout_.value() - 1);
+  return allocation_timeout_.value() <= 0;
 }
 
 #endif  // V8_ENABLE_ALLOCATION_TIMEOUT

src/heap/heap-allocator.h

@@ -83,7 +83,9 @@ class V8_EXPORT_PRIVATE HeapAllocator final {
   static void SetAllocationGcInterval(int allocation_gc_interval);
   static void InitializeOncePerProcess();
 
-  int get_allocation_timeout_for_testing() const { return allocation_timeout_; }
+  base::Optional<int> get_allocation_timeout_for_testing() const {
+    return allocation_timeout_;
+  }
 #endif  // V8_ENABLE_ALLOCATION_TIMEOUT
 
   // Give up all LABs. Used for e.g. full GCs.
@@ -187,7 +189,7 @@ class V8_EXPORT_PRIVATE HeapAllocator final {
   // allocation failure (which will eventually lead to garbage collection).
   // Allocation will fail for any values <=0. See `UpdateAllocationTimeout()`
   // for how the new timeout is computed.
-  int allocation_timeout_ = 0;
+  base::Optional<int> allocation_timeout_;
 
   // The configured GC interval, initialized from --gc-interval during
   // `InitializeOncePerProcess` and potentially dynamically updated by

src/heap/heap.cc

@@ -7507,10 +7507,6 @@ void StrongRootAllocatorBase::deallocate_impl(Address* p, size_t n) noexcept {
 void Heap::set_allocation_timeout(int allocation_timeout) {
   heap_allocator_->SetAllocationTimeout(allocation_timeout);
 }
-
-int Heap::get_allocation_timeout_for_testing() const {
-  return heap_allocator_->get_allocation_timeout_for_testing();
-}
 #endif  // V8_ENABLE_ALLOCATION_TIMEOUT
 
 void Heap::FinishSweepingIfOutOfWork() {

src/heap/heap.h

@@ -1572,7 +1572,6 @@ class Heap final {
 
 #ifdef V8_ENABLE_ALLOCATION_TIMEOUT
   void V8_EXPORT_PRIVATE set_allocation_timeout(int allocation_timeout);
-  int V8_EXPORT_PRIVATE get_allocation_timeout_for_testing() const;
 #endif  // V8_ENABLE_ALLOCATION_TIMEOUT
 
 #ifdef DEBUG

test/mjsunit/mjsunit.status

@@ -1697,6 +1697,8 @@
   'wasm/shared-memory-worker-stress': [PASS, SLOW, ['tsan', SKIP]],
   'regress/regress-740784': [PASS, ['tsan', SKIP]],
   'regress/regress-crbug-820312': [PASS, SLOW],
+  # Bug(chromium:42204525): Re-enable once this is supported.
+  'turboshaft/turboshaft-maglev-frontend': [SKIP],
 }],  # variant == stress_incremental_marking
 
 ##############################################################################

test/unittests/heap/heap-unittest.cc

@@ -521,7 +521,7 @@ TEST_F(HeapTestWithRandomGCInterval, AllocationTimeout) {
   // Invoke major GC to cause the timeout to be updated.
   InvokeMajorGC();
   const int initial_allocation_timeout =
-      heap()->get_allocation_timeout_for_testing();
+      allocator->get_allocation_timeout_for_testing().value_or(0);
   ASSERT_GT(initial_allocation_timeout, 0);
 
   for (int i = 0; i < initial_allocation_timeout - 1; ++i) {