Skip to content

Commit 49954eb

Browse files
mlippautzCommit Bot
mlippautz
authored and
Commit Bot
committed
Revert "[api,heap] Remove deprecated Persistent APIs"
This reverts commit 1ebf5f7. Reason for revert: Breaks TSAN Original change's description: > [api,heap] Remove deprecated Persistent APIs > > Removes APIs: > - MarkIndependent > - IsIndependent > - MarkActive > - RegisterExternalReference > > All weak persistent handles are now treated as independent. Users of > traced handles should already use v8::EmbedderHeapTracer. > > Bug: chromium:923361 > Change-Id: Ic90a647fe2ce9db92197ad6560e4907290805592 > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1578459 > Commit-Queue: Michael Lippautz <[email protected]> > Reviewed-by: Ulan Degenbaev <[email protected]> > Cr-Commit-Position: refs/heads/master@{#60953} [email protected],[email protected] Change-Id: I8281daf30b67c1b71ef6e65d8f13a59230ba0334 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: chromium:923361 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1578900 Reviewed-by: Michael Lippautz <[email protected]> Commit-Queue: Michael Lippautz <[email protected]> Cr-Commit-Position: refs/heads/master@{#60954}
1 parent 1ebf5f7 commit 49954eb

11 files changed

Lines changed: 426 additions & 185 deletions

File tree

