From 89f5bf7fde7d62bd0dd95dab5a2429189fc5d2ad Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marja=20H=C3=B6ltt=C3=A4?= <marja@chromium.org>
Date: Mon, 9 Apr 2018 16:58:53 +0200
Subject: [PATCH] [heap] Remove unnecessary length reloading from
 ArrayList::Add.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The reloading was needed when GC would compact the Heap::retained_maps
array. But that's no longer true; the compaction is done in
Heap::AddRetainedMap, outside GC. So it's not possible that the length would
change because of an allocation.

(Pre-cleanup for in-place weak ref work.)

BUG=v8:7308

Change-Id: I18554353014865992f9151002cc4097fb986faf1
Reviewed-on: https://chromium-review.googlesource.com/1002775
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Commit-Queue: Marja Hölttä <marja@chromium.org>
Cr-Commit-Position: refs/heads/master@{#52506}
---
 src/heap/heap.cc              |  3 +--
 src/objects.cc                | 16 ++++++----------
 src/objects/fixed-array.h     | 10 ++--------
 test/cctest/heap/test-heap.cc | 22 ----------------------
 4 files changed, 9 insertions(+), 42 deletions(-)

diff --git a/src/heap/heap.cc b/src/heap/heap.cc
index 1fbfb9bac7..4437ec19a7 100644
--- a/src/heap/heap.cc
+++ b/src/heap/heap.cc
@@ -5018,8 +5018,7 @@ void Heap::AddRetainedMap(Handle<Map> map) {
     CompactRetainedMaps(*array);
   }
   array = ArrayList::Add(
-      array, cell, handle(Smi::FromInt(FLAG_retain_maps_for_n_gc), isolate()),
-      ArrayList::kReloadLengthAfterAllocation);
+      array, cell, handle(Smi::FromInt(FLAG_retain_maps_for_n_gc), isolate()));
   if (*array != retained_maps()) {
     set_retained_maps(*array);
   }
diff --git a/src/objects.cc b/src/objects.cc
index 0a1e69fcce..5c933777cd 100644
--- a/src/objects.cc
+++ b/src/objects.cc
@@ -10275,14 +10275,11 @@ Handle<FixedArrayOfWeakCells> FixedArrayOfWeakCells::Allocate(
 }
 
 // static
-Handle<ArrayList> ArrayList::Add(Handle<ArrayList> array, Handle<Object> obj,
-                                 AddMode mode) {
+Handle<ArrayList> ArrayList::Add(Handle<ArrayList> array, Handle<Object> obj) {
   int length = array->Length();
   array = EnsureSpace(array, length + 1);
-  if (mode == kReloadLengthAfterAllocation) {
-    DCHECK(array->Length() <= length);
-    length = array->Length();
-  }
+  // Check that GC didn't remove elements from the array.
+  DCHECK_EQ(array->Length(), length);
   array->Set(length, *obj);
   array->SetLength(length + 1);
   return array;
@@ -10290,12 +10287,11 @@ Handle<ArrayList> ArrayList::Add(Handle<ArrayList> array, Handle<Object> obj,
 
 // static
 Handle<ArrayList> ArrayList::Add(Handle<ArrayList> array, Handle<Object> obj1,
-                                 Handle<Object> obj2, AddMode mode) {
+                                 Handle<Object> obj2) {
   int length = array->Length();
   array = EnsureSpace(array, length + 2);
-  if (mode == kReloadLengthAfterAllocation) {
-    length = array->Length();
-  }
+  // Check that GC didn't remove elements from the array.
+  DCHECK_EQ(array->Length(), length);
   array->Set(length, *obj1);
   array->Set(length + 1, *obj2);
   array->SetLength(length + 2);
diff --git a/src/objects/fixed-array.h b/src/objects/fixed-array.h
index 47fbee37c4..022821244f 100644
--- a/src/objects/fixed-array.h
+++ b/src/objects/fixed-array.h
@@ -372,15 +372,9 @@ class FixedArrayOfWeakCells : public FixedArray {
 // underlying FixedArray starting at kFirstIndex.
 class ArrayList : public FixedArray {
  public:
-  enum AddMode {
-    kNone,
-    // Use this if GC can delete elements from the array.
-    kReloadLengthAfterAllocation,
-  };
-  static Handle<ArrayList> Add(Handle<ArrayList> array, Handle<Object> obj,
-                               AddMode mode = kNone);
+  static Handle<ArrayList> Add(Handle<ArrayList> array, Handle<Object> obj);
   static Handle<ArrayList> Add(Handle<ArrayList> array, Handle<Object> obj1,
-                               Handle<Object> obj2, AddMode = kNone);
+                               Handle<Object> obj2);
   static Handle<ArrayList> New(Isolate* isolate, int size);
 
   // Returns the number of elements in the list, not the allocated size, which
diff --git a/test/cctest/heap/test-heap.cc b/test/cctest/heap/test-heap.cc
index d86c683e34..ba7438ac48 100644
--- a/test/cctest/heap/test-heap.cc
+++ b/test/cctest/heap/test-heap.cc
@@ -4658,28 +4658,6 @@ TEST(MapRetaining) {
   CheckMapRetainingFor(7);
 }
 
-
-TEST(RegressArrayListGC) {
-  FLAG_retain_maps_for_n_gc = 1;
-  FLAG_incremental_marking = 0;
-  FLAG_gc_global = true;
-  CcTest::InitializeVM();
-  v8::HandleScope scope(CcTest::isolate());
-  Isolate* isolate = CcTest::i_isolate();
-  Heap* heap = isolate->heap();
-  AddRetainedMap(isolate, heap);
-  Handle<Map> map = Map::Create(isolate, 1);
-  CcTest::CollectGarbage(OLD_SPACE);
-  // Force GC in old space on next addition of retained map.
-  Map::WeakCellForMap(map);
-  heap::SimulateFullSpace(CcTest::heap()->new_space());
-  for (int i = 0; i < 10; i++) {
-    heap->AddRetainedMap(map);
-  }
-  CcTest::CollectGarbage(OLD_SPACE);
-}
-
-
 TEST(WritableVsImmortalRoots) {
   for (int i = 0; i < Heap::kStrongRootListLength; ++i) {
     Heap::RootListIndex root_index = static_cast<Heap::RootListIndex>(i);