Skip to content

Commit

Permalink
deps: V8: backport fb26d0bb1835
Browse files Browse the repository at this point in the history
Original commit message:

    [objects] Compact and shrink script_list

    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}

Refs: v8/v8@fb26d0b

PR-URL: #33573
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
  • Loading branch information
mmarchini authored and BethGriggs committed Jun 26, 2020
1 parent 136d8ea commit 97a3f7b
Show file tree
Hide file tree
Showing 7 changed files with 215 additions and 16 deletions.
2 changes: 1 addition & 1 deletion common.gypi
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@

# Reset this number to 0 on major V8 upgrades.
# Increment by one for each non-official patch applied to deps/v8.
'v8_embedder_string': '-node.38',
'v8_embedder_string': '-node.39',

##### V8 defaults for Node.js #####

Expand Down
67 changes: 55 additions & 12 deletions deps/v8/src/heap/factory.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1677,8 +1677,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));
TRACE_EVENT_OBJECT_CREATED_WITH_ID(
Expand Down Expand Up @@ -2112,6 +2112,29 @@ Handle<FixedArray> Factory::CopyFixedArrayAndGrow(Handle<FixedArray> array,
return CopyArrayAndGrow(array, grow_by, allocation);
}

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, AllocationType allocation) {
DCHECK(!src->IsTransitionArray()); // Compacted by GC, this code doesn't work
Expand All @@ -2123,22 +2146,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(
Expand Down
11 changes: 11 additions & 0 deletions deps/v8/src/heap/factory.h
Original file line number Diff line number Diff line change
Expand Up @@ -524,6 +524,9 @@ class V8_EXPORT_PRIVATE Factory {

Handle<JSObject> NewFunctionPrototype(Handle<JSFunction> function);

Handle<WeakArrayList> NewWeakArrayList(
int capacity, AllocationType allocation = AllocationType::kYoung);

Handle<WeakCell> NewWeakCell();

// Returns a deep copy of the JavaScript object.
Expand All @@ -549,6 +552,10 @@ class V8_EXPORT_PRIVATE Factory {
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,
AllocationType allocation = AllocationType::kYoung);
Expand Down Expand Up @@ -1111,6 +1118,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.
Expand Down
18 changes: 18 additions & 0 deletions deps/v8/src/objects/fixed-array.h
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,17 @@ class WeakArrayList : public HeapObject {
Isolate* isolate, Handle<WeakArrayList> array,
const MaybeObjectHandle& value);

// 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(Isolate* isolate, int index) const;

Expand All @@ -352,6 +363,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();

Expand Down Expand Up @@ -383,6 +398,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
Expand Down
73 changes: 70 additions & 3 deletions deps/v8/src/objects/objects.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3952,6 +3952,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
Expand All @@ -3961,9 +4020,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);
}
Expand All @@ -3980,6 +4037,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.
Expand Down
1 change: 1 addition & 0 deletions deps/v8/test/unittests/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,7 @@ v8_source_set("unittests_sources") {
"numbers/conversions-unittest.cc",
"objects/object-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",
Expand Down
59 changes: 59 additions & 0 deletions deps/v8/test/unittests/objects/weakarraylist-unittest.cc
Original file line number Diff line number Diff line change
@@ -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

0 comments on commit 97a3f7b

Please sign in to comment.