‎include/v8-internal.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -165,6 +165,8 @@ class Internals {
165165
static const int kNodeStateMask = 0x7;
166166
static const int kNodeStateIsWeakValue = 2;
167167
static const int kNodeStateIsPendingValue = 3;
168+
static const int kNodeIsIndependentShift = 3;
169+
static const int kNodeIsActiveShift = 4;
168170

169171
static const int kFirstNonstringType = 0x40;
170172
static const int kOddballType = 0x43;

‎include/v8-util.h‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -194,6 +194,14 @@ class PersistentValueMapBase {
194194
return SetReturnValueFromVal(&returnValue, Traits::Get(&impl_, key));
195195
}
196196

197+
/**
198+
* Call V8::RegisterExternallyReferencedObject with the map value for given
199+
* key.
200+
*/
201+
V8_DEPRECATED(
202+
"Used TracedGlobal and EmbedderHeapTracer::RegisterEmbedderReference",
203+
inline void RegisterExternallyReferencedObject(K& key));
204+
197205
/**
198206
* Return value for key and remove it from the map.
199207
*/
@@ -344,6 +352,16 @@ class PersistentValueMapBase {
344352
const char* label_;
345353
};
346354

355+
template <typename K, typename V, typename Traits>
356+
inline void
357+
PersistentValueMapBase<K, V, Traits>::RegisterExternallyReferencedObject(
358+
K& key) {
359+
assert(Contains(key));
360+
V8::RegisterExternallyReferencedObject(
361+
reinterpret_cast<internal::Address*>(FromVal(Traits::Get(&impl_, key))),
362+
reinterpret_cast<internal::Isolate*>(GetIsolate()));
363+
}
364+
347365
template <typename K, typename V, typename Traits>
348366
class PersistentValueMap : public PersistentValueMapBase<K, V, Traits> {
349367
public:

‎include/v8.h‎

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -545,6 +545,38 @@ template <class T> class PersistentBase {
545545
*/
546546
V8_INLINE void AnnotateStrongRetainer(const char* label);
547547

548+
/**
549+
* Allows the embedder to tell the v8 garbage collector that a certain object
550+
* is alive. Only allowed when the embedder is asked to trace its heap by
551+
* EmbedderHeapTracer.
552+
*/
553+
V8_DEPRECATED(
554+
"Used TracedGlobal and EmbedderHeapTracer::RegisterEmbedderReference",
555+
V8_INLINE void RegisterExternalReference(Isolate* isolate) const);
556+
557+
/**
558+
* Marks the reference to this object independent. Garbage collector is free
559+
* to ignore any object groups containing this object. Weak callback for an
560+
* independent handle should not assume that it will be preceded by a global
561+
* GC prologue callback or followed by a global GC epilogue callback.
562+
*/
563+
V8_DEPRECATED(
564+
"Weak objects are always considered independent. "
565+
"Use TracedGlobal when trying to use EmbedderHeapTracer. "
566+
"Use a strong handle when trying to keep an object alive.",
567+
V8_INLINE void MarkIndependent());
568+
569+
/**
570+
* Marks the reference to this object as active. The scavenge garbage
571+
* collection should not reclaim the objects marked as active, even if the
572+
* object held by the handle is otherwise unreachable.
573+
*
574+
* This bit is cleared after the each garbage collection pass.
575+
*/
576+
V8_DEPRECATED("Use TracedGlobal.", V8_INLINE void MarkActive());
577+
578+
V8_DEPRECATED("See MarkIndependent.", V8_INLINE bool IsIndependent() const);
579+
548580
/** Returns true if the handle's reference is weak. */
549581
V8_INLINE bool IsWeak() const;
550582

@@ -8807,6 +8839,9 @@ class V8_EXPORT V8 {
88078839
const char* label);
88088840
static Value* Eternalize(Isolate* isolate, Value* handle);
88098841

8842+
static void RegisterExternallyReferencedObject(internal::Address* location,
8843+
internal::Isolate* isolate);
8844+
88108845
template <class K, class V, class T>
88118846
friend class PersistentValueMapBase;
88128847

@@ -9753,6 +9788,14 @@ void Persistent<T, M>::Copy(const Persistent<S, M2>& that) {
97539788
M::Copy(that, this);
97549789
}
97559790

9791+
template <class T>
9792+
bool PersistentBase<T>::IsIndependent() const {
9793+
typedef internal::Internals I;
9794+
if (this->IsEmpty()) return false;
9795+
return I::GetNodeFlag(reinterpret_cast<internal::Address*>(this->val_),
9796+
I::kNodeIsIndependentShift);
9797+
}
9798+
97569799
template <class T>
97579800
bool PersistentBase<T>::IsWeak() const {
97589801
typedef internal::Internals I;
@@ -9819,6 +9862,31 @@ void PersistentBase<T>::AnnotateStrongRetainer(const char* label) {
98199862
label);
98209863
}
98219864

9865+
template <class T>
9866+
void PersistentBase<T>::RegisterExternalReference(Isolate* isolate) const {
9867+
if (IsEmpty()) return;
9868+
V8::RegisterExternallyReferencedObject(
9869+
reinterpret_cast<internal::Address*>(this->val_),
9870+
reinterpret_cast<internal::Isolate*>(isolate));
9871+
}
9872+
9873+
template <class T>
9874+
void PersistentBase<T>::MarkIndependent() {
9875+
typedef internal::Internals I;
9876+
if (this->IsEmpty()) return;
9877+
I::UpdateNodeFlag(reinterpret_cast<internal::Address*>(this->val_), true,
9878+
I::kNodeIsIndependentShift);
9879+
}
9880+
9881+
template <class T>
9882+
void PersistentBase<T>::MarkActive() {
9883+
typedef internal::Internals I;
9884+
if (this->IsEmpty()) return;
9885+
I::UpdateNodeFlag(reinterpret_cast<internal::Address*>(this->val_), true,
9886+
I::kNodeIsActiveShift);
9887+
}
9888+
9889+
98229890
template <class T>
98239891
void PersistentBase<T>::SetWrapperClassId(uint16_t class_id) {
98249892
typedef internal::Internals I;

‎src/api.cc‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1037,6 +1037,11 @@ void V8::MoveTracedGlobalReference(internal::Address** from,
10371037
i::GlobalHandles::MoveTracedGlobal(from, to);
10381038
}
10391039

1040+
void V8::RegisterExternallyReferencedObject(i::Address* location,
1041+
i::Isolate* isolate) {
1042+
isolate->heap()->RegisterExternallyReferencedObject(location);
1043+
}
1044+
10401045
void V8::MakeWeak(i::Address* location, void* parameter,
10411046
WeakCallbackInfo<void>::Callback weak_callback,
10421047
WeakCallbackType type) {

‎src/global-handles.cc‎

Lines changed: 44 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -379,6 +379,10 @@ class GlobalHandles::Node final : public NodeBase<GlobalHandles::Node> {
379379
Internals::kNodeStateMask);
380380
STATIC_ASSERT(WEAK == Internals::kNodeStateIsWeakValue);
381381
STATIC_ASSERT(PENDING == Internals::kNodeStateIsPendingValue);
382+
STATIC_ASSERT(static_cast<int>(IsIndependent::kShift) ==
383+
Internals::kNodeIsIndependentShift);
384+
STATIC_ASSERT(static_cast<int>(IsActive::kShift) ==
385+
Internals::kNodeIsActiveShift);
382386
set_in_young_list(false);
383387
}
384388

@@ -402,6 +406,16 @@ class GlobalHandles::Node final : public NodeBase<GlobalHandles::Node> {
402406
flags_ = NodeState::update(flags_, state);
403407
}
404408

409+
bool is_independent() { return IsIndependent::decode(flags_); }
410+
void set_independent(bool v) { flags_ = IsIndependent::update(flags_, v); }
411+
412+
bool is_active() {
413+
return IsActive::decode(flags_);
414+
}
415+
void set_active(bool v) {
416+
flags_ = IsActive::update(flags_, v);
417+
}
418+
405419
bool is_in_young_list() const { return IsInYoungList::decode(flags_); }
406420
void set_in_young_list(bool v) { flags_ = IsInYoungList::update(flags_, v); }
407421

@@ -548,6 +562,7 @@ class GlobalHandles::Node final : public NodeBase<GlobalHandles::Node> {
548562
// This method invokes a finalizer. Updating the method name would require
549563
// adjusting CFI blacklist as weak_callback_ is invoked on the wrong type.
550564
CHECK(IsPendingFinalizer());
565+
CHECK(!is_active());
551566
set_state(NEAR_DEATH);
552567
// Check that we are not passing a finalized external string to
553568
// the callback.
@@ -578,17 +593,24 @@ class GlobalHandles::Node final : public NodeBase<GlobalHandles::Node> {
578593
private:
579594
// Fields that are not used for managing node memory.
580595
void ClearImplFields() {
596+
set_independent(false);
597+
set_active(false);
581598
weak_callback_ = nullptr;
582599
}
583600

584601
void CheckImplFieldsAreCleared() {
602+
DCHECK(!is_independent());
603+
DCHECK(!is_active());
585604
DCHECK_EQ(nullptr, weak_callback_);
586605
}
587606

588607
// This stores three flags (independent, partially_dependent and
589608
// in_young_list) and a State.
590609
class NodeState : public BitField8<State, 0, 3> {};
591-
class IsInYoungList : public BitField8<bool, NodeState::kNext, 1> {};
610+
class IsIndependent : public BitField8<bool, NodeState::kNext, 1> {};
611+
// The following two fields are mutually exclusive
612+
class IsActive : public BitField8<bool, IsIndependent::kNext, 1> {};
613+
class IsInYoungList : public BitField8<bool, IsActive::kNext, 1> {};
592614
class NodeWeaknessType
593615
: public BitField8<WeaknessType, IsInYoungList::kNext, 2> {};
594616

@@ -851,6 +873,12 @@ void GlobalHandles::IterateWeakRootsIdentifyFinalizers(
851873

852874
void GlobalHandles::IdentifyWeakUnmodifiedObjects(
853875
WeakSlotCallback is_unmodified) {
876+
for (Node* node : young_nodes_) {
877+
if (node->IsWeak() && !is_unmodified(node->location())) {
878+
node->set_active(true);
879+
}
880+
}
881+
854882
LocalEmbedderHeapTracer* const tracer =
855883
isolate()->heap()->local_embedder_heap_tracer();
856884
for (TracedNode* node : traced_young_nodes_) {
@@ -867,7 +895,9 @@ void GlobalHandles::IdentifyWeakUnmodifiedObjects(
867895

868896
void GlobalHandles::IterateYoungStrongAndDependentRoots(RootVisitor* v) {
869897
for (Node* node : young_nodes_) {
870-
if (node->IsStrongRetainer()) {
898+
if (node->IsStrongRetainer() ||
899+
(node->IsWeakRetainer() && !node->is_independent() &&
900+
node->is_active())) {
871901
v->VisitRootPointer(Root::kGlobalHandles, node->label(),
872902
node->location());
873903
}
@@ -883,7 +913,8 @@ void GlobalHandles::MarkYoungWeakUnmodifiedObjectsPending(
883913
WeakSlotCallbackWithHeap is_dead) {
884914
for (Node* node : young_nodes_) {
885915
DCHECK(node->is_in_young_list());
886-
if (node->IsWeak() && is_dead(isolate_->heap(), node->location())) {
916+
if ((node->is_independent() || !node->is_active()) && node->IsWeak() &&
917+
is_dead(isolate_->heap(), node->location())) {
887918
if (!node->IsPhantomCallback() && !node->IsPhantomResetHandle()) {
888919
node->MarkPending();
889920
}
@@ -895,7 +926,8 @@ void GlobalHandles::IterateYoungWeakUnmodifiedRootsForFinalizers(
895926
RootVisitor* v) {
896927
for (Node* node : young_nodes_) {
897928
DCHECK(node->is_in_young_list());
898-
if (node->IsWeakRetainer() && (node->state() == Node::PENDING)) {
929+
if ((node->is_independent() || !node->is_active()) &&
930+
node->IsWeakRetainer() && (node->state() == Node::PENDING)) {
899931
DCHECK(!node->IsPhantomCallback());
900932
DCHECK(!node->IsPhantomResetHandle());
901933
// Finalizers need to survive.
@@ -909,7 +941,8 @@ void GlobalHandles::IterateYoungWeakUnmodifiedRootsForPhantomHandles(
909941
RootVisitor* v, WeakSlotCallbackWithHeap should_reset_handle) {
910942
for (Node* node : young_nodes_) {
911943
DCHECK(node->is_in_young_list());
912-
if (node->IsWeakRetainer() && (node->state() != Node::PENDING)) {
944+
if ((node->is_independent() || !node->is_active()) &&
945+
node->IsWeakRetainer() && (node->state() != Node::PENDING)) {
913946
if (should_reset_handle(isolate_->heap(), node->location())) {
914947
DCHECK(node->IsPhantomResetHandle() || node->IsPhantomCallback());
915948
if (node->IsPhantomResetHandle()) {
@@ -975,6 +1008,9 @@ size_t GlobalHandles::PostScavengeProcessing(unsigned post_processing_count) {
9751008
// Filter free nodes.
9761009
if (!node->IsRetainer()) continue;
9771010

1011+
// Reset active state for all affected nodes.
1012+
node->set_active(false);
1013+
9781014
if (node->IsPending()) {
9791015
DCHECK(node->has_callback());
9801016
DCHECK(node->IsPendingFinalizer());
@@ -993,6 +1029,9 @@ size_t GlobalHandles::PostMarkSweepProcessing(unsigned post_processing_count) {
9931029
// Filter free nodes.
9941030
if (!node->IsRetainer()) continue;
9951031

1032+
// Reset active state for all affected nodes.
1033+
node->set_active(false);
1034+
9961035
if (node->IsPending()) {
9971036
DCHECK(node->has_callback());
9981037
DCHECK(node->IsPendingFinalizer());

‎src/profiler/sampling-heap-profiler.cc‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,16 @@ void SamplingHeapProfiler::SampleObject(Address soon_object, size_t size) {
9797
base::make_unique<Sample>(size, node, loc, this, next_sample_id());
9898
sample->global.SetWeak(sample.get(), OnWeakCallback,
9999
WeakCallbackType::kParameter);
100+
#if __clang__
101+
#pragma clang diagnostic push
102+
#pragma clang diagnostic ignored "-Wdeprecated"
103+
#endif
104+
// MarkIndependent is marked deprecated but we still rely on it here
105+
// temporarily.
106+
sample->global.MarkIndependent();
107+
#if __clang__
108+
#pragma clang diagnostic pop
109+
#endif
100110
samples_.emplace(sample.get(), std::move(sample));
101111
}
102112

‎test/cctest/heap/heap-utils.h‎

Lines changed: 0 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -12,22 +12,6 @@ namespace v8 {
1212
namespace internal {
1313
namespace heap {
1414

15-
class TemporaryEmbedderHeapTracerScope {
16-
public:
17-
TemporaryEmbedderHeapTracerScope(v8::Isolate* isolate,
18-
v8::EmbedderHeapTracer* tracer)
19-
: isolate_(isolate) {
20-
isolate_->SetEmbedderHeapTracer(tracer);
21-
}
22-
23-
~TemporaryEmbedderHeapTracerScope() {
24-
isolate_->SetEmbedderHeapTracer(nullptr);
25-
}
26-
27-
private:
28-
v8::Isolate* const isolate_;
29-
};
30-
3115
void SealCurrentObjects(Heap* heap);
3216

3317
int FixedArrayLenFromSize(int size);

0 commit comments

Comments
 (0)