[objects] Compact and shrink script_list

Dominik Inführ · Commit Bot · commit fb26d0bb1835 · 2020-01-08T17:48:32.000Z
So far creating scripts always grew the script_list without ever reusing cleared slots or shrinking. While this is probably not a problem with script_list in practice, this is still a memory leak. Fix this leak by using WeakArrayList::Append instead of AddToEnd. Append adds to the end of the array, but potentially compacts and shrinks the list as well. Other WeakArrayLists can use this method as well, as long as they are not using indices into this array. Bug: v8:10031 Change-Id: If743c4cc3f8d67ab735522f0ded038b2fb43e437 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1967385 Commit-Queue: Dominik Inführ <dinfuehr@chromium.org> Reviewed-by: Ulan Degenbaev <ulan@chromium.org> Cr-Commit-Position: refs/heads/master@{#65640}
diff --git a/src/heap/factory.cc b/src/heap/factory.cc
@@ -1644,8 +1644,8 @@ Handle<Script> Factory::NewScriptWithId(Handle<String> source, int script_id) {
   script->set_flags(0);
   script->set_host_defined_options(*empty_fixed_array());
   Handle<WeakArrayList> scripts = script_list();
-  scripts = WeakArrayList::AddToEnd(isolate(), scripts,
-                                    MaybeObjectHandle::Weak(script));
+  scripts = WeakArrayList::Append(isolate(), scripts,
+                                  MaybeObjectHandle::Weak(script));
   heap->set_script_list(*scripts);
   LOG(isolate(), ScriptEvent(Logger::ScriptEventType::kCreate, script_id));
   return script;
@@ -2075,6 +2075,29 @@ Handle<FixedArray> Factory::CopyFixedArrayAndGrow(Handle<FixedArray> array,
   return CopyArrayAndGrow(array, grow_by, AllocationType::kYoung);
 }
 
+Handle<WeakArrayList> Factory::NewUninitializedWeakArrayList(
+    int capacity, AllocationType allocation) {
+  DCHECK_LE(0, capacity);
+  if (capacity == 0) return empty_weak_array_list();
+
+  HeapObject obj = AllocateRawWeakArrayList(capacity, allocation);
+  obj.set_map_after_allocation(*weak_array_list_map(), SKIP_WRITE_BARRIER);
+
+  Handle<WeakArrayList> result(WeakArrayList::cast(obj), isolate());
+  result->set_length(0);
+  result->set_capacity(capacity);
+  return result;
+}
+
+Handle<WeakArrayList> Factory::NewWeakArrayList(int capacity,
+                                                AllocationType allocation) {
+  Handle<WeakArrayList> result =
+      NewUninitializedWeakArrayList(capacity, allocation);
+  MemsetTagged(ObjectSlot(result->data_start()),
+               ReadOnlyRoots(isolate()).undefined_value(), capacity);
+  return result;
+}
+
 Handle<WeakFixedArray> Factory::CopyWeakFixedArrayAndGrow(
     Handle<WeakFixedArray> src, int grow_by) {
   DCHECK(!src->IsTransitionArray());  // Compacted by GC, this code doesn't work
@@ -2086,22 +2109,42 @@ Handle<WeakArrayList> Factory::CopyWeakArrayListAndGrow(
   int old_capacity = src->capacity();
   int new_capacity = old_capacity + grow_by;
   DCHECK_GE(new_capacity, old_capacity);
-  HeapObject obj = AllocateRawWeakArrayList(new_capacity, allocation);
-  obj.set_map_after_allocation(src->map(), SKIP_WRITE_BARRIER);
-
-  WeakArrayList result = WeakArrayList::cast(obj);
+  Handle<WeakArrayList> result =
+      NewUninitializedWeakArrayList(new_capacity, allocation);
   int old_len = src->length();
-  result.set_length(old_len);
-  result.set_capacity(new_capacity);
+  result->set_length(old_len);
 
   // Copy the content.
   DisallowHeapAllocation no_gc;
-  WriteBarrierMode mode = obj.GetWriteBarrierMode(no_gc);
-  result.CopyElements(isolate(), 0, *src, 0, old_len, mode);
-  MemsetTagged(ObjectSlot(result.data_start() + old_len),
+  WriteBarrierMode mode = result->GetWriteBarrierMode(no_gc);
+  result->CopyElements(isolate(), 0, *src, 0, old_len, mode);
+  MemsetTagged(ObjectSlot(result->data_start() + old_len),
                ReadOnlyRoots(isolate()).undefined_value(),
                new_capacity - old_len);
-  return Handle<WeakArrayList>(result, isolate());
+  return result;
+}
+
+Handle<WeakArrayList> Factory::CompactWeakArrayList(Handle<WeakArrayList> src,
+                                                    int new_capacity,
+                                                    AllocationType allocation) {
+  Handle<WeakArrayList> result =
+      NewUninitializedWeakArrayList(new_capacity, allocation);
+
+  // Copy the content.
+  DisallowHeapAllocation no_gc;
+  WriteBarrierMode mode = result->GetWriteBarrierMode(no_gc);
+  int copy_to = 0, length = src->length();
+  for (int i = 0; i < length; i++) {
+    MaybeObject element = src->Get(i);
+    if (element->IsCleared()) continue;
+    result->Set(copy_to++, element, mode);
+  }
+  result->set_length(copy_to);
+
+  MemsetTagged(ObjectSlot(result->data_start() + copy_to),
+               ReadOnlyRoots(isolate()).undefined_value(),
+               new_capacity - copy_to);
+  return result;
 }
 
 Handle<PropertyArray> Factory::CopyPropertyArrayAndGrow(
diff --git a/src/heap/factory.h b/src/heap/factory.h
@@ -514,13 +514,20 @@ class V8_EXPORT_PRIVATE Factory {
   Handle<FixedArray> CopyFixedArrayAndGrow(Handle<FixedArray> array,
                                            int grow_by);
 
+  Handle<WeakArrayList> NewWeakArrayList(
+      int capacity, AllocationType allocation = AllocationType::kYoung);
+
   Handle<WeakFixedArray> CopyWeakFixedArrayAndGrow(Handle<WeakFixedArray> array,
                                                    int grow_by);
 
   Handle<WeakArrayList> CopyWeakArrayListAndGrow(
       Handle<WeakArrayList> array, int grow_by,
       AllocationType allocation = AllocationType::kYoung);
 
+  Handle<WeakArrayList> CompactWeakArrayList(
+      Handle<WeakArrayList> array, int new_capacity,
+      AllocationType allocation = AllocationType::kYoung);
+
   Handle<PropertyArray> CopyPropertyArrayAndGrow(Handle<PropertyArray> array,
                                                  int grow_by);
 
@@ -1093,6 +1100,10 @@ class V8_EXPORT_PRIVATE Factory {
   // Initializes JSObject body starting at given offset.
   void InitializeJSObjectBody(Handle<JSObject> obj, Handle<Map> map,
                               int start_offset);
+
+ private:
+  Handle<WeakArrayList> NewUninitializedWeakArrayList(
+      int capacity, AllocationType allocation = AllocationType::kYoung);
 };
 
 // Utility class to simplify argument handling around JSFunction creation.
diff --git a/src/objects/fixed-array.h b/src/objects/fixed-array.h
@@ -344,6 +344,17 @@ class WeakArrayList : public HeapObject {
       Isolate* isolate, Handle<WeakArrayList> array,
       const MaybeObjectHandle& value1, const MaybeObjectHandle& value2);
 
+  // Appends an element to the array and possibly compacts and shrinks live weak
+  // references to the start of the collection. Only use this method when
+  // indices to elements can change.
+  static Handle<WeakArrayList> Append(
+      Isolate* isolate, Handle<WeakArrayList> array,
+      const MaybeObjectHandle& value,
+      AllocationType allocation = AllocationType::kYoung);
+
+  // Compact weak references to the beginning of the array.
+  V8_EXPORT_PRIVATE void Compact(Isolate* isolate);
+
   inline MaybeObject Get(int index) const;
   inline MaybeObject Get(const Isolate* isolate, int index) const;
 
@@ -357,6 +368,10 @@ class WeakArrayList : public HeapObject {
     return kHeaderSize + capacity * kTaggedSize;
   }
 
+  static constexpr int CapacityForLength(int length) {
+    return length + Max(length / 2, 2);
+  }
+
   // Gives access to raw memory which stores the array's data.
   inline MaybeObjectSlot data_start();
 
@@ -387,6 +402,9 @@ class WeakArrayList : public HeapObject {
   // Returns the number of non-cleaned weak references in the array.
   int CountLiveWeakReferences() const;
 
+  // Returns the number of non-cleaned elements in the array.
+  int CountLiveElements() const;
+
   // Returns whether an entry was found and removed. Will move the elements
   // around in the array - this method can only be used in cases where the user
   // doesn't care about the indices! Users should make sure there are no
diff --git a/src/objects/objects.cc b/src/objects/objects.cc
@@ -3971,6 +3971,65 @@ Handle<WeakArrayList> WeakArrayList::AddToEnd(Isolate* isolate,
   return array;
 }
 
+// static
+Handle<WeakArrayList> WeakArrayList::Append(Isolate* isolate,
+                                            Handle<WeakArrayList> array,
+                                            const MaybeObjectHandle& value,
+                                            AllocationType allocation) {
+  int length = array->length();
+
+  if (length < array->capacity()) {
+    array->Set(length, *value);
+    array->set_length(length + 1);
+    return array;
+  }
+
+  // Not enough space in the array left, either grow, shrink or
+  // compact the array.
+  int new_length = array->CountLiveElements() + 1;
+
+  bool shrink = new_length < length / 4;
+  bool grow = 3 * (length / 4) < new_length;
+
+  if (shrink || grow) {
+    // Grow or shrink array and compact out-of-place.
+    int new_capacity = CapacityForLength(new_length);
+    array = isolate->factory()->CompactWeakArrayList(array, new_capacity,
+                                                     allocation);
+
+  } else {
+    // Perform compaction in the current array.
+    array->Compact(isolate);
+  }
+
+  // Now append value to the array, there should always be enough space now.
+  DCHECK_LT(array->length(), array->capacity());
+
+  // Reload length, allocation might have killed some weak refs.
+  int index = array->length();
+  array->Set(index, *value);
+  array->set_length(index + 1);
+  return array;
+}
+
+void WeakArrayList::Compact(Isolate* isolate) {
+  int length = this->length();
+  int new_length = 0;
+
+  for (int i = 0; i < length; i++) {
+    MaybeObject value = Get(isolate, i);
+
+    if (!value->IsCleared()) {
+      if (new_length != i) {
+        Set(new_length, value);
+      }
+      ++new_length;
+    }
+  }
+
+  set_length(new_length);
+}
+
 bool WeakArrayList::IsFull() { return length() == capacity(); }
 
 // static
@@ -3980,9 +4039,7 @@ Handle<WeakArrayList> WeakArrayList::EnsureSpace(Isolate* isolate,
                                                  AllocationType allocation) {
   int capacity = array->capacity();
   if (capacity < length) {
-    int new_capacity = length;
-    new_capacity = new_capacity + Max(new_capacity / 2, 2);
-    int grow_by = new_capacity - capacity;
+    int grow_by = CapacityForLength(length) - capacity;
     array = isolate->factory()->CopyWeakArrayListAndGrow(array, grow_by,
                                                          allocation);
   }
@@ -3999,6 +4056,16 @@ int WeakArrayList::CountLiveWeakReferences() const {
   return live_weak_references;
 }
 
+int WeakArrayList::CountLiveElements() const {
+  int non_cleared_objects = 0;
+  for (int i = 0; i < length(); i++) {
+    if (!Get(i)->IsCleared()) {
+      ++non_cleared_objects;
+    }
+  }
+  return non_cleared_objects;
+}
+
 bool WeakArrayList::RemoveOne(const MaybeObjectHandle& value) {
   if (length() == 0) return false;
   // Optimize for the most recently added element to be removed again.
diff --git a/test/unittests/BUILD.gn b/test/unittests/BUILD.gn
@@ -209,6 +209,7 @@ v8_source_set("unittests_sources") {
     "objects/object-unittest.cc",
     "objects/osr-optimized-code-cache-unittest.cc",
     "objects/value-serializer-unittest.cc",
+    "objects/weakarraylist-unittest.cc",
     "parser/ast-value-unittest.cc",
     "parser/preparser-unittest.cc",
     "profiler/strings-storage-unittest.cc",
diff --git a/test/unittests/objects/weakarraylist-unittest.cc b/test/unittests/objects/weakarraylist-unittest.cc
@@ -0,0 +1,59 @@
+// Copyright 2019 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "src/heap/factory.h"
+#include "test/unittests/test-utils.h"
+
+namespace v8 {
+namespace internal {
+
+using WeakArrayListTest = TestWithIsolate;
+
+TEST_F(WeakArrayListTest, Compact) {
+  Handle<WeakArrayList> list = isolate()->factory()->NewWeakArrayList(10);
+  EXPECT_EQ(list->length(), 0);
+  EXPECT_EQ(list->capacity(), 10);
+
+  MaybeObject some_object =
+      MaybeObject::FromObject(*isolate()->factory()->empty_fixed_array());
+  MaybeObject weak_ref = MaybeObject::MakeWeak(some_object);
+  MaybeObject smi = MaybeObject::FromSmi(Smi::FromInt(0));
+  MaybeObject cleared_ref = HeapObjectReference::ClearedValue(isolate());
+  list->Set(0, weak_ref);
+  list->Set(1, smi);
+  list->Set(2, cleared_ref);
+  list->Set(3, cleared_ref);
+  list->set_length(5);
+
+  list->Compact(isolate());
+  EXPECT_EQ(list->length(), 3);
+  EXPECT_EQ(list->capacity(), 10);
+}
+
+TEST_F(WeakArrayListTest, OutOfPlaceCompact) {
+  Handle<WeakArrayList> list = isolate()->factory()->NewWeakArrayList(20);
+  EXPECT_EQ(list->length(), 0);
+  EXPECT_EQ(list->capacity(), 20);
+
+  MaybeObject some_object =
+      MaybeObject::FromObject(*isolate()->factory()->empty_fixed_array());
+  MaybeObject weak_ref = MaybeObject::MakeWeak(some_object);
+  MaybeObject smi = MaybeObject::FromSmi(Smi::FromInt(0));
+  MaybeObject cleared_ref = HeapObjectReference::ClearedValue(isolate());
+  list->Set(0, weak_ref);
+  list->Set(1, smi);
+  list->Set(2, cleared_ref);
+  list->Set(3, smi);
+  list->Set(4, cleared_ref);
+  list->set_length(6);
+
+  Handle<WeakArrayList> compacted =
+      isolate()->factory()->CompactWeakArrayList(list, 4);
+  EXPECT_EQ(list->length(), 6);
+  EXPECT_EQ(compacted->length(), 4);
+  EXPECT_EQ(compacted->capacity(), 4);
+}
+
+}  // namespace internal
+}  // namespace v8
