From e7dc37c516feae74b17df86980e55097bb031dbb Mon Sep 17 00:00:00 2001 From: Ryan Macnak Date: Fri, 18 Sep 2020 21:32:56 +0000 Subject: [PATCH] [vm, gc] Don't start or finalize concurrent marking during a force growth scope. Adjust tests to exit force growth scopes before forcing a GC. Change-Id: I6d84c83e75ab8da4e0d623256d1997efd61341d1 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/163140 Commit-Queue: Ryan Macnak Reviewed-by: Alexander Aprelev Reviewed-by: Martin Kustermann --- runtime/vm/dart_api_impl_test.cc | 98 +++++++++++++++++--------------- runtime/vm/heap/heap.cc | 16 +++--- runtime/vm/heap/pages.cc | 3 + runtime/vm/heap/pages_test.cc | 1 + runtime/vm/thread_test.cc | 2 - 5 files changed, 63 insertions(+), 57 deletions(-) diff --git a/runtime/vm/dart_api_impl_test.cc b/runtime/vm/dart_api_impl_test.cc index a2226fb76858..8c96d22f428a 100644 --- a/runtime/vm/dart_api_impl_test.cc +++ b/runtime/vm/dart_api_impl_test.cc @@ -3133,10 +3133,6 @@ static void WeakPersistentHandleCallback(void* isolate_callback_data, } TEST_CASE(DartAPI_WeakPersistentHandle) { - // GCs due to allocations or weak handle creation can cause early promotion - // and interfer with the scenario this test is verifying. - NoHeapGrowthControlScope force_growth; - Dart_Handle local_new_ref = Dart_Null(); weak_new_ref = Dart_NewWeakPersistentHandle(local_new_ref, NULL, 0, WeakPersistentHandleCallback); @@ -3148,25 +3144,32 @@ TEST_CASE(DartAPI_WeakPersistentHandle) { { Dart_EnterScope(); - // Create an object in new space. - Dart_Handle new_ref = AllocateNewString("new string"); - EXPECT_VALID(new_ref); - - // Create an object in old space. - Dart_Handle old_ref = AllocateOldString("old string"); - EXPECT_VALID(old_ref); - - // Create a weak ref to the new space object. - weak_new_ref = Dart_NewWeakPersistentHandle(new_ref, NULL, 0, - WeakPersistentHandleCallback); - EXPECT_VALID(AsHandle(weak_new_ref)); - EXPECT(!Dart_IsNull(AsHandle(weak_new_ref))); - - // Create a weak ref to the old space object. - weak_old_ref = Dart_NewWeakPersistentHandle(old_ref, NULL, 0, - WeakPersistentHandleCallback); - EXPECT_VALID(AsHandle(weak_old_ref)); - EXPECT(!Dart_IsNull(AsHandle(weak_old_ref))); + Dart_Handle new_ref, old_ref; + { + // GCs due to allocations or weak handle creation can cause early + // promotion and interfere with the scenario this test is verifying. + NoHeapGrowthControlScope force_growth; + + // Create an object in new space. + new_ref = AllocateNewString("new string"); + EXPECT_VALID(new_ref); + + // Create an object in old space. + old_ref = AllocateOldString("old string"); + EXPECT_VALID(old_ref); + + // Create a weak ref to the new space object. + weak_new_ref = Dart_NewWeakPersistentHandle(new_ref, NULL, 0, + WeakPersistentHandleCallback); + EXPECT_VALID(AsHandle(weak_new_ref)); + EXPECT(!Dart_IsNull(AsHandle(weak_new_ref))); + + // Create a weak ref to the old space object. + weak_old_ref = Dart_NewWeakPersistentHandle(old_ref, NULL, 0, + WeakPersistentHandleCallback); + EXPECT_VALID(AsHandle(weak_old_ref)); + EXPECT(!Dart_IsNull(AsHandle(weak_old_ref))); + } { TransitionNativeToVM transition(thread); @@ -3264,10 +3267,6 @@ static void FinalizableHandleCallback(void* isolate_callback_data, void* peer) { } TEST_CASE(DartAPI_FinalizableHandle) { - // GCs due to allocations or weak handle creation can cause early promotion - // and interfer with the scenario this test is verifying. - NoHeapGrowthControlScope force_growth; - void* peer = reinterpret_cast(0); Dart_Handle local_new_ref = Dart_Null(); finalizable_new_ref = Dart_NewFinalizableHandle(local_new_ref, peer, 0, @@ -3283,25 +3282,32 @@ TEST_CASE(DartAPI_FinalizableHandle) { { Dart_EnterScope(); - // Create an object in new space. - Dart_Handle new_ref = AllocateNewString("new string"); - EXPECT_VALID(new_ref); - - // Create an object in old space. - Dart_Handle old_ref = AllocateOldString("old string"); - EXPECT_VALID(old_ref); - - // Create a weak ref to the new space object. - peer = reinterpret_cast(2); - finalizable_new_ref = - Dart_NewFinalizableHandle(new_ref, peer, 0, FinalizableHandleCallback); - finalizable_new_ref_peer = peer; - - // Create a weak ref to the old space object. - peer = reinterpret_cast(3); - finalizable_old_ref = - Dart_NewFinalizableHandle(old_ref, peer, 0, FinalizableHandleCallback); - finalizable_old_ref_peer = peer; + Dart_Handle new_ref, old_ref; + { + // GCs due to allocations or weak handle creation can cause early + // promotion and interfere with the scenario this test is verifying. + NoHeapGrowthControlScope force_growth; + + // Create an object in new space. + new_ref = AllocateNewString("new string"); + EXPECT_VALID(new_ref); + + // Create an object in old space. + old_ref = AllocateOldString("old string"); + EXPECT_VALID(old_ref); + + // Create a weak ref to the new space object. + peer = reinterpret_cast(2); + finalizable_new_ref = Dart_NewFinalizableHandle( + new_ref, peer, 0, FinalizableHandleCallback); + finalizable_new_ref_peer = peer; + + // Create a weak ref to the old space object. + peer = reinterpret_cast(3); + finalizable_old_ref = Dart_NewFinalizableHandle( + old_ref, peer, 0, FinalizableHandleCallback); + finalizable_old_ref_peer = peer; + } { TransitionNativeToVM transition(thread); diff --git a/runtime/vm/heap/heap.cc b/runtime/vm/heap/heap.cc index b2dd597e5201..2acdfd3a2af5 100644 --- a/runtime/vm/heap/heap.cc +++ b/runtime/vm/heap/heap.cc @@ -111,15 +111,13 @@ uword Heap::AllocateNew(intptr_t size) { uword Heap::AllocateOld(intptr_t size, OldPage::PageType type) { ASSERT(Thread::Current()->no_safepoint_scope_depth() == 0); - CollectForDebugging(); - uword addr = old_space_.TryAllocate(size, type); - if (addr != 0) { - return addr; - } - // If we are in the process of running a sweep, wait for the sweeper to free - // memory. - Thread* thread = Thread::Current(); if (old_space_.GrowthControlState()) { + CollectForDebugging(); + uword addr = old_space_.TryAllocate(size, type); + if (addr != 0) { + return addr; + } + Thread* thread = Thread::Current(); // Wait for any GC tasks that are in progress. WaitForSweeperTasks(thread); addr = old_space_.TryAllocate(size, type); @@ -148,7 +146,7 @@ uword Heap::AllocateOld(intptr_t size, OldPage::PageType type) { CollectAllGarbage(kLowMemory); WaitForSweeperTasks(thread); } - addr = old_space_.TryAllocate(size, type, PageSpace::kForceGrowth); + uword addr = old_space_.TryAllocate(size, type, PageSpace::kForceGrowth); if (addr != 0) { return addr; } diff --git a/runtime/vm/heap/pages.cc b/runtime/vm/heap/pages.cc index e0f10cb616b4..3c73952227f2 100644 --- a/runtime/vm/heap/pages.cc +++ b/runtime/vm/heap/pages.cc @@ -452,6 +452,7 @@ void PageSpace::FreePages(OldPage* pages) { void PageSpace::EvaluateConcurrentMarking(GrowthPolicy growth_policy) { if (growth_policy != kForceGrowth) { + ASSERT(GrowthControlState()); if (heap_ != NULL) { // Some unit tests. Thread* thread = Thread::Current(); if (thread->CanCollectGarbage()) { @@ -1020,6 +1021,8 @@ bool PageSpace::ShouldPerformIdleMarkCompact(int64_t deadline) { } void PageSpace::CollectGarbage(bool compact, bool finalize) { + ASSERT(GrowthControlState()); + if (!finalize) { #if defined(TARGET_ARCH_IA32) return; // Barrier not implemented. diff --git a/runtime/vm/heap/pages_test.cc b/runtime/vm/heap/pages_test.cc index 1db61132b8e0..fed0a88303f0 100644 --- a/runtime/vm/heap/pages_test.cc +++ b/runtime/vm/heap/pages_test.cc @@ -10,6 +10,7 @@ namespace dart { TEST_CASE(Pages) { PageSpace* space = new PageSpace(NULL, 4 * MBInWords); + space->InitGrowthControl(); EXPECT(!space->Contains(reinterpret_cast(&space))); uword block = space->TryAllocate(8 * kWordSize); EXPECT(block != 0); diff --git a/runtime/vm/thread_test.cc b/runtime/vm/thread_test.cc index a5618c6ef2de..41e3374d765a 100644 --- a/runtime/vm/thread_test.cc +++ b/runtime/vm/thread_test.cc @@ -176,8 +176,6 @@ ISOLATE_UNIT_TEST_CASE(ManyTasksWithZones) { Monitor sync[kTaskCount]; bool done[kTaskCount]; Isolate* isolate = thread->isolate(); - EXPECT(isolate->heap()->GrowthControlState()); - isolate->heap()->DisableGrowthControl(); for (int i = 0; i < kTaskCount; i++) { done[i] = false; Dart::thread_pool()->Run(isolate, &sync[i],