heap: Uniform object construction
Switch to a uniform object construction that does not differentiate between
mixins and regular garabge collected objects and as a consequence can interrupt
both construction cases with garbage collections.
Object construction now uniformely works as follows:
1. The object under construction is marked so in the HeapObjectHeader.
2. Upon discovering such an object it is delayed in a separate marking worklist.
3. Upon hitting the main atomic pause all such objects are conservatively
scanned for pointers without using the Trace method.
Special cases:
a. Mixins in construction: The HeapObjectHeader cannot retrived for such
objects. A default GetTraceDescriptor() implementation returning a sentinel
marker is used to discover that such objects should be delayed.
b. Upon reaching a safepoint (e.g. no stack), in-construction objects are moved
to a marking worklist that, similar to the regular marking worklist, allows for
incremental processing.
Effects:
- No more TLS access for no-GC scope on mixin construction.
- No more memory needed for the no-GC scope marker in mixin classes
- MakeGarbageCollected should require less binary size as implementations can be
aligned.
- Incremental marking Start and Step operations are safe to be called with stack
(even though it is preferable to not do so).
- Object construction is safe to be used with a concurrent marker as it is ok
for any objects (mixins as well as normal ones) to be published to the object
graph during constructors.
Change-Id: Ia39b5e76d551ed194612091167e2370b2d6a4e5f
Bug: 911662
Reviewed-on: https://chromium-review.googlesource.com/c/1401070
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#621637}diff --git a/third_party/blink/renderer/core/dom/live_node_list_registry.cc b/third_party/blink/renderer/core/dom/live_node_list_registry.cc
index 7250fafc..1ca504d3 100644
--- a/third_party/blink/renderer/core/dom/live_node_list_registry.cc
+++ b/third_party/blink/renderer/core/dom/live_node_list_registry.cc
@@ -44,7 +44,7 @@
void LiveNodeListRegistry::ClearWeakMembers(Visitor*) {
auto* it = std::remove_if(data_.begin(), data_.end(), [](Entry entry) {
- return !ObjectAliveTrait<LiveNodeListBase>::IsHeapObjectAlive(entry.first);
+ return !ThreadHeap::IsHeapObjectAlive(entry.first);
});
if (it == data_.end())
return;
diff --git a/third_party/blink/renderer/core/dom/shadow_root.cc b/third_party/blink/renderer/core/dom/shadow_root.cc
index 04bd6dd..7e88116 100644
--- a/third_party/blink/renderer/core/dom/shadow_root.cc
+++ b/third_party/blink/renderer/core/dom/shadow_root.cc
@@ -56,7 +56,6 @@
}
struct SameSizeAsShadowRoot : public DocumentFragment, public TreeScope {
- char empty_class_fields_due_to_gc_mixin_marker[1];
Member<void*> member[3];
unsigned counters_and_flags[1];
};
diff --git a/third_party/blink/renderer/modules/webmidi/midi_dispatcher.h b/third_party/blink/renderer/modules/webmidi/midi_dispatcher.h
index f40a3cc4..30be342 100644
--- a/third_party/blink/renderer/modules/webmidi/midi_dispatcher.h
+++ b/third_party/blink/renderer/modules/webmidi/midi_dispatcher.h
@@ -18,6 +18,7 @@
public midi::mojom::blink::MidiSessionClient {
public:
static MIDIDispatcher& Instance();
+ MIDIDispatcher();
~MIDIDispatcher() override;
void Trace(Visitor* visitor);
@@ -44,10 +45,6 @@
base::TimeTicks timestamp) override;
private:
- friend class ConstructTrait<MIDIDispatcher>;
-
- MIDIDispatcher();
-
midi::mojom::blink::MidiSessionProvider& GetMidiSessionProvider();
midi::mojom::blink::MidiSession& GetMidiSession();
diff --git a/third_party/blink/renderer/platform/heap/garbage_collected.h b/third_party/blink/renderer/platform/heap/garbage_collected.h
index 7326e43..6bb10aa 100644
--- a/third_party/blink/renderer/platform/heap/garbage_collected.h
+++ b/third_party/blink/renderer/platform/heap/garbage_collected.h
@@ -47,12 +47,17 @@
static const bool value = sizeof(CheckMarker<T>(nullptr)) == sizeof(YesType);
};
+// TraceDescriptor is used to describe how to trace an object.
struct TraceDescriptor {
STACK_ALLOCATED();
public:
+ // The adjusted base pointer of the object that should be traced.
void* base_object_payload;
+ // A callback for tracing the object.
TraceCallback callback;
+ // Indicator whether this object can be traced recursively or whether it
+ // requires iterative tracing.
bool can_trace_eagerly;
};
@@ -72,11 +77,14 @@
// USING_GARBAGE_COLLECTED_MIXIN(A);
// };
//
-// With the helper, as long as we are using Member<B>, TypeTrait<B> will
-// dispatch dynamically to retrieve the necessary tracing and header methods.
-// Note that this is only enabled for Member<B>. For Member<A> which we can
-// compute the necessary methods and pointers statically and this dynamic
-// dispatch is not used.
+// The classes involved and the helper macros allow for properly handling
+// definitions for Member<B> and friends. The mechanisms for handling Member<B>
+// involve dynamic dispatch. Note that for Member<A> all methods and pointers
+// are statically computed and no dynamic dispatch is involved.
+//
+// Note that garbage collections are allowed during mixin construction as
+// conservative scanning of objects does not rely on the Trace method but rather
+// scans the object field by field.
class PLATFORM_EXPORT GarbageCollectedMixin {
public:
typedef int IsGarbageCollectedMixinMarker;
@@ -86,7 +94,8 @@
// these objects can processed later on. This is necessary as
// not-fully-constructed mixin objects potentially require being processed
// as part emitting a write barrier for incremental marking. See
- // IncrementalMarkingTest::WriteBarrierDuringMixinConstruction as an example.
+ // |IncrementalMarkingTest::WriteBarrierDuringMixinConstruction| as an
+ // example.
//
// The not-fully-constructed objects are handled as follows:
// 1. Write barrier or marking of not fully constructed mixin gets called.
@@ -127,95 +136,34 @@
\
private:
-// A C++ object's vptr will be initialized to its leftmost base's vtable after
-// the constructors of all its subclasses have run, so if a subclass constructor
-// tries to access any of the vtbl entries of its leftmost base prematurely,
-// it'll find an as-yet incorrect vptr and fail. Which is exactly what a
-// garbage collector will try to do if it tries to access the leftmost base
-// while one of the subclass constructors of a GC mixin object triggers a GC.
-// It is consequently not safe to allow any GCs while these objects are under
-// (sub constructor) construction.
-//
-// To prevent GCs in that restricted window of a mixin object's construction:
-//
-// - The initial allocation of the mixin object will enter a no GC scope.
-// This is done by overriding 'operator new' for mixin instances.
-// - When the constructor for the mixin is invoked, after all the
-// derived constructors have run, it will invoke the constructor
-// for a field whose only purpose is to leave the GC scope.
-// GarbageCollectedMixinConstructorMarker's constructor takes care of
-// this and the field is declared by way of USING_GARBAGE_COLLECTED_MIXIN().
-
-#define DEFINE_GARBAGE_COLLECTED_MIXIN_CONSTRUCTOR_MARKER(TYPE) \
- public: \
- GarbageCollectedMixinConstructorMarker<ThreadingTrait<TYPE>::kAffinity> \
- mixin_constructor_marker_; \
- \
+// The Oilpan GC plugin checks for proper usages of the
+// USING_GARBAGE_COLLECTED_MIXIN macro using a typedef marker.
+#define DEFINE_GARBAGE_COLLECTED_MIXIN_CONSTRUCTOR_MARKER(TYPE) \
+ public: \
+ typedef int HasUsingGarbageCollectedMixinMacro; \
+ \
private:
-// Mixins that wrap/nest others requires extra handling:
-//
-// class A : public GarbageCollected<A>, public GarbageCollectedMixin {
-// USING_GARBAGE_COLLECTED_MIXIN(A);
-// ...
-// }'
-// public B final : public A, public SomeOtherMixinInterface {
-// USING_GARBAGE_COLLECTED_MIXIN(B);
-// ...
-// };
-//
-// The "operator new" for B will enter the forbidden GC scope, but
-// upon construction, two GarbageCollectedMixinConstructorMarker constructors
-// will run -- one for A (first) and another for B (secondly). Only
-// the second one should leave the forbidden GC scope. This is realized by
-// recording the address of B's GarbageCollectedMixinConstructorMarker
-// when the "operator new" for B runs, and leaving the forbidden GC scope
-// when the constructor of the recorded GarbageCollectedMixinConstructorMarker
-// runs.
+// The USING_GARBAGE_COLLECTED_MIXIN macro defines all methods and markers
+// needed for handling mixins.
#define USING_GARBAGE_COLLECTED_MIXIN(TYPE) \
IS_GARBAGE_COLLECTED_TYPE(); \
DEFINE_GARBAGE_COLLECTED_MIXIN_METHODS(TYPE) \
DEFINE_GARBAGE_COLLECTED_MIXIN_CONSTRUCTOR_MARKER(TYPE)
-// An empty class with a constructor that's arranged invoked when all derived
-// constructors of a mixin instance have completed and it is safe to allow GCs
-// again. See AllocateObjectTrait<> comment for more.
-//
-// USING_GARBAGE_COLLECTED_MIXIN() declares a
-// GarbageCollectedMixinConstructorMarker<> private field. By following Blink
-// convention of using the macro at the top of a class declaration, its
-// constructor will run first.
-class GarbageCollectedMixinConstructorMarkerBase {};
-template <ThreadAffinity affinity>
-class GarbageCollectedMixinConstructorMarker
- : public GarbageCollectedMixinConstructorMarkerBase {
- public:
- GarbageCollectedMixinConstructorMarker() {
- // FIXME: if prompt conservative GCs are needed, forced GCs that
- // were denied while within this scope, could now be performed.
- // For now, assume the next out-of-line allocation request will
- // happen soon enough and take care of it. Mixin objects aren't
- // overly common.
- ThreadState* state = ThreadStateFor<affinity>::GetState();
- state->LeaveGCForbiddenScopeIfNeeded(this);
- }
-};
-
// Merge two or more Mixins into one:
//
// class A : public GarbageCollectedMixin {};
// class B : public GarbageCollectedMixin {};
// class C : public A, public B {
// // C::GetTraceDescriptor is now ambiguous because there are two
-// candidates:
-// // A::GetTraceDescriptor and B::GetTraceDescriptor. Ditto for other
-// functions.
+// // candidates: A::GetTraceDescriptor and B::GetTraceDescriptor. Ditto for
+// // other functions.
//
// MERGE_GARBAGE_COLLECTED_MIXINS();
// // The macro defines C::GetTraceDescriptor, etc. so that they are no
-// longer
-// // ambiguous. USING_GARBAGE_COLLECTED_MIXIN(TYPE) overrides them later
-// // and provides the implementations.
+// // longer ambiguous. USING_GARBAGE_COLLECTED_MIXIN(TYPE) overrides them
+// // later and provides the implementations.
// };
#define MERGE_GARBAGE_COLLECTED_MIXINS() \
public: \
diff --git a/third_party/blink/renderer/platform/heap/heap.cc b/third_party/blink/renderer/platform/heap/heap.cc
index 5dddc0e..cf507ab 100644
--- a/third_party/blink/renderer/platform/heap/heap.cc
+++ b/third_party/blink/renderer/platform/heap/heap.cc
@@ -165,10 +165,31 @@
void ThreadHeap::DecommitCallbackStacks() {
marking_worklist_.reset(nullptr);
- not_fully_constructed_worklist_.reset(nullptr);
previously_not_fully_constructed_worklist_.reset(nullptr);
weak_callback_worklist_.reset(nullptr);
ephemeron_callbacks_.clear();
+
+ // The fixed point iteration may have found not-fully-constructed objects.
+ // Such objects should have already been found through the stack scan though
+ // and should thus already be marked.
+ if (!not_fully_constructed_worklist_->IsGlobalEmpty()) {
+#if DCHECK_IS_ON()
+ NotFullyConstructedItem item;
+ while (not_fully_constructed_worklist_->Pop(WorklistTaskId::MainThread,
+ &item)) {
+ BasePage* const page = PageFromObject(item);
+ HeapObjectHeader* const header =
+ page->IsLargeObjectPage()
+ ? static_cast<LargeObjectPage*>(page)->ObjectHeader()
+ : static_cast<NormalPage*>(page)->FindHeaderFromAddress(
+ reinterpret_cast<Address>(const_cast<void*>(item)));
+ DCHECK(header->IsMarked());
+ }
+#else
+ not_fully_constructed_worklist_->Clear();
+#endif
+ }
+ not_fully_constructed_worklist_.reset(nullptr);
}
HeapCompact* ThreadHeap::Compaction() {
@@ -260,8 +281,6 @@
} // namespace
bool ThreadHeap::AdvanceMarking(MarkingVisitor* visitor, TimeTicks deadline) {
- // const size_t kDeadlineCheckInterval = 2500;
- // size_t processed_callback_count = 0;
bool finished;
// Ephemeron fixed point loop.
do {
@@ -271,11 +290,13 @@
ThreadHeapStatsCollector::Scope stats_scope(
stats_collector(), ThreadHeapStatsCollector::kMarkProcessWorklist);
- finished =
- DrainWorklistWithDeadline(deadline, marking_worklist_.get(),
- [visitor](const MarkingItem& item) {
- item.callback(visitor, item.object);
- });
+ finished = DrainWorklistWithDeadline(
+ deadline, marking_worklist_.get(),
+ [visitor](const MarkingItem& item) {
+ DCHECK(!HeapObjectHeader::FromPayload(item.object)
+ ->IsInConstruction());
+ item.callback(visitor, item.object);
+ });
if (!finished)
return false;
@@ -600,6 +621,12 @@
if (header->IsMarked())
return;
+ if (header->IsInConstruction()) {
+ not_fully_constructed_worklist_->Push(WorklistTaskId::MainThread,
+ header->Payload());
+ return;
+ }
+
// Mark and push trace callback.
header->Mark();
marking_worklist_->Push(
diff --git a/third_party/blink/renderer/platform/heap/heap.h b/third_party/blink/renderer/platform/heap/heap.h
index 6919e657..0f81c2b 100644
--- a/third_party/blink/renderer/platform/heap/heap.h
+++ b/third_party/blink/renderer/platform/heap/heap.h
@@ -121,6 +121,8 @@
template <typename T>
class UntracedMember;
+namespace internal {
+
template <typename T, bool = NeedsAdjustPointer<T>::value>
class ObjectAliveTrait;
@@ -143,10 +145,17 @@
NO_SANITIZE_ADDRESS
static bool IsHeapObjectAlive(const T* object) {
static_assert(sizeof(T), "T must be fully defined");
- return object->GetHeapObjectHeader()->IsMarked();
+ const HeapObjectHeader* header = object->GetHeapObjectHeader();
+ if (header == BlinkGC::kNotFullyConstructedObject) {
+ // Objects under construction are always alive.
+ return true;
+ }
+ return header->IsMarked();
}
};
+} // namespace internal
+
class PLATFORM_EXPORT ThreadHeap {
public:
explicit ThreadHeap(ThreadState*);
@@ -173,7 +182,7 @@
return true;
DCHECK(&ThreadState::Current()->Heap() ==
&PageFromObject(object)->Arena()->GetThreadState()->Heap());
- return ObjectAliveTrait<T>::IsHeapObjectAlive(object);
+ return internal::ObjectAliveTrait<T>::IsHeapObjectAlive(object);
}
template <typename T>
static inline bool IsHeapObjectAlive(const Member<T>& member) {
@@ -546,53 +555,18 @@
DISALLOW_COPY_AND_ASSIGN(GarbageCollected);
};
-template <typename T, bool is_mixin = IsGarbageCollectedMixin<T>::value>
-class ConstructTrait {
- public:
-};
-
-template <typename T>
-class ConstructTrait<T, false> {
- public:
- template <typename... Args>
- static T* Construct(Args&&... args) {
- void* memory =
- T::AllocateObject(sizeof(T), IsEagerlyFinalizedType<T>::value);
- HeapObjectHeader* header = HeapObjectHeader::FromPayload(memory);
- header->MarkIsInConstruction();
- // Placement new as regular operator new() is deleted.
- T* object = ::new (memory) T(std::forward<Args>(args)...);
- header->UnmarkIsInConstruction();
- return object;
- }
-};
-
-template <typename T>
-class ConstructTrait<T, true> {
- public:
- template <typename... Args>
- NO_SANITIZE_UNRELATED_CAST static T* Construct(Args&&... args) {
- void* memory =
- T::AllocateObject(sizeof(T), IsEagerlyFinalizedType<T>::value);
- HeapObjectHeader* header = HeapObjectHeader::FromPayload(memory);
- header->MarkIsInConstruction();
- ThreadState* state =
- ThreadStateFor<ThreadingTrait<T>::kAffinity>::GetState();
- state->EnterGCForbiddenScopeIfNeeded(
- &(reinterpret_cast<T*>(memory)->mixin_constructor_marker_));
- // Placement new as regular operator new() is deleted.
- T* object = ::new (memory) T(std::forward<Args>(args)...);
- header->UnmarkIsInConstruction();
- return object;
- }
-};
-
// Constructs an instance of T, which is a garbage collected type.
template <typename T, typename... Args>
T* MakeGarbageCollected(Args&&... args) {
static_assert(WTF::IsGarbageCollectedType<T>::value,
"T needs to be a garbage collected object");
- return ConstructTrait<T>::Construct(std::forward<Args>(args)...);
+ void* memory = T::AllocateObject(sizeof(T), IsEagerlyFinalizedType<T>::value);
+ HeapObjectHeader* header = HeapObjectHeader::FromPayload(memory);
+ header->MarkIsInConstruction();
+ // Placement new as regular operator new() is deleted.
+ T* object = ::new (memory) T(std::forward<Args>(args)...);
+ header->UnmarkIsInConstruction();
+ return object;
}
// Assigning class types to their arenas.
@@ -753,7 +727,7 @@
// preserved to avoid reviving objects in containers.
return;
}
- if (!ObjectAliveTrait<T>::IsHeapObjectAlive(contents))
+ if (!ThreadHeap::IsHeapObjectAlive(contents))
*cell = nullptr;
}
}
diff --git a/third_party/blink/renderer/platform/heap/heap_test.cc b/third_party/blink/renderer/platform/heap/heap_test.cc
index 4b1c81e..c042269 100644
--- a/third_party/blink/renderer/platform/heap/heap_test.cc
+++ b/third_party/blink/renderer/platform/heap/heap_test.cc
@@ -5367,59 +5367,6 @@
return true;
}
-static bool CheckGCForbidden() {
- DCHECK(ThreadState::Current()->IsGCForbidden());
- return true;
-}
-
-class MixinClass : public GarbageCollectedMixin {
- public:
- MixinClass() : dummy_(CheckGCForbidden()) {}
-
- private:
- bool dummy_;
-};
-
-class ClassWithGarbageCollectingMixinConstructor
- : public GarbageCollected<ClassWithGarbageCollectingMixinConstructor>,
- public MixinClass {
- USING_GARBAGE_COLLECTED_MIXIN(ClassWithGarbageCollectingMixinConstructor);
-
- public:
- static int trace_called_;
-
- ClassWithGarbageCollectingMixinConstructor()
- : trace_counter_(TraceCounter::Create()),
- wrapper_(IntWrapper::Create(32)) {}
-
- void Trace(blink::Visitor* visitor) override {
- trace_called_++;
- visitor->Trace(trace_counter_);
- visitor->Trace(wrapper_);
- }
-
- void Verify() {
- EXPECT_EQ(32, wrapper_->Value());
- EXPECT_EQ(0, trace_counter_->TraceCount());
- EXPECT_EQ(0, trace_called_);
- }
-
- private:
- Member<TraceCounter> trace_counter_;
- Member<IntWrapper> wrapper_;
-};
-
-int ClassWithGarbageCollectingMixinConstructor::trace_called_ = 0;
-
-// Regression test for out of bounds call through vtable.
-// Passes if it doesn't crash.
-TEST(HeapTest, GarbageCollectionDuringMixinConstruction) {
- ClassWithGarbageCollectingMixinConstructor::trace_called_ = 0;
- ClassWithGarbageCollectingMixinConstructor* a =
- MakeGarbageCollected<ClassWithGarbageCollectingMixinConstructor>();
- a->Verify();
-}
-
template <typename T>
class TraceIfNeededTester
: public GarbageCollectedFinalized<TraceIfNeededTester<T>> {
@@ -5928,11 +5875,7 @@
USING_GARBAGE_COLLECTED_MIXIN(TestMixinAllocationA);
public:
- TestMixinAllocationA() {
- // Completely wrong in general, but test only
- // runs this constructor while constructing another mixin.
- DCHECK(ThreadState::Current()->IsGCForbidden());
- }
+ TestMixinAllocationA() = default;
void Trace(blink::Visitor* visitor) override {}
};
@@ -5942,14 +5885,8 @@
public:
TestMixinAllocationB()
- : a_(MakeGarbageCollected<TestMixinAllocationA>()) // Construct object
- // during a mixin
- // construction.
- {
- // Completely wrong in general, but test only
- // runs this constructor while constructing another mixin.
- DCHECK(ThreadState::Current()->IsGCForbidden());
- }
+ // Construct object during a mixin construction.
+ : a_(MakeGarbageCollected<TestMixinAllocationA>()) {}
void Trace(blink::Visitor* visitor) override {
visitor->Trace(a_);
@@ -5994,47 +5931,6 @@
}
};
-class TestMixinAllocatingObject final
- : public TestMixinAllocationB,
- public ObjectWithLargeAmountsOfAllocationInConstructor {
- USING_GARBAGE_COLLECTED_MIXIN(TestMixinAllocatingObject);
-
- public:
- static TestMixinAllocatingObject* Create(ClassWithMember* member) {
- return MakeGarbageCollected<TestMixinAllocatingObject>(member);
- }
-
- TestMixinAllocatingObject(ClassWithMember* member)
- : ObjectWithLargeAmountsOfAllocationInConstructor(600, member),
- trace_counter_(TraceCounter::Create()) {
- DCHECK(!ThreadState::Current()->IsGCForbidden());
- ConservativelyCollectGarbage();
- EXPECT_GT(member->TraceCount(), 0);
- EXPECT_GT(TraceCount(), 0);
- }
-
- void Trace(blink::Visitor* visitor) override {
- visitor->Trace(trace_counter_);
- TestMixinAllocationB::Trace(visitor);
- }
-
- int TraceCount() const { return trace_counter_->TraceCount(); }
-
- private:
- Member<TraceCounter> trace_counter_;
-};
-
-TEST(HeapTest, MixinConstructionNoGC) {
- ClearOutOldGarbage();
- Persistent<ClassWithMember> object = ClassWithMember::Create();
- EXPECT_EQ(0, object->TraceCount());
- TestMixinAllocatingObject* mixin =
- TestMixinAllocatingObject::Create(object.Get());
- EXPECT_TRUE(mixin);
- EXPECT_GT(object->TraceCount(), 0);
- EXPECT_GT(mixin->TraceCount(), 0);
-}
-
class WeakPersistentHolder final {
public:
explicit WeakPersistentHolder(IntWrapper* object) : object_(object) {}
@@ -6420,85 +6316,25 @@
vector.ShrinkToFit();
}
-namespace {
-
-class MixinCheckingConstructionScope : public GarbageCollectedMixin {
- public:
- MixinCheckingConstructionScope() {
- // Oilpan treats mixin construction as forbidden scopes for garbage
- // collection.
- CHECK(ThreadState::Current()->IsMixinInConstruction());
- }
-};
-
-class UsingMixinCheckingConstructionScope
- : public GarbageCollected<UsingMixinCheckingConstructionScope>,
- public MixinCheckingConstructionScope {
- USING_GARBAGE_COLLECTED_MIXIN(UsingMixinCheckingConstructionScope);
-};
-
-} // namespace
-
-TEST(HeapTest, NoConservativeGCDuringMixinConstruction) {
- // Regression test: https://crbug.com/904546
- MakeGarbageCollected<UsingMixinCheckingConstructionScope>();
-}
-
-namespace {
-
-class ObjectCheckingForInConstruction
- : public GarbageCollected<ObjectCheckingForInConstruction> {
- public:
- ObjectCheckingForInConstruction() {
- CHECK(HeapObjectHeader::FromPayload(this)->IsInConstruction());
- }
-
- virtual void Trace(Visitor* v) { v->Trace(foo_); }
-
- private:
- Member<IntWrapper> foo_;
-};
-
-class MixinCheckingInConstruction : public GarbageCollectedMixin {
- public:
- MixinCheckingInConstruction() {
- BasePage* const page = PageFromObject(reinterpret_cast<Address>(this));
- HeapObjectHeader* const header =
- static_cast<NormalPage*>(page)->FindHeaderFromAddress(
- reinterpret_cast<Address>(
- const_cast<MixinCheckingInConstruction*>(this)));
- CHECK(header->IsInConstruction());
- }
-
- void Trace(Visitor* v) override { v->Trace(bar_); }
-
- private:
- Member<IntWrapper> bar_;
-};
-
-class MixinAppCheckingInConstruction
- : public GarbageCollected<MixinAppCheckingInConstruction>,
- public MixinCheckingInConstruction {
- USING_GARBAGE_COLLECTED_MIXIN(MixinAppCheckingInConstruction)
- public:
- MixinAppCheckingInConstruction() {
- CHECK(HeapObjectHeader::FromPayload(this)->IsInConstruction());
- }
-
- void Trace(Visitor* v) override { v->Trace(foo_); }
-
- private:
- Member<IntWrapper> foo_;
-};
-
-} // namespace
-
TEST(HeapTest, GarbageCollectedInConstruction) {
- MakeGarbageCollected<ObjectCheckingForInConstruction>();
+ using O = ObjectWithCallbackBeforeInitializer<IntWrapper>;
+ MakeGarbageCollected<O>(base::BindOnce([](O* thiz) {
+ CHECK(HeapObjectHeader::FromPayload(thiz)->IsInConstruction());
+ }));
}
TEST(HeapTest, GarbageCollectedMixinInConstruction) {
- MakeGarbageCollected<MixinAppCheckingInConstruction>();
+ using O = ObjectWithMixinWithCallbackBeforeInitializer<IntWrapper>;
+ MakeGarbageCollected<O>(base::BindOnce([](O::Mixin* thiz) {
+ CHECK(HeapObjectHeader::FromPayload(static_cast<O*>(thiz))
+ ->IsInConstruction());
+ }));
+}
+
+TEST(HeapTest, GarbageCollectedMixinIsAliveDuringConstruction) {
+ using O = ObjectWithMixinWithCallbackBeforeInitializer<IntWrapper>;
+ MakeGarbageCollected<O>(base::BindOnce(
+ [](O::Mixin* thiz) { CHECK(ThreadHeap::IsHeapObjectAlive(thiz)); }));
}
} // namespace blink
diff --git a/third_party/blink/renderer/platform/heap/heap_test_utilities.h b/third_party/blink/renderer/platform/heap/heap_test_utilities.h
index 97778731..0e05f426 100644
--- a/third_party/blink/renderer/platform/heap/heap_test_utilities.h
+++ b/third_party/blink/renderer/platform/heap/heap_test_utilities.h
@@ -7,7 +7,12 @@
#ifndef THIRD_PARTY_BLINK_RENDERER_PLATFORM_HEAP_HEAP_TEST_UTILITIES_H_
#define THIRD_PARTY_BLINK_RENDERER_PLATFORM_HEAP_HEAP_TEST_UTILITIES_H_
+#include "base/callback.h"
#include "third_party/blink/renderer/platform/heap/blink_gc.h"
+#include "third_party/blink/renderer/platform/heap/garbage_collected.h"
+#include "third_party/blink/renderer/platform/heap/heap.h"
+#include "third_party/blink/renderer/platform/heap/trace_traits.h"
+#include "third_party/blink/renderer/platform/heap/visitor.h"
namespace blink {
@@ -16,6 +21,89 @@
BlinkGC::SweepingType sweeping_type = BlinkGC::kEagerSweeping);
void ClearOutOldGarbage();
+template <typename T>
+class ObjectWithCallbackBeforeInitializer : public GarbageCollected<T> {
+ public:
+ ObjectWithCallbackBeforeInitializer(
+ base::OnceCallback<void(ObjectWithCallbackBeforeInitializer<T>*)>&& cb,
+ T* value)
+ : bool_(ExecuteCallbackReturnTrue(this, std::move(cb))), value_(value) {}
+
+ ObjectWithCallbackBeforeInitializer(
+ base::OnceCallback<void(ObjectWithCallbackBeforeInitializer<T>*)>&& cb)
+ : bool_(ExecuteCallbackReturnTrue(this, std::move(cb))) {}
+
+ virtual void Trace(Visitor* visitor) { visitor->Trace(value_); }
+
+ T* value() const { return value_.Get(); }
+
+ private:
+ static bool ExecuteCallbackReturnTrue(
+ ObjectWithCallbackBeforeInitializer* thiz,
+ base::OnceCallback<void(ObjectWithCallbackBeforeInitializer<T>*)>&& cb) {
+ std::move(cb).Run(thiz);
+ return true;
+ }
+
+ bool bool_;
+ Member<T> value_;
+};
+
+template <typename T>
+class MixinWithCallbackBeforeInitializer : public GarbageCollectedMixin {
+ public:
+ MixinWithCallbackBeforeInitializer(
+ base::OnceCallback<void(MixinWithCallbackBeforeInitializer<T>*)>&& cb,
+ T* value)
+ : bool_(ExecuteCallbackReturnTrue(this, std::move(cb))), value_(value) {}
+
+ MixinWithCallbackBeforeInitializer(
+ base::OnceCallback<void(MixinWithCallbackBeforeInitializer<T>*)>&& cb)
+ : bool_(ExecuteCallbackReturnTrue(this, std::move(cb))) {}
+
+ void Trace(Visitor* visitor) override { visitor->Trace(value_); }
+
+ T* value() const { return value_.Get(); }
+
+ private:
+ static bool ExecuteCallbackReturnTrue(
+ MixinWithCallbackBeforeInitializer* thiz,
+ base::OnceCallback<void(MixinWithCallbackBeforeInitializer<T>*)>&& cb) {
+ std::move(cb).Run(thiz);
+ return true;
+ }
+
+ bool bool_;
+ Member<T> value_;
+};
+
+class BoolMixin {
+ protected:
+ bool bool_ = false;
+};
+
+template <typename T>
+class ObjectWithMixinWithCallbackBeforeInitializer
+ : public GarbageCollected<ObjectWithMixinWithCallbackBeforeInitializer<T>>,
+ public BoolMixin,
+ public MixinWithCallbackBeforeInitializer<T> {
+ USING_GARBAGE_COLLECTED_MIXIN(ObjectWithMixinWithCallbackBeforeInitializer);
+
+ public:
+ using Mixin = MixinWithCallbackBeforeInitializer<T>;
+
+ ObjectWithMixinWithCallbackBeforeInitializer(
+ base::OnceCallback<void(Mixin*)>&& cb,
+ T* value)
+ : Mixin(std::move(cb), value) {}
+
+ ObjectWithMixinWithCallbackBeforeInitializer(
+ base::OnceCallback<void(Mixin*)>&& cb)
+ : Mixin(std::move(cb)) {}
+
+ void Trace(Visitor* visitor) override { Mixin::Trace(visitor); }
+};
+
} // namespace blink
#endif // THIRD_PARTY_BLINK_RENDERER_PLATFORM_HEAP_HEAP_TEST_UTILITIES_H_
diff --git a/third_party/blink/renderer/platform/heap/incremental_marking_test.cc b/third_party/blink/renderer/platform/heap/incremental_marking_test.cc
index 69587d14..b359297 100644
--- a/third_party/blink/renderer/platform/heap/incremental_marking_test.cc
+++ b/third_party/blink/renderer/platform/heap/incremental_marking_test.cc
@@ -1563,28 +1563,30 @@
thread_state_->IncrementalMarkingStart(BlinkGC::GCReason::kTesting);
}
- bool SingleStep() {
+ bool SingleStep(BlinkGC::StackState stack_state =
+ BlinkGC::StackState::kNoHeapPointersOnStack) {
CHECK(thread_state_->IsIncrementalMarking());
if (thread_state_->GetGCState() ==
ThreadState::kIncrementalMarkingStepScheduled) {
- thread_state_->RunScheduledGC(BlinkGC::kNoHeapPointersOnStack);
+ thread_state_->IncrementalMarkingStep(stack_state);
return true;
}
return false;
}
- void FinishSteps() {
+ void FinishSteps(BlinkGC::StackState stack_state =
+ BlinkGC::StackState::kNoHeapPointersOnStack) {
CHECK(thread_state_->IsIncrementalMarking());
- while (SingleStep()) {
+ while (SingleStep(stack_state)) {
}
}
void FinishGC() {
CHECK(thread_state_->IsIncrementalMarking());
- FinishSteps();
+ FinishSteps(BlinkGC::StackState::kNoHeapPointersOnStack);
CHECK_EQ(ThreadState::kIncrementalMarkingFinalizeScheduled,
thread_state_->GetGCState());
- thread_state_->RunScheduledGC(BlinkGC::kNoHeapPointersOnStack);
+ thread_state_->RunScheduledGC(BlinkGC::StackState::kNoHeapPointersOnStack);
CHECK(!thread_state_->IsIncrementalMarking());
thread_state_->CompleteSweep();
}
@@ -1801,6 +1803,80 @@
ConservativelyCollectGarbage();
}
+namespace {
+
+template <typename T>
+class ObjectHolder : public GarbageCollected<ObjectHolder<T>> {
+ public:
+ ObjectHolder() = default;
+
+ virtual void Trace(Visitor* visitor) { visitor->Trace(holder_); }
+
+ void set_value(T* value) { holder_ = value; }
+ T* value() const { return holder_.Get(); }
+
+ private:
+ Member<T> holder_;
+};
+
+} // namespace
+
+TEST(IncrementalMarkingTest, StepDuringObjectConstruction) {
+ // Test ensures that objects in construction are delayed for processing to
+ // allow omitting write barriers on initializing stores.
+
+ using O = ObjectWithCallbackBeforeInitializer<Object>;
+ using Holder = ObjectHolder<O>;
+ Persistent<Holder> holder(MakeGarbageCollected<Holder>());
+ IncrementalMarkingTestDriver driver(ThreadState::Current());
+ driver.Start();
+ MakeGarbageCollected<O>(
+ base::BindOnce(
+ [](IncrementalMarkingTestDriver* driver, Holder* holder, O* thiz) {
+ // Publish not-fully-constructed object |thiz| by triggering write
+ // barrier for the object.
+ holder->set_value(thiz);
+ CHECK(HeapObjectHeader::FromPayload(holder->value())->IsValid());
+ // Finish call incremental steps.
+ driver->FinishSteps(BlinkGC::StackState::kHeapPointersOnStack);
+ },
+ &driver, holder.Get()),
+ MakeGarbageCollected<Object>());
+ driver.FinishGC();
+ CHECK(HeapObjectHeader::FromPayload(holder->value())->IsValid());
+ CHECK(HeapObjectHeader::FromPayload(holder->value()->value())->IsValid());
+ PreciselyCollectGarbage();
+}
+
+TEST(IncrementalMarkingTest, StepDuringMixinObjectConstruction) {
+ // Test ensures that mixin objects in construction are delayed for processing
+ // to allow omitting write barriers on initializing stores.
+
+ using Parent = ObjectWithMixinWithCallbackBeforeInitializer<Object>;
+ using Mixin = MixinWithCallbackBeforeInitializer<Object>;
+ using Holder = ObjectHolder<Mixin>;
+ Persistent<Holder> holder(MakeGarbageCollected<Holder>());
+ IncrementalMarkingTestDriver driver(ThreadState::Current());
+ driver.Start();
+ MakeGarbageCollected<Parent>(
+ base::BindOnce(
+ [](IncrementalMarkingTestDriver* driver, Holder* holder,
+ Mixin* thiz) {
+ // Publish not-fully-constructed object
+ // |thiz| by triggering write barrier for
+ // the object.
+ holder->set_value(thiz);
+ // Finish call incremental steps.
+ driver->FinishSteps(BlinkGC::StackState::kHeapPointersOnStack);
+ },
+ &driver, holder.Get()),
+ MakeGarbageCollected<Object>());
+ driver.FinishGC();
+ CHECK(holder->value()->GetHeapObjectHeader()->IsValid());
+ CHECK(HeapObjectHeader::FromPayload(holder->value()->value())->IsValid());
+ PreciselyCollectGarbage();
+}
+
} // namespace incremental_marking_test
} // namespace blink
diff --git a/third_party/blink/renderer/platform/heap/marking_visitor.cc b/third_party/blink/renderer/platform/heap/marking_visitor.cc
index 6efd4e027..b95eb73 100644
--- a/third_party/blink/renderer/platform/heap/marking_visitor.cc
+++ b/third_party/blink/renderer/platform/heap/marking_visitor.cc
@@ -64,11 +64,10 @@
if (!header || header->IsMarked())
return;
- // Simple case for fully constructed objects or those that have no vtable
- // which make dispatching to member fields trivial.
+ // Simple case for fully constructed objects.
const GCInfo* gc_info =
GCInfoTable::Get().GCInfoFromIndex(header->GcInfoIndex());
- if (!gc_info->HasVTable() || !header->IsInConstruction()) {
+ if (!header->IsInConstruction()) {
MarkHeader(header, gc_info->trace_);
return;
}
diff --git a/third_party/blink/renderer/platform/heap/marking_visitor.h b/third_party/blink/renderer/platform/heap/marking_visitor.h
index cbb41e8..a53f0bf 100644
--- a/third_party/blink/renderer/platform/heap/marking_visitor.h
+++ b/third_party/blink/renderer/platform/heap/marking_visitor.h
@@ -51,11 +51,14 @@
// Marking implementation.
- // Conservatively marks an object if pointed to by Address.
+ // Conservatively marks an object if pointed to by Address. The object may
+ // be in construction as the scan is conservative without relying on a
+ // Trace method.
void ConservativelyMarkAddress(BasePage*, Address);
// Marks an object dynamically using any address within its body and adds a
- // tracing callback for processing of the object.
+ // tracing callback for processing of the object. The object is not allowed
+ // to be in construction.
void DynamicallyMarkAddress(Address);
// Marks an object and adds a tracing callback for processing of the object.
@@ -97,8 +100,11 @@
// that lead to many recursions.
DCHECK(Heap().GetStackFrameDepth().IsAcceptableStackUse());
if (LIKELY(Heap().GetStackFrameDepth().IsSafeToRecurse())) {
- if (MarkHeaderNoTracing(
- HeapObjectHeader::FromPayload(desc.base_object_payload))) {
+ HeapObjectHeader* header =
+ HeapObjectHeader::FromPayload(desc.base_object_payload);
+ if (header->IsInConstruction()) {
+ not_fully_constructed_worklist_.Push(desc.base_object_payload);
+ } else if (MarkHeaderNoTracing(header)) {
desc.callback(this, desc.base_object_payload);
}
return;
@@ -199,7 +205,9 @@
DCHECK(header);
DCHECK(callback);
- if (MarkHeaderNoTracing(header)) {
+ if (header->IsInConstruction()) {
+ not_fully_constructed_worklist_.Push(header->Payload());
+ } else if (MarkHeaderNoTracing(header)) {
marking_worklist_.Push(
{reinterpret_cast<void*>(header->Payload()), callback});
}
diff --git a/third_party/blink/renderer/platform/heap/persistent.h b/third_party/blink/renderer/platform/heap/persistent.h
index 35ed1d2..69db982b 100644
--- a/third_party/blink/renderer/platform/heap/persistent.h
+++ b/third_party/blink/renderer/platform/heap/persistent.h
@@ -269,7 +269,7 @@
weaknessConfiguration, crossThreadnessConfiguration>;
Base* persistent = reinterpret_cast<Base*>(persistent_pointer);
T* object = persistent->Get();
- if (object && !ObjectAliveTrait<T>::IsHeapObjectAlive(object))
+ if (object && !ThreadHeap::IsHeapObjectAlive(object))
ClearWeakPersistent(persistent);
}
diff --git a/third_party/blink/renderer/platform/heap/thread_state.cc b/third_party/blink/renderer/platform/heap/thread_state.cc
index 1e7ffa8..a35433a 100644
--- a/third_party/blink/renderer/platform/heap/thread_state.cc
+++ b/third_party/blink/renderer/platform/heap/thread_state.cc
@@ -177,7 +177,6 @@
mixins_being_constructed_count_(0),
object_resurrection_forbidden_(false),
in_atomic_pause_(false),
- gc_mixin_marker_(nullptr),
gc_state_(kNoGCScheduled),
gc_phase_(GCPhase::kNone),
reason_for_scheduled_gc_(BlinkGC::GCReason::kMaxValue),
diff --git a/third_party/blink/renderer/platform/heap/thread_state.h b/third_party/blink/renderer/platform/heap/thread_state.h
index 7e7a50e6..00282d6 100644
--- a/third_party/blink/renderer/platform/heap/thread_state.h
+++ b/third_party/blink/renderer/platform/heap/thread_state.h
@@ -62,7 +62,6 @@
class IncrementalMarkingTestDriver;
} // namespace incremental_marking_test
-class GarbageCollectedMixinConstructorMarkerBase;
class MarkingVisitor;
class PersistentNode;
class PersistentRegion;
@@ -480,29 +479,6 @@
perform_cleanup_ = perform_cleanup;
}
- // By entering a gc-forbidden scope, conservative GCs will not
- // be allowed while handling an out-of-line allocation request.
- // Intended used when constructing subclasses of GC mixins, where
- // the object being constructed cannot be safely traced & marked
- // fully should a GC be allowed while its subclasses are being
- // constructed.
- void EnterGCForbiddenScopeIfNeeded(
- GarbageCollectedMixinConstructorMarkerBase* gc_mixin_marker) {
- DCHECK(CheckThread());
- if (!gc_mixin_marker_) {
- EnterMixinConstructionScope();
- gc_mixin_marker_ = gc_mixin_marker;
- }
- }
- void LeaveGCForbiddenScopeIfNeeded(
- GarbageCollectedMixinConstructorMarkerBase* gc_mixin_marker) {
- DCHECK(CheckThread());
- if (gc_mixin_marker_ == gc_mixin_marker) {
- LeaveMixinConstructionScope();
- gc_mixin_marker_ = nullptr;
- }
- }
-
void FreePersistentNode(PersistentRegion*, PersistentNode*);
using PersistentClearCallback = void (*)(void*);
@@ -707,8 +683,6 @@
TimeDelta next_incremental_marking_step_duration_;
TimeDelta previous_incremental_marking_time_left_;
- GarbageCollectedMixinConstructorMarkerBase* gc_mixin_marker_;
-
GCState gc_state_;
GCPhase gc_phase_;
BlinkGC::GCReason reason_for_scheduled_gc_;
