Like ThreadSafeRefCounted, we should have a thread safe version of CanMakeCheckedPtr.
Created attachment 435397 [details] Patch
Antti suggested that maybe we can use fetch_add/fetch_sub(1, std::memory_order_relaxed) instead of operator++/--. I suspect we want memory_order_acq_rel instead because we don't want these operations to be reordered with other things happening in the thread. WDYT?
On modern ARM64, std::memory_order_relaxed compiles to ldadd / stdadd. Those are refcount-optimized instructions that are literally free (as far as I can tell after benchmarking MallocBench, PLT, JetStream2, and Speedometer2 with atomic refcounting enabled for *all* objects). So, it's nice to use std::memory_order_relaxed, since it puts us on a path to good performance. (On Intel, std::memory_order_relaxed doesn't help. Atomics are always slow.) I can imagine some theoretical benefits to acquire / release semantics, or sequential consistency semantics, in our refcounted objects. But I believe "it's free, just use it" will be a bigger benefit in the end. So, I'd favor a switch to std::memory_order_relaxed in our thread-safe refcounts. That said, ThreadSafeRefCountedBase currently does ++/--, so maybe the best thing is to land this patch doing the same, and then switch them both. I'd prefer for our approach to be consistent throughout the code.
Comment on attachment 435397 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=435397&action=review r=me > Source/WTF/wtf/CheckedRef.h:253 > + operator IntegralType() const { assertThread(); return m_value; } > + bool operator!() const { assertThread(); return !m_value; } > + SingleThreadIntegralWrapper& operator++() { assertThread(); m_value++; return *this; } > + SingleThreadIntegralWrapper& operator--() { assertThread(); m_value--; return *this; } We can probably afford one line per statement for readability here.
Comment on attachment 435397 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=435397&action=review >> Source/WTF/wtf/CheckedRef.h:253 >> + SingleThreadIntegralWrapper& operator--() { assertThread(); m_value--; return *this; } > > We can probably afford one line per statement for readability here. My preference is to move functions out of the class body when we find we need multiple lines and multiple statements, and I think that works just fine even though you do have to add the inline keyword.
(In reply to Darin Adler from comment #5) > Comment on attachment 435397 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=435397&action=review > > >> Source/WTF/wtf/CheckedRef.h:253 > >> + SingleThreadIntegralWrapper& operator--() { assertThread(); m_value--; return *this; } > > > > We can probably afford one line per statement for readability here. > > My preference is to move functions out of the class body when we find we > need multiple lines and multiple statements, and I think that works just > fine even though you do have to add the inline keyword. Sure, I'd do both.
Created attachment 435625 [details] Patch for landing
Committed r281105 (240564@main): <https://commits.webkit.org/240564@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 435625 [details].
<rdar://problem/81996703>