RESOLVED FIXED 142661
Introduce WTF::Atomic to wrap std::atomic for a friendlier CAS 
https://bugs.webkit.org/show_bug.cgi?id=142661
Summary Introduce WTF::Atomic to wrap std::atomic for a friendlier CAS
Mark Lam
Reported 2015-03-13 00:42:10 PDT
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.
Attachments
the patch. (9.08 KB, patch)
2015-03-13 00:54 PDT, Mark Lam 		fpizlo: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Mark Lam
Comment 1 2015-03-13 00:54:53 PDT
Created attachment 248577 [details] the patch.
WebKit Commit Bot
Comment 2 2015-03-13 00:59:21 PDT
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 3 2015-03-13 10:59:09 PDT
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 4 2015-03-13 11:04:09 PDT
Comment on attachment 248577 [details] the patch. Landed in r181481: <http://trac.webkit.org/r181481>.
Darin Adler
Comment 5 2015-03-13 13:51:27 PDT
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
Comment 6 2015-03-13 14:32:55 PDT
(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.
Note You need to log in before you can comment on or make changes to this bug.