[heap] Cleanup embedder tracing APIs
Provide processing scope that makes it impossible to maintain locally
cached wrappers that could get invalidated in Blink and yield in
crashers.
Bug: chromium:843903, v8:8238
Change-Id: I7ba1905f6c77a97bcc61ac42f921dcac4772471f
Reviewed-on: https://chromium-review.googlesource.com/c/1349276
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#57795}diff --git a/src/heap/embedder-tracing.cc b/src/heap/embedder-tracing.cc
index 3b8e4ae..ff5ffd0 100644
--- a/src/heap/embedder-tracing.cc
+++ b/src/heap/embedder-tracing.cc
@@ -5,14 +5,23 @@
#include "src/heap/embedder-tracing.h"
#include "src/base/logging.h"
+#include "src/objects/embedder-data-slot.h"
+#include "src/objects/js-objects-inl.h"
namespace v8 {
namespace internal {
+void LocalEmbedderHeapTracer::SetRemoteTracer(EmbedderHeapTracer* tracer) {
+ if (remote_tracer_) remote_tracer_->isolate_ = nullptr;
+
+ remote_tracer_ = tracer;
+ if (remote_tracer_)
+ remote_tracer_->isolate_ = reinterpret_cast<v8::Isolate*>(isolate_);
+}
+
void LocalEmbedderHeapTracer::TracePrologue() {
if (!InUse()) return;
- CHECK(cached_wrappers_to_trace_.empty());
num_v8_marking_worklist_was_empty_ = 0;
embedder_worklist_empty_ = false;
remote_tracer_->TracePrologue();
@@ -21,7 +30,6 @@
void LocalEmbedderHeapTracer::TraceEpilogue() {
if (!InUse()) return;
- CHECK(cached_wrappers_to_trace_.empty());
remote_tracer_->TraceEpilogue();
}
@@ -37,30 +45,11 @@
bool LocalEmbedderHeapTracer::Trace(double deadline) {
if (!InUse()) return true;
- DCHECK_EQ(0, NumberOfCachedWrappersToTrace());
return remote_tracer_->AdvanceTracing(deadline);
}
bool LocalEmbedderHeapTracer::IsRemoteTracingDone() {
- return (InUse()) ? cached_wrappers_to_trace_.empty() &&
- remote_tracer_->IsTracingDone()
- : true;
-}
-
-void LocalEmbedderHeapTracer::RegisterWrappersWithRemoteTracer() {
- if (!InUse()) return;
-
- if (cached_wrappers_to_trace_.empty()) {
- return;
- }
-
- remote_tracer_->RegisterV8References(cached_wrappers_to_trace_);
- cached_wrappers_to_trace_.clear();
-}
-
-bool LocalEmbedderHeapTracer::RequiresImmediateWrapperProcessing() {
- const size_t kTooManyWrappers = 16000;
- return cached_wrappers_to_trace_.size() > kTooManyWrappers;
+ return !InUse() || remote_tracer_->IsTracingDone();
}
void LocalEmbedderHeapTracer::SetEmbedderStackStateForNextFinalization(
@@ -70,5 +59,46 @@
embedder_stack_state_ = stack_state;
}
+LocalEmbedderHeapTracer::ProcessingScope::ProcessingScope(
+ LocalEmbedderHeapTracer* tracer)
+ : tracer_(tracer) {
+ wrapper_cache_.reserve(kWrapperCacheSize);
+}
+
+LocalEmbedderHeapTracer::ProcessingScope::~ProcessingScope() {
+ if (!wrapper_cache_.empty()) {
+ tracer_->remote_tracer()->RegisterV8References(std::move(wrapper_cache_));
+ }
+ tracer_->embedder_worklist_empty_ = empty_worklist_;
+}
+
+void LocalEmbedderHeapTracer::ProcessingScope::TracePossibleWrapper(
+ JSObject* js_object) {
+ DCHECK(js_object->IsApiWrapper());
+ if (js_object->GetEmbedderFieldCount() < 2) return;
+
+ void* pointer0;
+ void* pointer1;
+ if (EmbedderDataSlot(js_object, 0).ToAlignedPointer(&pointer0) && pointer0 &&
+ EmbedderDataSlot(js_object, 1).ToAlignedPointer(&pointer1)) {
+ wrapper_cache_.push_back({pointer0, pointer1});
+ }
+ FlushWrapperCacheIfFull();
+}
+
+void LocalEmbedderHeapTracer::ProcessingScope::FlushWrapperCacheIfFull() {
+ if (wrapper_cache_.size() == wrapper_cache_.capacity()) {
+ tracer_->remote_tracer()->RegisterV8References(std::move(wrapper_cache_));
+ wrapper_cache_.clear();
+ wrapper_cache_.reserve(kWrapperCacheSize);
+ }
+}
+
+void LocalEmbedderHeapTracer::ProcessingScope::AddWrapperInfoForTesting(
+ WrapperInfo info) {
+ wrapper_cache_.push_back(info);
+ FlushWrapperCacheIfFull();
+}
+
} // namespace internal
} // namespace v8
diff --git a/src/heap/embedder-tracing.h b/src/heap/embedder-tracing.h
index 7b4d1ab..49103e5 100644
--- a/src/heap/embedder-tracing.h
+++ b/src/heap/embedder-tracing.h
@@ -13,10 +13,32 @@
namespace internal {
class Heap;
+class JSObject;
class V8_EXPORT_PRIVATE LocalEmbedderHeapTracer final {
public:
typedef std::pair<void*, void*> WrapperInfo;
+ typedef std::vector<WrapperInfo> WrapperCache;
+
+ class V8_EXPORT_PRIVATE ProcessingScope {
+ public:
+ explicit ProcessingScope(LocalEmbedderHeapTracer* tracer);
+ ~ProcessingScope();
+
+ void TracePossibleWrapper(JSObject* js_object);
+ void set_not_done() { empty_worklist_ = false; }
+
+ void AddWrapperInfoForTesting(WrapperInfo info);
+
+ private:
+ static constexpr size_t kWrapperCacheSize = 1000;
+
+ void FlushWrapperCacheIfFull();
+
+ LocalEmbedderHeapTracer* const tracer_;
+ WrapperCache wrapper_cache_;
+ bool empty_worklist_ = true;
+ };
explicit LocalEmbedderHeapTracer(Isolate* isolate) : isolate_(isolate) {}
@@ -24,37 +46,16 @@
if (remote_tracer_) remote_tracer_->isolate_ = nullptr;
}
+ bool InUse() const { return remote_tracer_ != nullptr; }
EmbedderHeapTracer* remote_tracer() const { return remote_tracer_; }
- void SetRemoteTracer(EmbedderHeapTracer* tracer) {
- if (remote_tracer_) remote_tracer_->isolate_ = nullptr;
-
- remote_tracer_ = tracer;
- if (remote_tracer_)
- remote_tracer_->isolate_ = reinterpret_cast<v8::Isolate*>(isolate_);
- }
-
- bool InUse() const { return remote_tracer_ != nullptr; }
-
+ void SetRemoteTracer(EmbedderHeapTracer* tracer);
void TracePrologue();
void TraceEpilogue();
void EnterFinalPause();
bool Trace(double deadline);
bool IsRemoteTracingDone();
- size_t NumberOfCachedWrappersToTrace() {
- return cached_wrappers_to_trace_.size();
- }
- void AddWrapperToTrace(WrapperInfo entry) {
- cached_wrappers_to_trace_.push_back(entry);
- }
- void ClearCachedWrappersToTrace() { cached_wrappers_to_trace_.clear(); }
- void RegisterWrappersWithRemoteTracer();
-
- // In order to avoid running out of memory we force tracing wrappers if there
- // are too many of them.
- bool RequiresImmediateWrapperProcessing();
-
void NotifyV8MarkingWorklistWasEmpty() {
num_v8_marking_worklist_was_empty_++;
}
@@ -66,23 +67,16 @@
num_v8_marking_worklist_was_empty_ > kMaxIncrementalFixpointRounds;
}
- void SetEmbedderWorklistEmpty(bool empty) {
- embedder_worklist_empty_ = empty;
- }
-
void SetEmbedderStackStateForNextFinalization(
EmbedderHeapTracer::EmbedderStackState stack_state);
private:
- typedef std::vector<WrapperInfo> WrapperCache;
-
Isolate* const isolate_;
- WrapperCache cached_wrappers_to_trace_;
EmbedderHeapTracer* remote_tracer_ = nullptr;
+
size_t num_v8_marking_worklist_was_empty_ = 0;
EmbedderHeapTracer::EmbedderStackState embedder_stack_state_ =
EmbedderHeapTracer::kUnknown;
-
// Indicates whether the embedder worklist was observed empty on the main
// thread. This is opportunistic as concurrent marking tasks may hold local
// segments of potential embedder fields to move to the main thread.
diff --git a/src/heap/heap.cc b/src/heap/heap.cc
index fbdaa1c..09b00f4 100644
--- a/src/heap/heap.cc
+++ b/src/heap/heap.cc
@@ -2965,9 +2965,6 @@
}
}
}
- // We potentially deserialized wrappers which require registering with the
- // embedder as the marker will not find them.
- local_embedder_heap_tracer()->RegisterWrappersWithRemoteTracer();
// Large object space doesn't use reservations, so it needs custom handling.
for (HeapObject* object : large_objects) {
@@ -4547,18 +4544,6 @@
return local_embedder_heap_tracer()->remote_tracer();
}
-void Heap::TracePossibleWrapper(JSObject* js_object) {
- DCHECK(js_object->IsApiWrapper());
- if (js_object->GetEmbedderFieldCount() < 2) return;
- void* pointer0;
- void* pointer1;
- if (EmbedderDataSlot(js_object, 0).ToAlignedPointer(&pointer0) && pointer0 &&
- EmbedderDataSlot(js_object, 1).ToAlignedPointer(&pointer1)) {
- local_embedder_heap_tracer()->AddWrapperToTrace(
- std::pair<void*, void*>(pointer0, pointer1));
- }
-}
-
void Heap::RegisterExternallyReferencedObject(Address* location) {
// The embedder is not aware of whether numbers are materialized as heap
// objects are just passed around as Smis.
diff --git a/src/heap/heap.h b/src/heap/heap.h
index 0c6c746..86eeb07 100644
--- a/src/heap/heap.h
+++ b/src/heap/heap.h
@@ -893,7 +893,6 @@
void SetEmbedderHeapTracer(EmbedderHeapTracer* tracer);
EmbedderHeapTracer* GetEmbedderHeapTracer() const;
- void TracePossibleWrapper(JSObject* js_object);
void RegisterExternallyReferencedObject(Address* location);
void SetEmbedderStackStateForNextFinalizaton(
EmbedderHeapTracer::EmbedderStackState stack_state);
diff --git a/src/heap/incremental-marking.cc b/src/heap/incremental-marking.cc
index 4fae57c..1a89aa3 100644
--- a/src/heap/incremental-marking.cc
+++ b/src/heap/incremental-marking.cc
@@ -8,6 +8,7 @@
#include "src/compilation-cache.h"
#include "src/conversions.h"
#include "src/heap/concurrent-marking.h"
+#include "src/heap/embedder-tracing.h"
#include "src/heap/gc-idle-time-handler.h"
#include "src/heap/gc-tracer.h"
#include "src/heap/heap-inl.h"
@@ -815,11 +816,6 @@
int size = VisitObject(obj->map(), obj);
bytes_processed += size - unscanned_bytes_of_large_object_;
}
- // Report all found wrappers to the embedder. This is necessary as the
- // embedder could potentially invalidate wrappers as soon as V8 is done
- // with its incremental marking processing. Any cached wrappers could
- // result in broken pointers at this point.
- heap_->local_embedder_heap_tracer()->RegisterWrappersWithRemoteTracer();
return bytes_processed;
}
@@ -829,20 +825,20 @@
TRACE_GC(heap()->tracer(), GCTracer::Scope::MC_INCREMENTAL_EMBEDDER_TRACING);
double deadline = heap_->MonotonicallyIncreasingTimeInMs() + duration_ms;
do {
- HeapObject* object;
- size_t cnt = 0;
- bool embedder_fields_empty = true;
- while (marking_worklist()->embedder()->Pop(0, &object)) {
- heap_->TracePossibleWrapper(JSObject::cast(object));
- if (++cnt == kObjectsToProcessBeforeInterrupt) {
- cnt = 0;
- embedder_fields_empty = false;
- break;
+ {
+ LocalEmbedderHeapTracer::ProcessingScope scope(
+ heap_->local_embedder_heap_tracer());
+ HeapObject* object;
+ size_t cnt = 0;
+ while (marking_worklist()->embedder()->Pop(0, &object)) {
+ scope.TracePossibleWrapper(JSObject::cast(object));
+ if (++cnt == kObjectsToProcessBeforeInterrupt) {
+ cnt = 0;
+ scope.set_not_done();
+ break;
+ }
}
}
- heap_->local_embedder_heap_tracer()->SetEmbedderWorklistEmpty(
- embedder_fields_empty);
- heap_->local_embedder_heap_tracer()->RegisterWrappersWithRemoteTracer();
heap_->local_embedder_heap_tracer()->Trace(deadline);
} while (heap_->MonotonicallyIncreasingTimeInMs() < deadline);
}
@@ -967,8 +963,6 @@
TRACE_EVENT0("v8", "V8.GCIncrementalMarking");
TRACE_GC(heap_->tracer(), GCTracer::Scope::MC_INCREMENTAL);
DCHECK(!IsStopped());
- DCHECK_EQ(
- 0, heap_->local_embedder_heap_tracer()->NumberOfCachedWrappersToTrace());
double remaining_time_in_ms = 0.0;
do {
diff --git a/src/heap/mark-compact-inl.h b/src/heap/mark-compact-inl.h
index 6bb3f2a..16c27ac 100644
--- a/src/heap/mark-compact-inl.h
+++ b/src/heap/mark-compact-inl.h
@@ -74,7 +74,8 @@
MarkingVisitor<fixed_array_mode, retaining_path_mode,
MarkingState>::VisitEmbedderTracingSubclass(Map map, T* object) {
if (heap_->local_embedder_heap_tracer()->InUse()) {
- heap_->TracePossibleWrapper(object);
+ marking_worklist()->embedder()->Push(MarkCompactCollectorBase::kMainThread,
+ object);
}
int size = T::BodyDescriptor::SizeOf(map, object);
T::BodyDescriptor::IterateBody(map, object, size, this);
diff --git a/src/heap/mark-compact.cc b/src/heap/mark-compact.cc
index 2200aa6..3131583 100644
--- a/src/heap/mark-compact.cc
+++ b/src/heap/mark-compact.cc
@@ -1487,6 +1487,7 @@
work_to_do = work_to_do || !marking_worklist()->IsEmpty() ||
heap()->concurrent_marking()->ephemeron_marked() ||
+ !marking_worklist()->IsEmbedderEmpty() ||
!heap()->local_embedder_heap_tracer()->IsRemoteTracingDone();
++iterations;
}
@@ -1614,11 +1615,14 @@
void MarkCompactCollector::PerformWrapperTracing() {
if (heap_->local_embedder_heap_tracer()->InUse()) {
TRACE_GC(heap()->tracer(), GCTracer::Scope::MC_MARK_EMBEDDER_TRACING);
- HeapObject* object;
- while (marking_worklist()->embedder()->Pop(kMainThread, &object)) {
- heap_->TracePossibleWrapper(JSObject::cast(object));
+ {
+ LocalEmbedderHeapTracer::ProcessingScope scope(
+ heap_->local_embedder_heap_tracer());
+ HeapObject* object;
+ while (marking_worklist()->embedder()->Pop(kMainThread, &object)) {
+ scope.TracePossibleWrapper(JSObject::cast(object));
+ }
}
- heap_->local_embedder_heap_tracer()->RegisterWrappersWithRemoteTracer();
heap_->local_embedder_heap_tracer()->Trace(
std::numeric_limits<double>::infinity());
}
@@ -1779,7 +1783,8 @@
// once.
PerformWrapperTracing();
ProcessMarkingWorklist();
- } while (!heap_->local_embedder_heap_tracer()->IsRemoteTracingDone());
+ } while (!heap_->local_embedder_heap_tracer()->IsRemoteTracingDone() ||
+ !marking_worklist()->IsEmbedderEmpty());
DCHECK(marking_worklist()->IsEmbedderEmpty());
DCHECK(marking_worklist()->IsEmpty());
}
diff --git a/src/heap/mark-compact.h b/src/heap/mark-compact.h
index 0bef1f1..dbb2444 100644
--- a/src/heap/mark-compact.h
+++ b/src/heap/mark-compact.h
@@ -235,6 +235,8 @@
// Base class for minor and full MC collectors.
class MarkCompactCollectorBase {
public:
+ static const int kMainThread = 0;
+
virtual ~MarkCompactCollectorBase() = default;
virtual void SetUp() = 0;
@@ -245,7 +247,6 @@
inline Isolate* isolate();
protected:
- static const int kMainThread = 0;
explicit MarkCompactCollectorBase(Heap* heap)
: heap_(heap), old_to_new_slots_(0) {}
diff --git a/src/heap/scavenger.cc b/src/heap/scavenger.cc
index 56aeef7..6649d38 100644
--- a/src/heap/scavenger.cc
+++ b/src/heap/scavenger.cc
@@ -279,10 +279,6 @@
// Update how much has survived scavenge.
heap_->IncrementYoungSurvivorsCounter(heap_->SurvivedNewSpaceObjectSize());
-
- // Scavenger may find new wrappers by iterating objects promoted onto a black
- // page.
- heap_->local_embedder_heap_tracer()->RegisterWrappersWithRemoteTracer();
}
void ScavengerCollector::HandleSurvivingNewLargeObjects() {
diff --git a/test/unittests/heap/embedder-tracing-unittest.cc b/test/unittests/heap/embedder-tracing-unittest.cc
index 941c474..5bbbace 100644
--- a/test/unittests/heap/embedder-tracing-unittest.cc
+++ b/test/unittests/heap/embedder-tracing-unittest.cc
@@ -83,6 +83,14 @@
local_tracer.EnterFinalPause();
}
+TEST(LocalEmbedderHeapTracer, IsRemoteTracingDoneForwards) {
+ StrictMock<MockEmbedderHeapTracer> remote_tracer;
+ LocalEmbedderHeapTracer local_tracer(nullptr);
+ local_tracer.SetRemoteTracer(&remote_tracer);
+ EXPECT_CALL(remote_tracer, IsTracingDone());
+ local_tracer.IsRemoteTracingDone();
+}
+
TEST(LocalEmbedderHeapTracer, EnterFinalPauseDefaultStackStateUnkown) {
StrictMock<MockEmbedderHeapTracer> remote_tracer;
LocalEmbedderHeapTracer local_tracer(nullptr);
@@ -152,52 +160,19 @@
local_tracer.IsRemoteTracingDone();
}
-TEST(LocalEmbedderHeapTracer, NumberOfCachedWrappersToTraceExcludesRemote) {
- LocalEmbedderHeapTracer local_tracer(nullptr);
- StrictMock<MockEmbedderHeapTracer> remote_tracer;
- local_tracer.SetRemoteTracer(&remote_tracer);
- local_tracer.NumberOfCachedWrappersToTrace();
-}
-
-TEST(LocalEmbedderHeapTracer, RegisterWrappersWithRemoteTracer) {
+TEST(LocalEmbedderHeapTracer, RegisterV8ReferencesWithRemoteTracer) {
StrictMock<MockEmbedderHeapTracer> remote_tracer;
LocalEmbedderHeapTracer local_tracer(nullptr);
local_tracer.SetRemoteTracer(&remote_tracer);
- local_tracer.AddWrapperToTrace(CreateWrapperInfo());
- EXPECT_EQ(1u, local_tracer.NumberOfCachedWrappersToTrace());
- EXPECT_CALL(remote_tracer, RegisterV8References(_));
- local_tracer.RegisterWrappersWithRemoteTracer();
- EXPECT_EQ(0u, local_tracer.NumberOfCachedWrappersToTrace());
+ {
+ LocalEmbedderHeapTracer::ProcessingScope scope(&local_tracer);
+ scope.AddWrapperInfoForTesting(CreateWrapperInfo());
+ EXPECT_CALL(remote_tracer, RegisterV8References(_));
+ }
EXPECT_CALL(remote_tracer, IsTracingDone()).WillOnce(Return(false));
EXPECT_FALSE(local_tracer.IsRemoteTracingDone());
}
-TEST(LocalEmbedderHeapTracer, TraceFinishes) {
- StrictMock<MockEmbedderHeapTracer> remote_tracer;
- LocalEmbedderHeapTracer local_tracer(nullptr);
- local_tracer.SetRemoteTracer(&remote_tracer);
- local_tracer.AddWrapperToTrace(CreateWrapperInfo());
- EXPECT_EQ(1u, local_tracer.NumberOfCachedWrappersToTrace());
- EXPECT_CALL(remote_tracer, RegisterV8References(_));
- local_tracer.RegisterWrappersWithRemoteTracer();
- EXPECT_CALL(remote_tracer, AdvanceTracing(_)).WillOnce(Return(true));
- EXPECT_TRUE(local_tracer.Trace(std::numeric_limits<double>::infinity()));
- EXPECT_EQ(0u, local_tracer.NumberOfCachedWrappersToTrace());
-}
-
-TEST(LocalEmbedderHeapTracer, TraceDoesNotFinish) {
- StrictMock<MockEmbedderHeapTracer> remote_tracer;
- LocalEmbedderHeapTracer local_tracer(nullptr);
- local_tracer.SetRemoteTracer(&remote_tracer);
- local_tracer.AddWrapperToTrace(CreateWrapperInfo());
- EXPECT_EQ(1u, local_tracer.NumberOfCachedWrappersToTrace());
- EXPECT_CALL(remote_tracer, RegisterV8References(_));
- local_tracer.RegisterWrappersWithRemoteTracer();
- EXPECT_CALL(remote_tracer, AdvanceTracing(_)).WillOnce(Return(false));
- EXPECT_FALSE(local_tracer.Trace(1.0));
- EXPECT_EQ(0u, local_tracer.NumberOfCachedWrappersToTrace());
-}
-
TEST_F(LocalEmbedderHeapTracerWithIsolate, SetRemoteTracerSetsIsolate) {
StrictMock<MockEmbedderHeapTracer> remote_tracer;
LocalEmbedderHeapTracer local_tracer(isolate());
