Bug 229018 – Add thread safe version of CanMakeCheckedPtr

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Ryosuke Niwa]

Like ThreadSafeRefCounted, we should have a thread safe version of CanMakeCheckedPtr.

[Ryosuke Niwa]

Created attachment 435397 [details]
Patch

[Ryosuke Niwa]

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?

[Geoffrey Garen]

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.

[Geoffrey Garen]

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.

[Darin Adler]

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.

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

Created attachment 435625 [details]
Patch for landing

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/81996703>