diff --git a/js/src/gc/Cell.h b/js/src/gc/Cell.h index 810eb476cf88..ee4cf29b2fea 100644 --- a/js/src/gc/Cell.h +++ b/js/src/gc/Cell.h @@ -106,11 +106,17 @@ class CellColor { // Cell header word. Stores GC flags and derived class data. // -// This is atomic since it can be read from and written to by different -// threads during compacting GC, in a limited way. Specifically, writes that -// update the derived class data can race with reads that check the forwarded -// flag. The writes do not change the forwarded flag (which is always false in -// this situation). +// Loads of GC flags + all stores are marked as (relaxed) atomic operations, +// to deal with the following benign data race during compacting GC: +// +// - Thread 1 checks isForwarded (which is always false in this situation). +// - Thread 2 updates the derived class data (without changing the forwarded +// flag). +// +// To improve performance, we don't use atomic operations for get() because +// atomic operations inhibit certain compiler optimizations: GCC and Clang are +// unable to fold multiple loads even if they're both relaxed atomics. This is +// especially a problem for chained loads such as obj->shape->base->clasp. class HeaderWord { // Indicates whether the cell has been forwarded (moved) by generational or // compacting GC and is now a RelocationOverlay. @@ -119,9 +125,6 @@ class HeaderWord { uintptr_t value_; - uintptr_t getAtomic() const { - return __atomic_load_n(&value_, __ATOMIC_RELAXED); - } void setAtomic(uintptr_t value) { __atomic_store_n(&value_, value, __ATOMIC_RELAXED); } @@ -132,9 +135,14 @@ class HeaderWord { static_assert(gc::CellFlagBitsReservedForGC >= 3, "Not enough flag bits reserved for GC"); + uintptr_t getAtomic() const { + return __atomic_load_n(&value_, __ATOMIC_RELAXED); + } + // Accessors for derived class data. uintptr_t get() const { - uintptr_t value = getAtomic(); + // Note: non-atomic load. See class comment. + uintptr_t value = value_; MOZ_ASSERT((value & RESERVED_MASK) == 0); return value; } @@ -817,6 +825,11 @@ class alignas(gc::CellAlignBytes) CellWithTenuredGCPointer : public BaseCell { MOZ_ASSERT(this->flags() == 0); return reinterpret_cast(uintptr_t(this->header_.get())); } + PtrT* headerPtrAtomic() const { + staticAsserts(); + MOZ_ASSERT(this->flags() == 0); + return reinterpret_cast(uintptr_t(this->header_.getAtomic())); + } void unbarrieredSetHeaderPtr(PtrT* newValue) { uintptr_t data = uintptr_t(newValue); diff --git a/js/src/gc/Marking-inl.h b/js/src/gc/Marking-inl.h index 01e3cc45db3a..b6c268195b79 100644 --- a/js/src/gc/Marking-inl.h +++ b/js/src/gc/Marking-inl.h @@ -105,7 +105,7 @@ inline T MaybeForwarded(T t) { } inline const JSClass* MaybeForwardedObjectClass(const JSObject* obj) { - Shape* shape = MaybeForwarded(obj->shape()); + Shape* shape = MaybeForwarded(obj->shapeMaybeForwarded()); BaseShape* baseShape = MaybeForwarded(shape->base()); return baseShape->clasp(); } diff --git a/js/src/vm/JSObject.h b/js/src/vm/JSObject.h index 4c58c6192eed..a49f98f14663 100644 --- a/js/src/vm/JSObject.h +++ b/js/src/vm/JSObject.h @@ -90,6 +90,9 @@ class JSObject // The Shape is stored in the cell header. js::Shape* shape() const { return headerPtr(); } + // Like shape(), but uses getAtomic to read the header word. + js::Shape* shapeMaybeForwarded() const { return headerPtrAtomic(); } + #ifndef JS_64BIT // Ensure fixed slots have 8-byte alignment on 32-bit platforms. uint32_t padding_;