Skip to content

Commit f09d67c

Browse files
mi-acmi-ac
committed
Merge: Revert "[heap] Avoid double-read of handles in garbage collector" (Version 14.5.201.1)
(Cherry-pick from commit 4b61a58) Change-Id: I3a4ff65554b669261ff3b51d39d218ee25a73edc Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/7451458 Owners-Override: Michael Achenbach <[email protected]> Bot-Commit: Rubber Stamper <[email protected]> Cr-Commit-Position: refs/heads/14.5.201@{#2} Cr-Branched-From: 3f00643-refs/heads/main@{#104623}
1 parent 8fcd1e9 commit f09d67c

12 files changed

+151
-120
lines changed

‎include/v8-version.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@
1111
#define V8_MAJOR_VERSION 14
1212
#define V8_MINOR_VERSION 5
1313
#define V8_BUILD_NUMBER 201
14-
#define V8_PATCH_LEVEL 0
14+
#define V8_PATCH_LEVEL 1
1515

1616
// Use 1 for candidates and 0 otherwise.
1717
// (Boolean macro values are not supported by all preprocessors.)

‎src/heap/array-buffer-sweeper.cc‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -485,7 +485,6 @@ bool ArrayBufferSweeper::SweepingState::SweepingJob::SweepListFull(
485485
size_t swept_extensions = 0;
486486

487487
while (current) {
488-
current->InitializationBarrier();
489488
DCHECK_EQ(list.age_, current->age());
490489
if ((swept_extensions++ & (kYieldCheckInterval - 1)) == 0) {
491490
if (delegate->ShouldYield()) break;
@@ -530,7 +529,6 @@ bool ArrayBufferSweeper::SweepingState::SweepingJob::SweepYoung(
530529
size_t swept_extensions = 0;
531530

532531
while (current) {
533-
current->InitializationBarrier();
534532
DCHECK_EQ(ArrayBufferExtension::Age::kYoung, current->age());
535533
if ((swept_extensions++ & (kYieldCheckInterval - 1)) == 0) {
536534
if (delegate->ShouldYield()) break;

‎src/heap/marking-visitor-inl.h‎

Lines changed: 37 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -236,51 +236,35 @@ void MarkingVisitorBase<ConcreteVisitor>::VisitExternalPointer(
236236
Tagged<HeapObject> host, ExternalPointerSlot slot) {
237237
#ifdef V8_COMPRESS_POINTERS
238238
DCHECK(!slot.tag_range().IsEmpty());
239-
if (!slot.HasExternalPointerHandle()) {
240-
return;
241-
}
242-
const ExternalPointerHandle handle = slot.Relaxed_LoadHandle();
243-
ExternalPointerTable* table;
244-
ExternalPointerTable::Space* space;
245-
if (IsSharedExternalPointerType(slot.tag_range())) {
246-
table = shared_external_pointer_table_;
247-
space = shared_external_pointer_space_;
248-
} else {
249-
table = external_pointer_table_;
250-
if constexpr (v8_flags.sticky_mark_bits.value()) {
251-
// Everything is considered old during major GC.
252-
DCHECK(!HeapLayout::InYoungGeneration(host));
253-
if (handle == kNullExternalPointerHandle) return;
254-
// The object may either be in young or old EPT.
255-
if (table->Contains(heap_->young_external_pointer_space(), handle)) {
256-
space = heap_->young_external_pointer_space();
239+
if (slot.HasExternalPointerHandle()) {
240+
ExternalPointerHandle handle = slot.Relaxed_LoadHandle();
241+
ExternalPointerTable* table;
242+
ExternalPointerTable::Space* space;
243+
if (IsSharedExternalPointerType(slot.tag_range())) {
244+
table = shared_external_pointer_table_;
245+
space = shared_external_pointer_space_;
246+
} else {
247+
table = external_pointer_table_;
248+
if (v8_flags.sticky_mark_bits) {
249+
// Everything is considered old during major GC.
250+
DCHECK(!HeapLayout::InYoungGeneration(host));
251+
if (handle == kNullExternalPointerHandle) return;
252+
// The object may either be in young or old EPT.
253+
if (table->Contains(heap_->young_external_pointer_space(), handle)) {
254+
space = heap_->young_external_pointer_space();
255+
} else {
256+
DCHECK(table->Contains(heap_->old_external_pointer_space(), handle));
257+
space = heap_->old_external_pointer_space();
258+
}
257259
} else {
258-
DCHECK(table->Contains(heap_->old_external_pointer_space(), handle));
259-
space = heap_->old_external_pointer_space();
260+
space = HeapLayout::InYoungGeneration(host)
261+
? heap_->young_external_pointer_space()
262+
: heap_->old_external_pointer_space();
260263
}
261-
} else {
262-
space = HeapLayout::InYoungGeneration(host)
263-
? heap_->young_external_pointer_space()
264-
: heap_->old_external_pointer_space();
265264
}
265+
table->Mark(space, handle, slot.address());
266266
}
267-
table->Mark(space, handle, slot.address());
268-
if (slot.tag_range() != kArrayBufferExtensionTag) {
269-
return;
270-
}
271-
Address maybe_extension = table->Get(handle, slot.tag_range());
272-
#else // !V8_COMPRESS_POINTERS
273-
if (slot.tag_range() != kArrayBufferExtensionTag) {
274-
return;
275-
}
276-
Address maybe_extension = slot.load(heap_->isolate());
277-
#endif // !V8_COMPRESS_POINTERS
278-
if (maybe_extension) {
279-
ArrayBufferExtension* extension =
280-
reinterpret_cast<ArrayBufferExtension*>(maybe_extension);
281-
extension->InitializationBarrier();
282-
extension->Mark();
283-
}
267+
#endif // V8_COMPRESS_POINTERS
284268
}
285269

286270
template <typename ConcreteVisitor>
@@ -676,6 +660,18 @@ size_t MarkingVisitorBase<ConcreteVisitor>::VisitFixedArray(
676660
: Base::VisitFixedArray(map, object, maybe_object_size);
677661
}
678662

663+
// ===========================================================================
664+
// Custom visitation =========================================================
665+
// ===========================================================================
666+
667+
template <typename ConcreteVisitor>
668+
size_t MarkingVisitorBase<ConcreteVisitor>::VisitJSArrayBuffer(
669+
Tagged<Map> map, Tagged<JSArrayBuffer> object,
670+
MaybeObjectSize maybe_object_size) {
671+
object->MarkExtension();
672+
return Base::VisitJSArrayBuffer(map, object, maybe_object_size);
673+
}
674+
679675
// ===========================================================================
680676
// Weak JavaScript objects ===================================================
681677
// ===========================================================================

‎src/heap/marking-visitor.h‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -91,6 +91,9 @@ class MarkingVisitorBase : public ConcurrentHeapVisitor<ConcreteVisitor> {
9191
MaybeObjectSize);
9292
V8_INLINE size_t VisitFixedArray(Tagged<Map> map, Tagged<FixedArray> object,
9393
MaybeObjectSize);
94+
V8_INLINE size_t VisitJSArrayBuffer(Tagged<Map> map,
95+
Tagged<JSArrayBuffer> object,
96+
MaybeObjectSize);
9497
V8_INLINE size_t VisitJSFunction(Tagged<Map> map, Tagged<JSFunction> object,
9598
MaybeObjectSize);
9699
V8_INLINE size_t VisitJSWeakRef(Tagged<Map> map, Tagged<JSWeakRef> object,

‎src/heap/scavenger.cc‎

Lines changed: 13 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -519,11 +519,6 @@ class ScavengerObjectVisitorBase : public NewSpaceVisitor<ConcreteVisitor> {
519519
ExternalPointerHandle handle = slot.Relaxed_LoadHandle();
520520
Heap* heap = scavenger_->heap();
521521
ExternalPointerTable& table = heap->isolate()->external_pointer_table();
522-
ArrayBufferExtension* array_buffer_extension =
523-
slot.tag_range() == kArrayBufferExtensionTag
524-
? reinterpret_cast<ArrayBufferExtension*>(
525-
table.Get(handle, kArrayBufferExtensionTag))
526-
: nullptr;
527522
if constexpr (kExpectedObjectAge == ObjectAge::kYoung) {
528523
// For survivor objects, mark their EPT entries when they are
529524
// copied. Scavenger then sweeps the young EPT space at the end of
@@ -539,21 +534,20 @@ class ScavengerObjectVisitorBase : public NewSpaceVisitor<ConcreteVisitor> {
539534
heap->old_external_pointer_space(), handle, slot.address(),
540535
ExternalPointerTable::EvacuateMarkMode::kTransferMark);
541536
}
542-
#else // !V8_COMPRESS_POINTERS
543-
ArrayBufferExtension* array_buffer_extension =
544-
slot.tag_range() == kArrayBufferExtensionTag
545-
? reinterpret_cast<ArrayBufferExtension*>(
546-
slot.load(scavenger_->heap()->isolate()))
547-
: nullptr;
548-
#endif // !V8_COMPRESS_POINTERS
549-
if (array_buffer_extension) {
550-
array_buffer_extension->InitializationBarrier();
551-
if constexpr (kExpectedObjectAge == ObjectAge::kYoung) {
552-
array_buffer_extension->YoungMark();
553-
} else {
554-
array_buffer_extension->YoungMarkPromoted();
555-
}
537+
#endif // V8_COMPRESS_POINTERS
538+
}
539+
540+
V8_INLINE size_t VisitJSArrayBuffer(Tagged<Map> map,
541+
Tagged<JSArrayBuffer> object,
542+
MaybeObjectSize) {
543+
if constexpr (kExpectedObjectAge == ObjectAge::kYoung) {
544+
object->YoungMarkExtension();
545+
} else {
546+
object->YoungMarkExtensionPromoted();
556547
}
548+
int size = JSArrayBuffer::BodyDescriptor::SizeOf(map, object);
549+
JSArrayBuffer::BodyDescriptor::IterateBody(map, object, size, this);
550+
return size;
557551
}
558552

559553
V8_INLINE size_t VisitJSWeakRef(Tagged<Map> map, Tagged<JSWeakRef> object,

‎src/heap/sweeper.cc‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -513,11 +513,7 @@ class PromotedPageRecordMigratedSlotVisitor final
513513
V8_INLINE size_t VisitJSArrayBuffer(Tagged<Map> map,
514514
Tagged<JSArrayBuffer> object,
515515
MaybeObjectSize maybe_object_size) {
516-
ArrayBufferExtension* extension = object->extension();
517-
if (extension) {
518-
extension->InitializationBarrier();
519-
extension->YoungMarkPromoted();
520-
}
516+
object->YoungMarkExtensionPromoted();
521517
return NewSpaceVisitor<PromotedPageRecordMigratedSlotVisitor>::
522518
VisitJSArrayBuffer(map, object, maybe_object_size);
523519
}

‎src/heap/young-generation-marking-visitor-inl.h‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,14 @@ void YoungGenerationMarkingVisitor<marking_mode>::VisitCppHeapPointer(
7777
}
7878
}
7979

80+
template <YoungGenerationMarkingVisitationMode marking_mode>
81+
size_t YoungGenerationMarkingVisitor<marking_mode>::VisitJSArrayBuffer(
82+
Tagged<Map> map, Tagged<JSArrayBuffer> object,
83+
MaybeObjectSize maybe_object_size) {
84+
object->YoungMarkExtension();
85+
return Base::VisitJSArrayBuffer(map, object, maybe_object_size);
86+
}
87+
8088
template <YoungGenerationMarkingVisitationMode marking_mode>
8189
template <typename T, typename TBodyDescriptor>
8290
size_t YoungGenerationMarkingVisitor<marking_mode>::VisitJSObjectSubclass(
@@ -123,14 +131,6 @@ void YoungGenerationMarkingVisitor<marking_mode>::VisitExternalPointer(
123131
ExternalPointerTable& table = isolate_->external_pointer_table();
124132
auto* space = isolate_->heap()->young_external_pointer_space();
125133
table.Mark(space, handle, slot.address());
126-
if (slot.tag_range() == kArrayBufferExtensionTag) {
127-
if (ArrayBufferExtension* extension =
128-
reinterpret_cast<ArrayBufferExtension*>(
129-
table.Get(handle, kArrayBufferExtensionTag))) {
130-
extension->InitializationBarrier();
131-
extension->YoungMark();
132-
}
133-
}
134134
}
135135

136136
// Add to the remset whether the handle is null or not, as the slot could be

‎src/heap/young-generation-marking-visitor.h‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,10 @@ class YoungGenerationMarkingVisitor final
6464
VisitPointersImpl(host, p, p + 1);
6565
}
6666

67+
// Visitation specializations used for unified heap young gen marking.
68+
V8_INLINE size_t VisitJSArrayBuffer(Tagged<Map> map,
69+
Tagged<JSArrayBuffer> object,
70+
MaybeObjectSize);
6771
// Visitation specializations used for collecting pretenuring feedback.
6872
template <typename T, typename TBodyDescriptor = typename T::BodyDescriptor>
6973
V8_INLINE size_t VisitJSObjectSubclass(Tagged<Map> map, Tagged<T> object,

‎src/objects/js-array-buffer-inl.h‎

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -109,13 +109,16 @@ void JSArrayBuffer::init_extension() {
109109

110110
ArrayBufferExtension* JSArrayBuffer::extension() const {
111111
#if V8_COMPRESS_POINTERS
112+
// We need Acquire semantics here when loading the entry, see below.
113+
// Consider adding respective external pointer accessors if non-relaxed
114+
// ordering semantics are ever needed in other places as well.
112115
Isolate* isolate = Isolate::Current();
113116
ExternalPointerHandle handle =
114-
base::AsAtomic32::Relaxed_Load(extension_handle_location());
117+
base::AsAtomic32::Acquire_Load(extension_handle_location());
115118
return reinterpret_cast<ArrayBufferExtension*>(
116119
isolate->external_pointer_table().Get(handle, kArrayBufferExtensionTag));
117120
#else
118-
return base::AsAtomicPointer::Relaxed_Load(extension_location());
121+
return base::AsAtomicPointer::Acquire_Load(extension_location());
119122
#endif // V8_COMPRESS_POINTERS
120123
}
121124

@@ -125,21 +128,23 @@ void JSArrayBuffer::set_extension(ArrayBufferExtension* extension) {
125128
// pointer fields in the no-sandbox-ptr-compression config, replace this code
126129
// here and above with the respective external pointer accessors.
127130
IsolateForPointerCompression isolate = Isolate::Current();
128-
constexpr ExternalPointerTag tag = kArrayBufferExtensionTag;
131+
const ExternalPointerTag tag = kArrayBufferExtensionTag;
129132
Address value = reinterpret_cast<Address>(extension);
130133
ExternalPointerTable& table = isolate.GetExternalPointerTableFor(tag);
134+
131135
ExternalPointerHandle current_handle =
132136
base::AsAtomic32::Relaxed_Load(extension_handle_location());
133137
if (current_handle == kNullExternalPointerHandle) {
138+
// We need Release semantics here, see above.
134139
ExternalPointerHandle handle = table.AllocateAndInitializeEntry(
135140
isolate.GetExternalPointerTableSpaceFor(tag, address()), value, tag);
136-
base::AsAtomic32::Relaxed_Store(extension_handle_location(), handle);
141+
base::AsAtomic32::Release_Store(extension_handle_location(), handle);
137142
EXTERNAL_POINTER_WRITE_BARRIER(*this, kExtensionOffset, tag);
138143
} else {
139144
table.Set(current_handle, value, tag);
140145
}
141146
#else
142-
base::AsAtomicPointer::Relaxed_Store(extension_location(), extension);
147+
base::AsAtomicPointer::Release_Store(extension_location(), extension);
143148
#endif // V8_COMPRESS_POINTERS
144149
WriteBarrier::ForArrayBufferExtension(*this, extension);
145150
}

‎src/objects/js-array-buffer.cc‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -277,6 +277,28 @@ std::shared_ptr<BackingStore> JSArrayBuffer::RemoveExtension() {
277277
return result;
278278
}
279279

280+
void JSArrayBuffer::MarkExtension() {
281+
ArrayBufferExtension* extension = this->extension();
282+
if (extension) {
283+
extension->Mark();
284+
}
285+
}
286+
287+
void JSArrayBuffer::YoungMarkExtension() {
288+
ArrayBufferExtension* extension = this->extension();
289+
if (extension) {
290+
DCHECK_EQ(ArrayBufferExtension::Age::kYoung, extension->age());
291+
extension->YoungMark();
292+
}
293+
}
294+
295+
void JSArrayBuffer::YoungMarkExtensionPromoted() {
296+
ArrayBufferExtension* extension = this->extension();
297+
if (extension) {
298+
extension->YoungMarkPromoted();
299+
}
300+
}
301+
280302
Handle<JSArrayBuffer> JSTypedArray::GetBuffer(Isolate* isolate) {
281303
DirectHandle<JSTypedArray> self(*this, isolate);
282304
DCHECK(IsTypedArrayOrRabGsabTypedArrayElementsKind(self->GetElementsKind()));

0 commit comments

Comments
 (0)