commit 71e8b418bfe328e1346861fa33387aefccc63119
author Ryan Macnak <rmacnak@google.com> Mon Jul 02 20:37:35 2018 +0000
committer commit-bot@chromium.org <commit-bot@chromium.org> Mon Jul 02 20:37:35 2018 +0000
tree 99932cb5a54344304570db4ec9d88e7de4f73986
parent e16f5781fe796309cb11e626673171f9c29e82b2

[vm] Account for external promotions; include external allocations in idle GC trigger.

A new-space collection during AllocateExternal may promote enough external allocations to warrant an old-space collection.

Bug: https://github.com/dart-lang/sdk/issues/33314
Change-Id: Ie7855e506869535435dc76b61ad30e5f42eda4a2
Reviewed-on: https://dart-review.googlesource.com/63449
Reviewed-by: Zach Anderson <zra@google.com>
Commit-Queue: Ryan Macnak <rmacnak@google.com>

runtime/vm/heap/heap.cc

diff --git a/runtime/vm/heap/heap.cc b/runtime/vm/heap/heap.cc
index 376f9b7..9a281b0 100644
--- a/runtime/vm/heap/heap.cc
+++ b/runtime/vm/heap/heap.cc
@@ -135,16 +135,17 @@
       // Attempt to free some external allocation by a scavenge. (If the total
       // remains above the limit, next external alloc will trigger another.)
       CollectGarbage(kScavenge, kExternal);
+      // Promotion may have pushed old space over its limit.
+      if (old_space_.NeedsGarbageCollection()) {
+        CollectGarbage(kMarkSweep, kExternal);
+      }
     }
   } else {
     ASSERT(space == kOld);
     old_space_.AllocateExternal(cid, size);
-  }
-  // Idle GC does not check whether promotions should trigger a full GC.
-  // As a workaround, we check here on every external allocation. See issue
-  // dartbug.com/33314.
-  if (old_space_.NeedsGarbageCollection()) {
-    CollectAllGarbage(kExternal);
+    if (old_space_.NeedsGarbageCollection()) {
+      CollectAllGarbage(kExternal);
+    }
   }
 }
 

runtime/vm/heap/heap_test.cc

diff --git a/runtime/vm/heap/heap_test.cc b/runtime/vm/heap/heap_test.cc
index 1281e13..e2f2379 100644
--- a/runtime/vm/heap/heap_test.cc
+++ b/runtime/vm/heap/heap_test.cc
@@ -576,4 +576,39 @@
   EXPECT(size_before < size_after);
 }
 
+static void NoopFinalizer(void* isolate_callback_data,
+                          Dart_WeakPersistentHandle handle,
+                          void* peer) {}
+
+ISOLATE_UNIT_TEST_CASE(ExternalPromotion) {
+  Isolate* isolate = Isolate::Current();
+  Heap* heap = isolate->heap();
+
+  heap->CollectAllGarbage();
+  intptr_t size_before = kWordSize * (heap->new_space()->ExternalInWords() +
+                                      heap->old_space()->ExternalInWords());
+
+  Array& old = Array::Handle(Array::New(100, Heap::kOld));
+  Array& neu = Array::Handle();
+  for (intptr_t i = 0; i < 100; i++) {
+    neu = Array::New(1, Heap::kNew);
+    FinalizablePersistentHandle::New(isolate, neu, NULL, NoopFinalizer, 1 * MB);
+    old.SetAt(i, neu);
+  }
+
+  intptr_t size_middle = kWordSize * (heap->new_space()->ExternalInWords() +
+                                      heap->old_space()->ExternalInWords());
+  EXPECT_EQ(size_before + 100 * MB, size_middle);
+
+  old = Array::null();
+  neu = Array::null();
+
+  heap->CollectAllGarbage();
+
+  intptr_t size_after = kWordSize * (heap->new_space()->ExternalInWords() +
+                                     heap->old_space()->ExternalInWords());
+
+  EXPECT_EQ(size_before, size_after);
+}
+
 }  // namespace dart

runtime/vm/heap/pages.cc

diff --git a/runtime/vm/heap/pages.cc b/runtime/vm/heap/pages.cc
index 344932e..fe335e4 100644
--- a/runtime/vm/heap/pages.cc
+++ b/runtime/vm/heap/pages.cc
@@ -1268,7 +1268,8 @@
   if (heap_growth_ratio_ == 100) {
     return false;
   }
-  return current.capacity_in_words > idle_gc_threshold_in_words_;
+  return (current.capacity_in_words > idle_gc_threshold_in_words_) ||
+         NeedsExternalCollection(current);
 }
 
 void PageSpaceController::EvaluateGarbageCollection(SpaceUsage before,