[vm, gc] When there are outstanding typed data pointers, delay external GC.

TEST=ci
Bug: https://github.com/dart-lang/sdk/issues/48488
Change-Id: I73ccd4a974b921885450271202f432cdc1439aea
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/234822
Reviewed-by: Siva Annamalai <asiva@google.com>
Commit-Queue: Ryan Macnak <rmacnak@google.com>

runtime/vm/base_isolate.h

@@ -20,12 +20,6 @@ class Zone;
 // constructor/destructor for performance.
 class BaseIsolate {
  public:
-#if defined(DEBUG)
-  void AssertCurrentThreadIsMutator() const;
-#else
-  void AssertCurrentThreadIsMutator() const {}
-#endif  // DEBUG
-
 #if defined(DEBUG)
   static void AssertCurrent(BaseIsolate* isolate);
 #endif

runtime/vm/dart_api_impl.h

@@ -334,7 +334,13 @@ class Api : AllStatic {
 #define START_NO_CALLBACK_SCOPE(thread) thread->IncrementNoCallbackScopeDepth()
 
 // End a no Dart API call backs Scope.
-#define END_NO_CALLBACK_SCOPE(thread) thread->DecrementNoCallbackScopeDepth()
+#define END_NO_CALLBACK_SCOPE(thread)                                          \
+  do {                                                                         \
+    thread->DecrementNoCallbackScopeDepth();                                   \
+    if (thread->no_callback_scope_depth() == 0) {                              \
+      thread->heap()->CheckExternalGC(thread);                                 \
+    }                                                                          \
+  } while (false)
 
 #define CHECK_CALLBACK_STATE(thread)                                           \
   if (thread->no_callback_scope_depth() != 0) {                                \

runtime/vm/dart_api_impl_test.cc

@@ -2808,6 +2808,34 @@ TEST_CASE(DartAPI_ByteDataDirectAccessVerified) {
   TestByteDataDirectAccess();
 }
 
+static void NopCallback(void* isolate_callback_data, void* peer) {}
+
+TEST_CASE(DartAPI_ExternalAllocationDuringNoCallbackScope) {
+  Dart_Handle bytes = Dart_NewTypedData(Dart_TypedData_kUint8, 100);
+  EXPECT_VALID(bytes);
+
+  intptr_t gc_count_before = Thread::Current()->heap()->Collections(Heap::kNew);
+
+  Dart_TypedData_Type type;
+  void* data;
+  intptr_t len;
+  Dart_Handle result = Dart_TypedDataAcquireData(bytes, &type, &data, &len);
+  EXPECT_VALID(result);
+
+  Dart_WeakPersistentHandle weak =
+      Dart_NewWeakPersistentHandle(bytes, NULL, 100 * MB, NopCallback);
+  EXPECT_VALID(reinterpret_cast<Dart_Handle>(weak));
+
+  EXPECT_EQ(gc_count_before,
+            Thread::Current()->heap()->Collections(Heap::kNew));
+
+  result = Dart_TypedDataReleaseData(bytes);
+  EXPECT_VALID(result);
+
+  EXPECT_LT(gc_count_before,
+            Thread::Current()->heap()->Collections(Heap::kNew));
+}
+
 static void ExternalTypedDataAccessTests(Dart_Handle obj,
                                          Dart_TypedData_Type expected_type,
                                          uint8_t data[],
@@ -2917,8 +2945,6 @@ TEST_CASE(DartAPI_ExternalUint8ClampedArrayAccess) {
   EXPECT(value);
 }
 
-static void NopCallback(void* isolate_callback_data, void* peer) {}
-
 static void UnreachedCallback(void* isolate_callback_data, void* peer) {
   UNREACHABLE();
 }

runtime/vm/heap/heap.cc

@@ -149,30 +149,18 @@ uword Heap::AllocateOld(intptr_t size, OldPage::PageType type) {
 }
 
 void Heap::AllocatedExternal(intptr_t size, Space space) {
-  ASSERT(Thread::Current()->no_safepoint_scope_depth() == 0);
   if (space == kNew) {
-    Isolate::Current()->AssertCurrentThreadIsMutator();
     new_space_.AllocatedExternal(size);
-    if (new_space_.ExternalInWords() <= (4 * new_space_.CapacityInWords())) {
-      return;
-    }
-    // Attempt to free some external allocation by a scavenge. (If the total
-    // remains above the limit, next external alloc will trigger another.)
-    CollectGarbage(GCType::kScavenge, GCReason::kExternal);
-    // Promotion may have pushed old space over its limit. Fall through for old
-    // space GC check.
   } else {
     ASSERT(space == kOld);
     old_space_.AllocatedExternal(size);
   }
 
-  if (old_space_.ReachedHardThreshold()) {
-    if (last_gc_was_old_space_) {
-      CollectNewSpaceGarbage(Thread::Current(), GCReason::kFull);
-    }
-    CollectGarbage(GCType::kMarkSweep, GCReason::kExternal);
+  Thread* thread = Thread::Current();
+  if (thread->no_callback_scope_depth() == 0) {
+    CheckExternalGC(thread);
   } else {
-    CheckStartConcurrentMarking(Thread::Current(), GCReason::kExternal);
+    // Check delayed until Dart_TypedDataRelease.
   }
 }
 
@@ -190,6 +178,27 @@ void Heap::PromotedExternal(intptr_t size) {
   old_space_.AllocatedExternal(size);
 }
 
+void Heap::CheckExternalGC(Thread* thread) {
+  ASSERT(thread->no_safepoint_scope_depth() == 0);
+  ASSERT(thread->no_callback_scope_depth() == 0);
+  if (new_space_.ExternalInWords() >= (4 * new_space_.CapacityInWords())) {
+    // Attempt to free some external allocation by a scavenge. (If the total
+    // remains above the limit, next external alloc will trigger another.)
+    CollectGarbage(GCType::kScavenge, GCReason::kExternal);
+    // Promotion may have pushed old space over its limit. Fall through for old
+    // space GC check.
+  }
+
+  if (old_space_.ReachedHardThreshold()) {
+    if (last_gc_was_old_space_) {
+      CollectNewSpaceGarbage(thread, GCReason::kFull);
+    }
+    CollectGarbage(GCType::kMarkSweep, GCReason::kExternal);
+  } else {
+    CheckStartConcurrentMarking(thread, GCReason::kExternal);
+  }
+}
+
 bool Heap::Contains(uword addr) const {
   return new_space_.Contains(addr) || old_space_.Contains(addr);
 }

runtime/vm/heap/heap.h

@@ -83,6 +83,7 @@ class Heap {
   void FreedExternal(intptr_t size, Space space);
   // Move external size from new to old space. Does not by itself trigger GC.
   void PromotedExternal(intptr_t size);
+  void CheckExternalGC(Thread* thread);
 
   // Heap contains the specified address.
   bool Contains(uword addr) const;

runtime/vm/isolate.cc

@@ -1656,11 +1656,6 @@ void Isolate::FlagsCopyFrom(const Dart_IsolateFlags& api_flags) {
 void BaseIsolate::AssertCurrent(BaseIsolate* isolate) {
   ASSERT(isolate == Isolate::Current());
 }
-
-void BaseIsolate::AssertCurrentThreadIsMutator() const {
-  ASSERT(Isolate::Current() == this);
-  ASSERT(Thread::Current()->IsMutatorThread());
-}
 #endif  // defined(DEBUG)
 
 #if defined(DEBUG)