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. You will also likely get someone who will
ask why not support an overload that operates on T* instead of
Atomic<T>*. 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).
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 +=...
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)
> 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. 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.
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.
--
Joshua Cranmer
Thunderbird and DXR developer
Source code archæologist
_______________________________________________
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
