From cc13b6c0bc30a6b14ce3e04c316c342412b04714 Mon Sep 17 00:00:00 2001 From: Manos Koukoutos Date: Wed, 8 Jun 2022 15:25:09 +0000 Subject: [PATCH] Revert "[heap] Avoid dynamic updates of FLAG_gc_interval" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit abcb6bb8b4ea890b20fe73096ad28c7d608da796. Reason for revert: https://ci.chromium.org/ui/p/v8/builders/ci/V8%20Linux64%20TSAN%20-%20isolates/20029/overview Original change's description: > [heap] Avoid dynamic updates of FLAG_gc_interval > > Flags will be protected from updates after V8 initialization (in the > future). This CL avoids any updates of the --gc-interval flag during > runtime, and instead updates a static field on the HeapAllocator > directly. > > R=​mlippautz@chromium.org > > Bug: v8:12887 > Change-Id: I17a495cae50a46d59a8159c6ece1558d4d61b949 > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3687691 > Commit-Queue: Clemens Backes > Reviewed-by: Michael Lippautz > Cr-Commit-Position: refs/heads/main@{#80998} Bug: v8:12887 Change-Id: I18310a3f515506d617f42be7a208013957625eaf No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3695559 Reviewed-by: Manos Koukoutos Owners-Override: Manos Koukoutos Reviewed-by: Clemens Backes Bot-Commit: Rubber Stamper Commit-Queue: Manos Koukoutos Cr-Commit-Position: refs/heads/main@{#81002} --- src/flags/flag-definitions.h | 6 ------ src/heap/heap-allocator.cc | 16 ++-------------- src/heap/heap-allocator.h | 10 +--------- src/heap/heap.cc | 3 --- src/runtime/runtime-test.cc | 4 ++-- test/cctest/heap/test-heap.cc | 7 +++++-- test/cctest/test-api.cc | 4 +--- 7 files changed, 11 insertions(+), 39 deletions(-) diff --git a/src/flags/flag-definitions.h b/src/flags/flag-definitions.h index dd78f98ee5..45cceb01d2 100644 --- a/src/flags/flag-definitions.h +++ b/src/flags/flag-definitions.h @@ -1199,16 +1199,10 @@ DEFINE_BOOL(separate_gc_phases, false, DEFINE_BOOL(global_gc_scheduling, true, "enable GC scheduling based on global memory") DEFINE_BOOL(gc_global, false, "always perform global GCs") - -// TODO(12950): The next two flags only have an effect if -// V8_ENABLE_ALLOCATION_TIMEOUT is set, so we should only define them in that -// config. That currently breaks Node's parallel/test-domain-error-types test -// though. DEFINE_INT(random_gc_interval, 0, "Collect garbage after random(0, X) allocations. It overrides " "gc_interval.") DEFINE_INT(gc_interval, -1, "garbage collect after allocations") - DEFINE_INT(retain_maps_for_n_gc, 2, "keeps maps alive for old space garbage collections") DEFINE_BOOL(trace_gc, false, diff --git a/src/heap/heap-allocator.cc b/src/heap/heap-allocator.cc index cc4ef943fa..bc1a20dd4b 100644 --- a/src/heap/heap-allocator.cc +++ b/src/heap/heap-allocator.cc @@ -141,18 +141,6 @@ void HeapAllocator::IncrementObjectCounters() { #endif // DEBUG #ifdef V8_ENABLE_ALLOCATION_TIMEOUT -// static -void HeapAllocator::InitializeOncePerProcess() { - SetAllocationGcInterval(FLAG_gc_interval); -} - -// static -void HeapAllocator::SetAllocationGcInterval(int allocation_gc_interval) { - allocation_gc_interval_ = allocation_gc_interval; -} - -// static -int HeapAllocator::allocation_gc_interval_ = -1; void HeapAllocator::SetAllocationTimeout(int allocation_timeout) { allocation_timeout_ = allocation_timeout; @@ -170,8 +158,8 @@ void HeapAllocator::UpdateAllocationTimeout() { // allow the subsequent allocation attempts to go through. constexpr int kFewAllocationsHeadroom = 6; allocation_timeout_ = std::max(kFewAllocationsHeadroom, new_timeout); - } else if (allocation_gc_interval_ >= 0) { - allocation_timeout_ = allocation_gc_interval_; + } else if (FLAG_gc_interval >= 0) { + allocation_timeout_ = FLAG_gc_interval; } } diff --git a/src/heap/heap-allocator.h b/src/heap/heap-allocator.h index d24e8e6573..adb118a249 100644 --- a/src/heap/heap-allocator.h +++ b/src/heap/heap-allocator.h @@ -70,9 +70,6 @@ class V8_EXPORT_PRIVATE HeapAllocator final { #ifdef V8_ENABLE_ALLOCATION_TIMEOUT void UpdateAllocationTimeout(); void SetAllocationTimeout(int allocation_timeout); - - static void SetAllocationGcInterval(int allocation_gc_interval); - static void InitializeOncePerProcess(); #endif // V8_ENABLE_ALLOCATION_TIMEOUT private: @@ -110,15 +107,10 @@ class V8_EXPORT_PRIVATE HeapAllocator final { ConcurrentAllocator* shared_map_allocator_; #ifdef V8_ENABLE_ALLOCATION_TIMEOUT - // If the {allocation_gc_interval_} is set to a positive value, this variable + // If the --gc-interval flag is set to a positive value, this variable // holds the value indicating the number of allocations remain until the // next failure and garbage collection. int allocation_timeout_ = 0; - - // The configured GC interval, initialized from --gc-interval during - // {InitializeOncePerProcess} and potentially dynamically updated by - // %SetAllocationTimeout. - static int allocation_gc_interval_; #endif // V8_ENABLE_ALLOCATION_TIMEOUT }; diff --git a/src/heap/heap.cc b/src/heap/heap.cc index d062939e23..f295e7f48d 100644 --- a/src/heap/heap.cc +++ b/src/heap/heap.cc @@ -5927,9 +5927,6 @@ void Heap::InitializeHashSeed() { // static void Heap::InitializeOncePerProcess() { -#ifdef V8_ENABLE_ALLOCATION_TIMEOUT - HeapAllocator::InitializeOncePerProcess(); -#endif MemoryAllocator::InitializeOncePerProcess(); } diff --git a/src/runtime/runtime-test.cc b/src/runtime/runtime-test.cc index 3ca6e95784..43eb0ba3f9 100644 --- a/src/runtime/runtime-test.cc +++ b/src/runtime/runtime-test.cc @@ -884,12 +884,12 @@ RUNTIME_FUNCTION(Runtime_SetAllocationTimeout) { SealHandleScope shs(isolate); DCHECK(args.length() == 2 || args.length() == 3); #ifdef V8_ENABLE_ALLOCATION_TIMEOUT - CONVERT_INT32_ARG_FUZZ_SAFE(interval, 0); - HeapAllocator::SetAllocationGcInterval(interval); CONVERT_INT32_ARG_FUZZ_SAFE(timeout, 1); isolate->heap()->set_allocation_timeout(timeout); #endif #ifdef DEBUG + CONVERT_INT32_ARG_FUZZ_SAFE(interval, 0); + FLAG_gc_interval = interval; if (args.length() == 3) { // Enable/disable inline allocation if requested. CONVERT_BOOLEAN_ARG_FUZZ_SAFE(inline_allocation, 2); diff --git a/test/cctest/heap/test-heap.cc b/test/cctest/heap/test-heap.cc index 62a00714de..5c037cf6a9 100644 --- a/test/cctest/heap/test-heap.cc +++ b/test/cctest/heap/test-heap.cc @@ -3023,7 +3023,7 @@ static void AddPropertyTo(int gc_count, Handle object, Factory* factory = isolate->factory(); Handle prop_name = factory->InternalizeUtf8String(property_name); Handle twenty_three(Smi::FromInt(23), isolate); - HeapAllocator::SetAllocationGcInterval(gc_count); + FLAG_gc_interval = gc_count; FLAG_gc_global = true; FLAG_retain_maps_for_n_gc = 0; CcTest::heap()->set_allocation_timeout(gc_count); @@ -4825,7 +4825,7 @@ TEST(AddInstructionChangesNewSpacePromotion) { FLAG_allow_natives_syntax = true; FLAG_expose_gc = true; FLAG_stress_compaction = true; - HeapAllocator::SetAllocationGcInterval(1000); + FLAG_gc_interval = 1000; CcTest::InitializeVM(); if (!FLAG_allocation_site_pretenuring) return; v8::HandleScope scope(CcTest::isolate()); @@ -5210,6 +5210,8 @@ TEST(RetainedMapsCleanup) { } TEST(PreprocessStackTrace) { + // Do not automatically trigger early GC. + FLAG_gc_interval = -1; CcTest::InitializeVM(); v8::HandleScope scope(CcTest::isolate()); v8::TryCatch try_catch(CcTest::isolate()); @@ -5239,6 +5241,7 @@ TEST(PreprocessStackTrace) { } } + void AllocateInSpace(Isolate* isolate, size_t bytes, AllocationSpace space) { CHECK_LE(FixedArray::kHeaderSize, bytes); CHECK(IsAligned(bytes, kTaggedSize)); diff --git a/test/cctest/test-api.cc b/test/cctest/test-api.cc index ec0a95da3b..716683f1a1 100644 --- a/test/cctest/test-api.cc +++ b/test/cctest/test-api.cc @@ -24152,11 +24152,9 @@ TEST(CreateSyntheticModule) { } TEST(CreateSyntheticModuleGC) { -#ifdef V8_ENABLE_ALLOCATION_TIMEOUT // Try to make sure that CreateSyntheticModule() deals well with a GC // happening during its execution. - i::HeapAllocator::SetAllocationGcInterval(10); -#endif + i::FLAG_gc_interval = 10; i::FLAG_inline_new = false; LocalContext env;