From 6a988c61b7cfd11b4927b2645e7573e573d03d3b Mon Sep 17 00:00:00 2001 From: Ali Ijaz Sheikh Date: Thu, 28 Sep 2017 13:14:33 -0700 Subject: [PATCH] deps: V8: cherry-pick 163d360 from upstream MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Original commit message [heap] Fix memory leak in the remembered set. Empty slot set buckets can leak in the following scenarios. Scenario 1 (large object space): 1) A large array is allocated in the large object space. 2) The array is filled with old->new references, which allocates new slot set buckets. 3) The references are overwritten with smis or old space pointers, which make the slots set buckets empty. 4) Garbage collection (scavenge or mark-compact) iterates the slots set of the array and pre-frees the empty buckets. 5) Steps 2-4 repeated many times and leak arbitary many empty buckets. The fix to free empty buckets for large object space in mark-compact. Scenario 2 (no mark-compact): 1) A small array is allocated in the old space. 2) The array is filled with old->new references, which allocates new slot set buckets. 3) The references are overwritten with smis or old space pointers, which make the slots set buckets empty. 4) Scavenge iterates the slots set of the array and pre-frees the empty buckets. 5) Steps 2-4 repeated many times and leak arbitary many empty buckets. The fix to free empty buckets for swept pages in scavenger. Bug: v8:6800 TBR: mlippautz@chromium.org Change-Id: I48d94870f5acf4f6208858271886911c895a9126 Reviewed-on: https://chromium-review.googlesource.com/668442 Reviewed-by: Ulan Degenbaev Commit-Queue: Ulan Degenbaev Cr-Commit-Position: refs/heads/master@{#48041} PR-URL: https://github.com/nodejs/node/pull/15664 Reviewed-By: Colin Ihrig Reviewed-By: Benedikt Meurer Reviewed-By: Michaël Zasso Reviewed-By: James M Snell --- deps/v8/src/heap/heap.cc | 6 +++- deps/v8/src/heap/remembered-set.h | 25 ++++++++++++++ deps/v8/src/heap/slot-set.h | 36 +++++++++++++++----- deps/v8/src/heap/spaces.cc | 1 + deps/v8/test/cctest/heap/test-heap.cc | 47 +++++++++++++++++++++++++++ 5 files changed, 105 insertions(+), 10 deletions(-) diff --git a/deps/v8/src/heap/heap.cc b/deps/v8/src/heap/heap.cc index b168c3104c7666..65526f748c4e6d 100644 --- a/deps/v8/src/heap/heap.cc +++ b/deps/v8/src/heap/heap.cc @@ -1814,7 +1814,11 @@ void Heap::Scavenge() { ArrayBufferTracker::FreeDeadInNewSpace(this); RememberedSet::IterateMemoryChunks(this, [](MemoryChunk* chunk) { - RememberedSet::PreFreeEmptyBuckets(chunk); + if (chunk->SweepingDone()) { + RememberedSet::FreeEmptyBuckets(chunk); + } else { + RememberedSet::PreFreeEmptyBuckets(chunk); + } }); // Update how much has survived scavenge. diff --git a/deps/v8/src/heap/remembered-set.h b/deps/v8/src/heap/remembered-set.h index 5908940d9e913c..543becfdf6ff76 100644 --- a/deps/v8/src/heap/remembered-set.h +++ b/deps/v8/src/heap/remembered-set.h @@ -150,6 +150,19 @@ class RememberedSet : public AllStatic { } } + static int NumberOfPreFreedEmptyBuckets(MemoryChunk* chunk) { + DCHECK(type == OLD_TO_NEW); + int result = 0; + SlotSet* slots = chunk->slot_set(); + if (slots != nullptr) { + size_t pages = (chunk->size() + Page::kPageSize - 1) / Page::kPageSize; + for (size_t page = 0; page < pages; page++) { + result += slots[page].NumberOfPreFreedEmptyBuckets(); + } + } + return result; + } + static void PreFreeEmptyBuckets(MemoryChunk* chunk) { DCHECK(type == OLD_TO_NEW); SlotSet* slots = chunk->slot_set(); @@ -161,6 +174,18 @@ class RememberedSet : public AllStatic { } } + static void FreeEmptyBuckets(MemoryChunk* chunk) { + DCHECK(type == OLD_TO_NEW); + SlotSet* slots = chunk->slot_set(); + if (slots != nullptr) { + size_t pages = (chunk->size() + Page::kPageSize - 1) / Page::kPageSize; + for (size_t page = 0; page < pages; page++) { + slots[page].FreeEmptyBuckets(); + slots[page].FreeToBeFreedBuckets(); + } + } + } + // Given a page and a typed slot in that page, this function adds the slot // to the remembered set. static void InsertTyped(Page* page, Address host_addr, SlotType slot_type, diff --git a/deps/v8/src/heap/slot-set.h b/deps/v8/src/heap/slot-set.h index 64ba266f21f46b..8da187d8a1057a 100644 --- a/deps/v8/src/heap/slot-set.h +++ b/deps/v8/src/heap/slot-set.h @@ -225,25 +225,33 @@ class SlotSet : public Malloced { return new_count; } + int NumberOfPreFreedEmptyBuckets() { + base::LockGuard guard(&to_be_freed_buckets_mutex_); + return static_cast(to_be_freed_buckets_.size()); + } + void PreFreeEmptyBuckets() { for (int bucket_index = 0; bucket_index < kBuckets; bucket_index++) { Bucket bucket = LoadBucket(&buckets_[bucket_index]); if (bucket != nullptr) { - bool found_non_empty_cell = false; - int cell_offset = bucket_index * kBitsPerBucket; - for (int i = 0; i < kCellsPerBucket; i++, cell_offset += kBitsPerCell) { - if (LoadCell(&bucket[i])) { - found_non_empty_cell = true; - break; - } - } - if (!found_non_empty_cell) { + if (IsEmptyBucket(bucket)) { PreFreeEmptyBucket(bucket_index); } } } } + void FreeEmptyBuckets() { + for (int bucket_index = 0; bucket_index < kBuckets; bucket_index++) { + Bucket bucket = LoadBucket(&buckets_[bucket_index]); + if (bucket != nullptr) { + if (IsEmptyBucket(bucket)) { + ReleaseBucket(bucket_index); + } + } + } + } + void FreeToBeFreedBuckets() { base::LockGuard guard(&to_be_freed_buckets_mutex_); while (!to_be_freed_buckets_.empty()) { @@ -251,6 +259,7 @@ class SlotSet : public Malloced { to_be_freed_buckets_.pop(); DeleteArray(top); } + DCHECK_EQ(0u, to_be_freed_buckets_.size()); } private: @@ -313,6 +322,15 @@ class SlotSet : public Malloced { } } + bool IsEmptyBucket(Bucket bucket) { + for (int i = 0; i < kCellsPerBucket; i++) { + if (LoadCell(&bucket[i])) { + return false; + } + } + return true; + } + template bool SwapInNewBucket(Bucket* bucket, Bucket value) { if (access_mode == AccessMode::ATOMIC) { diff --git a/deps/v8/src/heap/spaces.cc b/deps/v8/src/heap/spaces.cc index f275a4d518d9d4..798ef89ec1f201 100644 --- a/deps/v8/src/heap/spaces.cc +++ b/deps/v8/src/heap/spaces.cc @@ -3308,6 +3308,7 @@ void LargeObjectSpace::ClearMarkingStateOfLiveObjects() { Marking::MarkWhite( ObjectMarking::MarkBitFrom(obj, MarkingState::Internal(obj))); MemoryChunk* chunk = MemoryChunk::FromAddress(obj->address()); + RememberedSet::FreeEmptyBuckets(chunk); chunk->ResetProgressBar(); MarkingState::Internal(chunk).SetLiveBytes(0); } diff --git a/deps/v8/test/cctest/heap/test-heap.cc b/deps/v8/test/cctest/heap/test-heap.cc index d9608292e8c0ed..a2273d2d8c9fe7 100644 --- a/deps/v8/test/cctest/heap/test-heap.cc +++ b/deps/v8/test/cctest/heap/test-heap.cc @@ -6237,6 +6237,53 @@ HEAP_TEST(Regress5831) { CHECK(chunk->NeverEvacuate()); } +TEST(Regress6800) { + CcTest::InitializeVM(); + Isolate* isolate = CcTest::i_isolate(); + HandleScope handle_scope(isolate); + + const int kRootLength = 1000; + Handle root = + isolate->factory()->NewFixedArray(kRootLength, TENURED); + { + HandleScope inner_scope(isolate); + Handle new_space_array = isolate->factory()->NewFixedArray(1); + for (int i = 0; i < kRootLength; i++) { + root->set(i, *new_space_array); + } + for (int i = 0; i < kRootLength; i++) { + root->set(i, CcTest::heap()->undefined_value()); + } + } + CcTest::CollectGarbage(NEW_SPACE); + CHECK_EQ(0, RememberedSet::NumberOfPreFreedEmptyBuckets( + MemoryChunk::FromAddress(root->address()))); +} + +TEST(Regress6800LargeObject) { + CcTest::InitializeVM(); + Isolate* isolate = CcTest::i_isolate(); + HandleScope handle_scope(isolate); + + const int kRootLength = i::kMaxRegularHeapObjectSize / kPointerSize; + Handle root = + isolate->factory()->NewFixedArray(kRootLength, TENURED); + CcTest::heap()->lo_space()->Contains(*root); + { + HandleScope inner_scope(isolate); + Handle new_space_array = isolate->factory()->NewFixedArray(1); + for (int i = 0; i < kRootLength; i++) { + root->set(i, *new_space_array); + } + for (int i = 0; i < kRootLength; i++) { + root->set(i, CcTest::heap()->undefined_value()); + } + } + CcTest::CollectGarbage(OLD_SPACE); + CHECK_EQ(0, RememberedSet::NumberOfPreFreedEmptyBuckets( + MemoryChunk::FromAddress(root->address()))); +} + HEAP_TEST(RegressMissingWriteBarrierInAllocate) { if (!FLAG_incremental_marking) return; FLAG_black_allocation = true;