I've got some outside experience with std::atomic so make of it what you
will :)
How often are you touching atomic variables directly? In my experience with
a similar thread safe inline ref count and smart pointer system to
Mozilla's (using std::atomic for the ref count), there's been no confusion
as the equivalent ref counted base class is super small and easy to
read/verify. In the other cases where std::atomic occurs, careful
consideration is already given to threading due to the nature of the code
so the similarity to a non-atomic integer type actually has prompted me to
have a (temporary) false sense of thread unsafety, rather than a false
sense of thread safety. And I think that is fine. For my team's code, I
think it's fine that they have similar notation as it makes operations
(like statistics counters) fairly easy to read vs explicit
atomicIncrement(&counter, value) calls.
-Rob
On Tue, Apr 1, 2014 at 2:32 PM, Ehsan Akhgari <ehsan.akhg...@gmail.com>wrote:
> The subject of this post is intentionally chosen to make you want to read
> this. :-)
>
> The summary is that I think the mozilla::Atomic API which is modeled after
> std::atomic is harmful in that it makes it very non-obvious that you're
> dealing with atomic integers. Basically the existing interface makes
> mozilla::Atomic look like a normal integer, so that if you see code like:
>
> --mRefCnt;
> if (mRefCnt == 0) {
> delete this;
> }
>
> You won't immediately think about checking the type of mRefCnt (the
> refcount case being just an example here of course), which makes it hard to
> spot that there is a thread safety bug in this code. Given that I and
> three experienced Gecko hackers fell prey to this issue (bug 985878 and bug
> 987667), I thought about the underlying problem a bit, and managed to
> convince myself that it's a bad idea for us to pretend that atomic integers
> can be treated the same way as non-atomic integers.
>
> So, over in bug 987887 I'm proposing to remove all of the methods on
> mozilla::Atomic except for copy construction and assignment and replace
> them with global functions that can operate on the atomic type. <atomic>
> has a number of global functions that can operate on std::atomic types <
> http://en.cppreference.com/w/cpp/atomic> and we can look into implementing
> similar ones (for example, mozilla::AtomicFetchAdd,
> mozilla::AtomicExchange, etc. -- names to be bikeshedded outside of this
> thread!)
>
> There is an extensive discussion on bug 987887 about this. The proponents
> of this change have argued that it makes it easier to spot that a given
> type is an atomic one because the type is explicitly treated differently
> than normal built-in types, even at the lack of a bit of convenience that
> the member operators provide. The opponents of this change have argued
> that the alternative is just as error prone as what we have today, and that
> it diverges us from being compatible with the std::atomic type (in answer
> to which I have said that is a good thing, since std::atomic is affected by
> the same problem and I think is a bad idea for us to copy it.)
>
> I am of course biased towards my side of the argument, so please read the
> bug to get a better understanding of people's positions.
>
> What do people feel about my proposal? Do you think it improves writing
> and reviewing thread safe code to be less error prone?
>
> Cheers,
> --
> Ehsan
> <http://ehsanakhgari.org/>
> _______________________________________________
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>
_______________________________________________
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
