Bug 142661 – Introduce WTF::Atomic to wrap std::atomic for a friendlier CAS

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

The CAS functions provided by std::atomic takes a reference to the expected value and modifies it if the CAS fails.  However, in a lot of our CAS usage, we don't want the expected value to change.  The solution to this is to provide a WTF::Atomic struct that wraps std::atomic, and provide CAS methods that won't alter the expected value if the CAS fails.

Also change CodeBlock, ByteSpinLock, and the DFG's crashLock to use WTF::Atomic instead of std::atomic.

[Mark Lam]

Created attachment 248577 [details]
the patch.

[WebKit Commit Bot]

Attachment 248577 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Atomics.h:89:  compare_exchange_weak is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/Atomics.h:95:  compare_exchange_strong is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Comment on attachment 248577 [details]
the patch.

Thanks for the review.  I'll land this manually so that I can get on with the next step.

[Mark Lam]

Comment on attachment 248577 [details]
the patch.

Landed in r181481: <http://trac.webkit.org/r181481>.

[Darin Adler]

Comment on attachment 248577 [details]
the patch.

I think we should add helper functions of our own that work on a std::atomic rather than introducing WTF::Atomic, a slightly-different-behavior derived class. I understand that we are more used to a different design for compare/exchange, but long term we can expect people to be familiar with the standard classes and it would be nice to use them rather than subtle variations on them. Even though this class was the quickest way to fix the problems caused by our misunderstanding.

[Filip Pizlo]

(In reply to comment #5)
> Comment on attachment 248577 [details]
> the patch.
> 
> I think we should add helper functions of our own that work on a std::atomic
> rather than introducing WTF::Atomic, a slightly-different-behavior derived
> class. I understand that we are more used to a different design for
> compare/exchange, but long term we can expect people to be familiar with the
> standard classes and it would be nice to use them rather than subtle
> variations on them. Even though this class was the quickest way to fix the
> problems caused by our misunderstanding.

I don't really think it was a misunderstanding as much as a disagreement.  The fact that the standard specified CAS to mutate expected is pretty bad.  WTF has precedent for having our own classes that replace standard classes, in cases where we don't believe that the standard class gives us what we need.