[webkit-reviews] review granted: [Bug 201576] Make a ThreadSafeRefCounted object safe to ref & deref inside its destructor : [Attachment 380078] Updated for ToT

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 201576: Make a ThreadSafeRefCounted object safe to ref & deref inside its
destructor
https://bugs.webkit.org/show_bug.cgi?id=201576

Attachment 380078: Updated for ToT

https://bugs.webkit.org/attachment.cgi?id=380078&action=review




--- Comment #17 from Geoffrey Garen <ggaren at apple.com> ---
Comment on attachment 380078
  --> https://bugs.webkit.org/attachment.cgi?id=380078
Updated for ToT

r=me

It's clever to --m_refCount and then set back to 1. I think that's the right
solution even though it doesn't fix a concurrent race. I'll write down my
reasoning here, in case we ever reconsider:

(1) Unconditional atomic increment/decrement is free on modern ARM hardware (as
measured recently by me on MallocBench, PLT, Speedometer2, and JetStream2); but
conditional increment/decrement is not free. It's nice for performance, and
therefore our ability to use ThreadSafeRefCounted more, if our approach uses
unconditional decrement here.

(2) Our goal, as I understand it, is really to defend against re-entrant
destruction caused by creating a new RefPtr during destruction. That goal seems
a lot more important than protecting against racy raw calls to ref() / deref()
because it directly impacts our ability to use RefPtr in more places.