On Wed, Apr 2, 2014 at 5:29 PM, Joshua Cranmer 🐧 <pidgeo...@gmail.com>wrote:
> On 4/2/2014 3:52 PM, Ehsan Akhgari wrote: > >> On 2014-04-02, 12:11 AM, Joshua Cranmer 🐧 wrote: >> >>> On 4/1/2014 4:32 PM, Ehsan Akhgari wrote: >>> >>>> 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!) >>>> >>> >>> I am very much against this, and I think your proposal is a "medicine" >>> which is far more harmful than the "problem" ever is or was. >>> >>> My original design for mozilla::Atomic was meant to avoid what I saw as >>> the biggest footgun: you cannot misuse mozilla::Atomic in such a way >>> that you combine atomic and non-atomic accesses on a single variable. >>> You also cannot mix-and-match different memory orders on the same atomic >>> variable (in this manner, I willfully departed from std::atomic). Using >>> global functions instead would tend to cause people to want to draw this >>> information from the point of declaration to the point of use: note that >>> it is much easier in tooling to find the declaration of a variable than >>> it is to find all uses of one. >>> >> >> What roc said. Really nothing in my proposal would invalidate what you >> said above. To make things super clear, I am only proposing a syntax >> change. >> > > I think I wasn't clear in my original meaning. The problem I have is that, > when you add in mozilla::AtomicFetchAdd, you are likely to get someone who > will ask why not add an overload that supports alternative memory ordering > requirements. Sure, but those people can also ask for support for alternate memory orderings today right? > You will also likely get someone who will ask why not support an overload > that operates on T* instead of Atomic<T>*. We'll point those people to this thread! > I am also mystified as to why you proposed global functions instead of, > say, member functions on mozilla::Atomic itself (I very purposefully never > intended to support the global functions in <atomic> in the first place). > I have no reservations against making them member functions with a clear name that do not return T/T*/etc. I was trying to not be super inconsistent with std::atomic, but if you think that's ok, that's fine by me. > >> There are other issues with your design. The verbose names have, to me >>> at least, been a constant source of confusion: I can never recall if >>> FetchAndAdd returns the value prior to or subsequent to the addition; >>> note that operator+= does not have the same problems. >>> >> >> Please suggest better names then, I'm pretty open to any other name that >> you think is clearer. You can't invalidate my proposal by saying you don't >> like the names. :-) >> > > PlusEquals? Of course, then people would wonder why you don't just use > +=... > We'll refer them to this thread. :-) > >> As for the original problem, I think you're misdiagnosing the failure. >>> The only real problem of the code is that it's possible that people >>> could mistake the variable for not being an atomic variable. >>> >> >> Yes, that is exactly what the issue is! >> >> > Taking a >> >>> look through most of the uses of atomics in our codebase, >>> >> >> FWIW I'm much more interested in code that we'll write in the years to >> come. I don't look forward to people running into this footgun over and >> over again from now on. >> > > I would assert that the use cases that people are likely to encounter in > the future are actually very different from the issue at heart here. There > are basically four types of atomic variables: > * Reference counts > * Cross-thread flags > * Unordered counters > * Lockless algorithms > > Existing usage is heavily biased to the first two of these, and the really > interesting uses tend to be the last one. Your proposal tends to reduce the > utility of mozilla::Atomic for the second and third cases to solve a > problem which is mostly limited to the first pattern. If the first pattern > is easy to miscode, then perhaps the better answer is not to emasculate the > interface with extra verbosity but rather to do introduce a > mozilla::AtomicRefCount class that only supports ++ and -- operations and > doesn't support actually observing the value. (Lockless algorithms tend to > mostly use pointers and CAS) > I'm kind of regretting using the example of reference counts for this discussion. I just wanted to give a real example of something that actually happened and went unnoticed until it hit a user, but I think it ultimately just muddied the waters here. None of what I've said is restricted to reference counting. I don't dare predict what people are going to do with the Atomic type, and I find assertions such as "mostly limited to the first pattern" very hard to believe. The underlying issue is that the syntax is hiding the atomicity semantics. As long as you have that syntax around, any code that uses this type is prone to this problem. > >> > the cases >> >>> where people are liable to not realize that these are atomics are >>> relatively few, especially if patch authors and reviewers are both >>> well-acquainted with the surrounding code. So it's not clear to me that >>> including extra-verbose names would really provide much of a benefit. >>> Instead, I think you're overreacting to what is a comparatively rare >>> case: a method templated to support both non-thread-safe and thread-safe >>> variants, and much less frequently used or tested in thread-safe >>> conditions (note that if you made the same mistake with >>> nsISupports-based refcounting, your patch would be fail sufficiently >>> many tests to be backed out of mozilla-inbound immediately). >>> >> >> See dbaron's reply here: <https://groups.google.com/d/ >> msg/mozilla.dev.platform/nMfQAHeAmiA/GNXkqrKzLKQJ>. I never claimed >> that this will help reviewers catch 100% of the problems, but I don't see >> how you can claim that it doesn't help at all! >> > > I never claimed that it doesn't help. I said that I don't think it helps > *much*: it doesn't make misuse an error, so you're relying on people being > alert and catching the failure. Depends on which misuse you're talking about. It totally makes treating an atomic value as non-atomic syntactically detectible by the compiler. > I looked through the codebase and found another mozilla::Atomic usage that > suffers from this bug--and it was introduced by a mass-conversion change of > PR_ATOMIC_* to mozilla::Atomic. > (Do you have a link to the code/bug please?) > There are other ways to solve the root problem here. I already mentioned > the mozilla::AtomicRefCount idea. Another idea I just thought up is to use > our handy clang plugin to error out on any cases where an atomic variable > is atomically modified and then read in the same basic block, which would > certainly catch misuses without needing to modify mozilla::Atomic. Using the clang plugin to detect this will make the problem non-detectible in places where the code is b2g/OSX/Windows specific. We have a fair bit of those. Can you please help me understand why do you resist changing mozilla::Atomic? Is it just to hold on to the shorter syntax that these operators give you? It seems like you're OK with at least some degree of incompatibility with <atomic>? Thanks! -- Ehsan <http://ehsanakhgari.org/> _______________________________________________ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
