Bug 1798284 part 3 - Use a non-atomic load for HeaderWord::get. r=jonco

This improves GCC/Clang compiler optimizations. For example, when checking if
an object has `JSClass` `A` or `B` with `obj->is<A>() || obj->is<B>()`, the
compiler is now able to fold the redundant `obj->shape->base->clasp` loads.

This has an exception for `MaybeForwardedObjectClass` because that's called from
`ArrayBufferViewObject::trace` during compacting GC.

forwardedshape

Differential Revision: https://phabricator.services.mozilla.com/D160793

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<PtrT*>(uintptr_t(this->header_.get()));
   }
+  PtrT* headerPtrAtomic() const {
+    staticAsserts();
+    MOZ_ASSERT(this->flags() == 0);
+    return reinterpret_cast<PtrT*>(uintptr_t(this->header_.getAtomic()));
+  }
 
   void unbarrieredSetHeaderPtr(PtrT* newValue) {
     uintptr_t data = uintptr_t(newValue);

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();
 }

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_;