Re: mozilla::Atomic considered harmful
On 2014-04-02, 8:45 PM, Joshua Cranmer wrote: On 4/2/2014 5:30 PM, Ehsan Akhgari wrote: 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. std::atomic has the fetch_* methods on them. I'm actually surprised that C++11 has global memory functions here, considering that they only accept atomicT* in arguments and don't support T*. Ah right, OK let's use member functions then! 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 My (admittedly limited) experience of atomics suggests that the cases where it wouldn't be obvious from context that the variable in question is atomic (and that the variable could be accidentally misused atomically--mozilla::Atomicbool is rather hard to misuse) are comparatively limited: I can't think of a situation outside of reference counts where people could plausibly use an atomic without realizing it's an atomic. OK I don't know how to convince you on that if what has been said earlier in the thread is not convincing enough. 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. The only reason the clang plugin isn't running on OS X is because I don't use OS X and thus don't know how to make it work with clang there, and no one else has offered to step in and add support. There's no reason it couldn't be extended to work on Windows or B2G in the long run (or even making other custom plugins for msvc/gcc), although that would have the ancillary effect of making Clang builds effectively tier 1 there. Sure, agreed on all of the above, but I'm still not sure if it's a good idea to move compile time checks which are expressible in C++ to a compiler plugin. (No question on the ones that are not, of course.) 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? To me, it's a matter of reducing the usability and fundamental design to bring about illusions of safety in what I contend is a corner case of API usage. I disagree that it's going to only give you an illusion of safety, as it clearly makes at least some unsafe code not compile. I can't see how one can claim the contrary. Cheers, Ehsan ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::Atomic considered harmful
On 2014-04-02, 6:53 PM, Martin Thomson wrote: On 2014-04-02, at 14:01, Ehsan Akhgari ehsan.akhg...@gmail.com wrote: AtomicFetchAndSub(mRefCnt, -1); I think that I’ll join those who seem to favour member functions over statics, as you seem to prefer for some reason. I'm open to using member functions and just agreed to doing so elsewhere in the thread. If you want to avoid inventing names and such, how about copying from something that is known to work already: http://docs.oracle.com/javase/6/docs/api/java/util/concurrent/atomic/package-summary.html .get() : T .getAndSet(T) : T .getAndIncrement() : T The Java APIs are pretty aggressive at covering all options; no need to be so complete. The names are sensible though. Sure, but you need to convince Joshua on the names. :-) Any sane ones are good as far as I'm concerned. Cheers, Ehsan ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::Atomic considered harmful
Ehsan Akhgari wrote:
On 2014-04-01, 8:10 PM, Martin Thomson wrote:
count_type tmp = --mRefCnt;
if (tmp == 0) {
delete this;
}
And how do we enforce people to write code like the above example
using the current Atomic interface?
Would WARN_UNUSED_RESULT help here, so that you remember to use the
result of the -- operator?
--
Warning: May contain traces of nuts.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::Atomic considered harmful
On 04/01/2014 02:32 PM, Ehsan Akhgari wrote:
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;
}
I think this is a good idea to make the atomic-part of the type appear in
the code. This will at least be a visual hint of threading issue handling.
On the other hand I am against removing these operators, these operators are
convenient as they provide a low cost for instrumenting the current code.
What is possible, is to provide a mozilla::Atomic type with no operator on
it, and provide a lock function:
template typename T
mozilla::AtomicHandlerT
mozilla::lock(mozilla::AtomicT cnt) {
…
}
Such as the AtomicHandler has all the operator to manipulate the Atomic.
Also, we should prevent any other form of construction of the AtomicHandler,
except through the lock function. The above code will then look like this:
--lock(mRefCnt);
if (lock(mRefCnt) == 0) {
delete this;
}
This way, this is more obvious that we might not be doing the right things,
as long as we are careful to refuse AtomicHandler references in reviews.
--
Nicolas B. Pierron
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::Atomic considered harmful
On 4/2/2014 11:33 AM, Nicolas B. Pierron wrote:
--lock(mRefCnt);
if (lock(mRefCnt) == 0) {
delete this;
}
This way, this is more obvious that we might not be doing the right
things, as long as we are careful to refuse AtomicHandler references
in reviews.
I personally don't think this will save us. This can easily slip
through review as well.
Also, I'm using our mozilla::Atomic for not just refcounting but as an
easy lock-less t-s counters. If I had to change the code from
mMyCounter += something; to mozilla::Unused
AtomicFetchAndAdd(mMyCounter, something); I would not be happy :)
According the refcnt code (or any code that may be concerned) better is
to treat is as always thread safe if not an overkill of course...
Same as you wear condoms with strangers, right?
-hb-
smime.p7s
Description: S/MIME Cryptographic Signature
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::Atomic considered harmful
On Wed, Apr 02, 2014 at 05:03:53PM +0200, Honza Bambas wrote:
On 4/2/2014 11:33 AM, Nicolas B. Pierron wrote:
--lock(mRefCnt);
if (lock(mRefCnt) == 0) {
delete this;
}
This way, this is more obvious that we might not be doing the right
things, as long as we are careful to refuse AtomicHandler references in
reviews.
I personally don't think this will save us. This can easily slip through
review as well.
Also, I'm using our mozilla::Atomic for not just refcounting but as an
easy lock-less t-s counters. If I had to change the code from mMyCounter +=
something; to mozilla::Unused AtomicFetchAndAdd(mMyCounter, something);
I would not be happy :)
According the refcnt code (or any code that may be concerned) better is to
treat is as always thread safe if not an overkill of course... Same as
you wear condoms with strangers, right?
so are you offering to audit all of the existing code that might be
used with AtomicT to make sure it is threadsafe?
Trev
-hb-
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
signature.asc
Description: Digital signature
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::Atomic considered harmful
On 4/2/2014 5:24 PM, Trevor Saunders wrote:
On Wed, Apr 02, 2014 at 05:03:53PM +0200, Honza Bambas wrote:
On 4/2/2014 11:33 AM, Nicolas B. Pierron wrote:
--lock(mRefCnt);
if (lock(mRefCnt) == 0) {
delete this;
}
This way, this is more obvious that we might not be doing the right
things, as long as we are careful to refuse AtomicHandler references in
reviews.
I personally don't think this will save us. This can easily slip through
review as well.
Also, I'm using our mozilla::Atomic for not just refcounting but as an
easy lock-less t-s counters. If I had to change the code from mMyCounter +=
something; to mozilla::Unused AtomicFetchAndAdd(mMyCounter, something);
I would not be happy :)
According the refcnt code (or any code that may be concerned) better is to
treat is as always thread safe if not an overkill of course... Same as
you wear condoms with strangers, right?
so are you offering to audit all of the existing code that might be
used with AtomicT to make sure it is threadsafe?
Maybe yes, as I want to do for usage of weak reference in bug
https://bugzilla.mozilla.org/show_bug.cgi?id=956338
On the other hand, none of the suggestions here doesn't sound to me like
there would be no need for an audit after we migrate to them and be
perfectly safe. Despite we would probably have to change all places we
use atomics right now - which is more or less equal to an audit :)
-hb-
Trev
-hb-
___
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
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::Atomic considered harmful
On Wed, Apr 02, 2014 at 05:37:39PM +0200, Honza Bambas wrote:
On 4/2/2014 5:24 PM, Trevor Saunders wrote:
On Wed, Apr 02, 2014 at 05:03:53PM +0200, Honza Bambas wrote:
On 4/2/2014 11:33 AM, Nicolas B. Pierron wrote:
--lock(mRefCnt);
if (lock(mRefCnt) == 0) {
delete this;
}
This way, this is more obvious that we might not be doing the right
things, as long as we are careful to refuse AtomicHandler references in
reviews.
I personally don't think this will save us. This can easily slip through
review as well.
Also, I'm using our mozilla::Atomic for not just refcounting but as an
easy lock-less t-s counters. If I had to change the code from mMyCounter +=
something; to mozilla::Unused AtomicFetchAndAdd(mMyCounter, something);
I would not be happy :)
According the refcnt code (or any code that may be concerned) better is to
treat is as always thread safe if not an overkill of course... Same as
you wear condoms with strangers, right?
so are you offering to audit all of the existing code that might be
used with AtomicT to make sure it is threadsafe?
Maybe yes, as I want to do for usage of weak reference in bug
https://bugzilla.mozilla.org/show_bug.cgi?id=956338
On the other hand, none of the suggestions here doesn't sound to me like
there would be no need for an audit after we migrate to them and be
perfectly safe. Despite we would probably have to change all places we use
atomics right now - which is more or less equal to an audit :)
no, you'd need to audit more than just what currently uses atomics
because of things like this
templatetypename T
T
MaxOf3(T a, T b, T c)
{
T temp = a b ? a : b;
return temp c ? temp : c;
}
Which is fine as long as we only use it with non threadsafe objects,
but if someone comes along and uses it on atomic values they'll expect
the implementation is threadsafe and so introduce a race.
Trev
-hb-
Trev
-hb-
___
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
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
signature.asc
Description: Digital signature
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::Atomic considered harmful
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. 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. :-) 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. 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! Cheers, Ehsan ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::Atomic considered harmful
On 2014-04-02, 11:03 AM, Honza Bambas wrote:
On 4/2/2014 11:33 AM, Nicolas B. Pierron wrote:
--lock(mRefCnt);
if (lock(mRefCnt) == 0) {
delete this;
}
This way, this is more obvious that we might not be doing the right
things, as long as we are careful to refuse AtomicHandler references
in reviews.
I personally don't think this will save us. This can easily slip
through review as well.
Hmm... This is kind of an interesting suggestion. I think it would be
better to call it AtomicLock or something along those lines. But, at
that point, what is the goal exactly? Is
--AtomicLock(mRefCnt)
much more readable than:
AtomicFetchAndSub(mRefCnt, -1);
It is at least weird in that it's decrementing a temporary which is
usually a bad idea. And I think we have already lost on the ease of use
front here.
Also, I'm using our mozilla::Atomic for not just refcounting but as an
easy lock-less t-s counters. If I had to change the code from
mMyCounter += something; to mozilla::Unused
AtomicFetchAndAdd(mMyCounter, something); I would not be happy :)
To be clear, I also like convenience where it doesn't trade off safety,
but this is not one of those cases. Also, code is read more often than
it's written, etc. :-)
Cheers,
Ehsan
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::Atomic considered harmful
On 2014-04-02, 4:43 AM, Neil wrote:
Ehsan Akhgari wrote:
On 2014-04-01, 8:10 PM, Martin Thomson wrote:
count_type tmp = --mRefCnt;
if (tmp == 0) {
delete this;
}
And how do we enforce people to write code like the above example
using the current Atomic interface?
Would WARN_UNUSED_RESULT help here, so that you remember to use the
result of the -- operator?
Yes, that came up on the bug as well, and it's a fallback strategy if my
proposal doesn't win people's hearts and minds. But it has won some
already, so let's hold off on that idea for now. :-)
Cheers,
Ehsan
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::Atomic considered harmful
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 AtomicT*. 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
Re: mozilla::Atomic considered harmful
On Wed, Apr 2, 2014 at 5:29 PM, Joshua Cranmer pidgeo...@gmail.comwrote: 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 AtomicT*. 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
Re: mozilla::Atomic considered harmful
On 2014-04-02, at 14:01, Ehsan Akhgari ehsan.akhg...@gmail.com wrote: AtomicFetchAndSub(mRefCnt, -1); I think that I’ll join those who seem to favour member functions over statics, as you seem to prefer for some reason. If you want to avoid inventing names and such, how about copying from something that is known to work already: http://docs.oracle.com/javase/6/docs/api/java/util/concurrent/atomic/package-summary.html .get() : T .getAndSet(T) : T .getAndIncrement() : T The Java APIs are pretty aggressive at covering all options; no need to be so complete. The names are sensible though. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::Atomic considered harmful
2014-04-02 11:03 GMT-04:00 Honza Bambas honzab@firemni.cz:
On 4/2/2014 11:33 AM, Nicolas B. Pierron wrote:
--lock(mRefCnt);
if (lock(mRefCnt) == 0) {
delete this;
}
This way, this is more obvious that we might not be doing the right
things, as long as we are careful to refuse AtomicHandler references in
reviews.
I personally don't think this will save us. This can easily slip through
review as well.
Also, I'm using our mozilla::Atomic for not just refcounting but as an
easy lock-less t-s counters. If I had to change the code from mMyCounter
+= something; to mozilla::Unused AtomicFetchAndAdd(mMyCounter,
something); I would not be happy :)
I hope that here on dev-platform we all agree that what we're really
interested in is making it easier to *read* code, much more than making it
easier to *write* code!
Assuming that we do, then the above argument weighs very little against the
explicitness of AtomicFetchAdd saving the person reading this code from
missing the atomic part!
Benoit
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::Atomic considered harmful
On 4/2/2014 5:30 PM, Ehsan Akhgari wrote: 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. std::atomic has the fetch_* methods on them. I'm actually surprised that C++11 has global memory functions here, considering that they only accept atomicT* in arguments and don't support T*. 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 My (admittedly limited) experience of atomics suggests that the cases where it wouldn't be obvious from context that the variable in question is atomic (and that the variable could be accidentally misused atomically--mozilla::Atomicbool is rather hard to misuse) are comparatively limited: I can't think of a situation outside of reference counts where people could plausibly use an atomic without realizing it's an 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. The only reason the clang plugin isn't running on OS X is because I don't use OS X and thus don't know how to make it work with clang there, and no one else has offered to step in and add support. There's no reason it couldn't be extended to work on Windows or B2G in the long run (or even making other custom plugins for msvc/gcc), although that would have the ancillary effect of making Clang builds effectively tier 1 there. 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? To me, it's a matter of reducing the usability and fundamental design to bring about illusions of safety in what I contend is a corner case of API usage. -- 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
mozilla::Atomic considered harmful
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
Re: mozilla::Atomic considered harmful
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.comwrote:
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
Re: mozilla::Atomic considered harmful
On 2014-04-01, at 15:15, Rob Arnold tell...@gmail.com wrote: 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. That was my impression too. I usually find that I have to chase after the definition to ensure that the variable is atomic, not the other way around. I’m used to Java’s primitives, which are explicit. Just don’t do both. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::Atomic considered harmful
On 04/01/2014 02:32 PM, Ehsan Akhgari wrote: What do people feel about my proposal? Do you think it improves writing and reviewing thread safe code to be less error prone? As I said in the bug, not particularly. I don't think you can program with atomics in any sort of brain-off way, and I don't think the boilerplate difference of += versus fetch-and-add or whatever really affects that. To the extent things should be done differently, it should be that *template* functions that deal with atomic/non-atomic versions of the same algorithm deserve extra review and special care, and perhaps even should be implemented twice, rather than sharing a single implementation. And I think the cases in question here are flavors of approximately a single issue, and we do not have a fundamental problem here to be solved by making the API more obtuse in practice. Jeff ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::Atomic considered harmful
2014-04-01 18:40 GMT-04:00 Jeff Walden jwalden+...@mit.edu: On 04/01/2014 02:32 PM, Ehsan Akhgari wrote: What do people feel about my proposal? Do you think it improves writing and reviewing thread safe code to be less error prone? As I said in the bug, not particularly. I don't think you can program with atomics in any sort of brain-off way, and I don't think the boilerplate difference of += versus fetch-and-add or whatever really affects that. To the extent things should be done differently, it should be that *template* functions that deal with atomic/non-atomic versions of the same algorithm deserve extra review and special care, and perhaps even should be implemented twice, rather than sharing a single implementation. And I think the cases in question here are flavors of approximately a single issue, and we do not have a fundamental problem here to be solved by making the API more obtuse in practice. How are we going to enforce (and ensure that future people enforce) that? (The part about functions that deal with atomic/non-atomic versions of the same algorithm deserve extra review and special care) ? I like Ehsan's proposal because, as far as I am concerned, explicit function names help me very well to remember to check atomic semantics; especially if we follow the standard atomic naming where the functions start by atomic_ , e.g. std::atomic_fetch_add. On the other hand, if the function name that we use for that is just operator + then it becomes very hard for me as a reviewer, because I have to remember checking everytime I see a + to check if the variables at hand are atomics. Benoit ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::Atomic considered harmful
On 2014-04-01, 6:59 PM, Benoit Jacob wrote: 2014-04-01 18:40 GMT-04:00 Jeff Walden jwalden+...@mit.edu: On 04/01/2014 02:32 PM, Ehsan Akhgari wrote: What do people feel about my proposal? Do you think it improves writing and reviewing thread safe code to be less error prone? As I said in the bug, not particularly. I don't think you can program with atomics in any sort of brain-off way, and I don't think the boilerplate difference of += versus fetch-and-add or whatever really affects that. To the extent things should be done differently, it should be that *template* functions that deal with atomic/non-atomic versions of the same algorithm deserve extra review and special care, and perhaps even should be implemented twice, rather than sharing a single implementation. And I think the cases in question here are flavors of approximately a single issue, and we do not have a fundamental problem here to be solved by making the API more obtuse in practice. How are we going to enforce (and ensure that future people enforce) that? (The part about functions that deal with atomic/non-atomic versions of the same algorithm deserve extra review and special care) ? My proposal would enforce that! I like Ehsan's proposal because, as far as I am concerned, explicit function names help me very well to remember to check atomic semantics; especially if we follow the standard atomic naming where the functions start by atomic_ , e.g. std::atomic_fetch_add. On the other hand, if the function name that we use for that is just operator + then it becomes very hard for me as a reviewer, because I have to remember checking everytime I see a + to check if the variables at hand are atomics. Just to clarify my position a bit more, I think criticizing my position by pretending that I'm advocating for a brain-off way of programming with atomics is a bit contrived. I definitely understand that atomics require special feeding and care. What's under debate is whether we should make that obvious to authors and reviewers by not conflating things such as operator++ etc. to work on both atomic and non-atomic types. Cheers, Ehsan ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::Atomic considered harmful
On 2014-04-01, at 16:17, Ehsan Akhgari ehsan.akhg...@gmail.com wrote: Just to clarify my position a bit more, I think criticizing my position by pretending that I'm advocating for a brain-off way of programming with atomics is a bit contrived. I definitely understand that atomics require special feeding and care. What's under debate is whether we should make that obvious to authors and reviewers by not conflating things such as operator++ etc. to work on both atomic and non-atomic types. I don’t think that the pushback is based on the fact that code using Atomicuint32_t is at least as thread safe as code using uint32_t. As is, someone reading code is likely to see threading errors that are actually safe due to use of Atomic. The opposite - missing a real error - happens because we are human. It doesn’t seem more or less likely if you require the more explicit syntax. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::Atomic considered harmful
On 2014-04-01, 6:15 PM, Rob Arnold wrote: 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. I was hesitant to give the refcount example to avoid confusing people to think I'm just talking about refcounts, but I'm glad you got the point! In the case of that mRefCnt variable, I originally did not pay attention to the bug in the code because nothing about |mRefCnt| looked like it's an atomic variable. To be honest, I don't go and check the type of every single variable that I manipulate in my patches, in a lot of cases I rely on cues in the surrounding code to tell me something about the type (unconsciously thinking, this thing is called mRefCnt, and it has operator++ and --, so it's got to be an integer.) I cannot speak why the reviewers did not catch this bug, but I suspect because of similar reasons. 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. I agree that in code which does obvious threading, my proposal won't improve things, but I'm mostly worried about bugs creeping in where we use atomics for things that are not obviously threading related. Cheers, Ehsan ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::Atomic considered harmful
On 2014-04-01, 7:32 PM, Martin Thomson wrote:
On 2014-04-01, at 16:17, Ehsan Akhgari ehsan.akhg...@gmail.com wrote:
Just to clarify my position a bit more, I think criticizing my position by
pretending that I'm advocating for a brain-off way of programming with atomics
is a bit contrived. I definitely understand that atomics require special
feeding and care. What's under debate is whether we should make that obvious
to authors and reviewers by not conflating things such as operator++ etc. to
work on both atomic and non-atomic types.
I don’t think that the pushback is based on the fact that code using
Atomicuint32_t is at least as thread safe as code using uint32_t.
As is, someone reading code is likely to see threading errors that are actually safe
due to use of Atomic. The opposite - missing a real error - happens because
we are human. It doesn’t seem more or less likely if you require the more explicit
syntax.
So are you suggesting that if the code in my original patch looked like
this:
// (a)
if (mozilla::AtomicFetchSub(mRefCnt, -1) == 1) {
delete this;
}
I would still go ahead and rewrite it as:
// (b)
mozilla::AtomicFetchSub(mRefCnt, -1);
if (mozilla::AtomicLoad(mRefCnt) == 0) {
delete this;
}
My contention is that it is obvious enough by looking at (a) to tell
that mRefCnt is an atomic value which should be handled with the
necessary care, so the pattern (b) would be caught either at code
writing time or at code review time.
Cheers,
Ehsan
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::Atomic considered harmful
On Tue, Apr 01, 2014 at 05:32:12PM -0400, Ehsan Akhgari 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.
Actually, the thread safety bug in this code is largely the same whether the
type of mRefCnt is mozilla::Atomic or not. Compiler optimizations may
remove the bug, but it's there to begin with.
As I said in the bug, all this is saying is that thread safety is hard,
and atomics are merely one of the tools to achieve thread safety. They
are not a magic wand that fixes thread safety.
I also think that making their API not have operators like std::atomic
has would bring a false sense of security to people writing code using
them, because it would supposedly be less confusing when it really
wouldn't.
Mike
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::Atomic considered harmful
On 2014-04-01, at 16:48, Ehsan Akhgari ehsan.akhg...@gmail.com wrote:
My contention is that it is obvious enough by looking at (a) to tell that
mRefCnt is an atomic value which should be handled with the necessary care,
so the pattern (b) would be caught either at code writing time or at code
review time.
My point was that:
--mRefCnt;
if (mRefCnt == 0) {
delete this;
}
is as much an obvious threading issue as:
if (--mRefCnt == 0) {
delete this;
}
…absent some extra knowledge.
More verbosity isn’t going to change this. This might:
count_type tmp = --mRefCnt;
if (tmp == 0) {
delete this;
}
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::Atomic considered harmful
On 2014-04-01, 7:58 PM, Mike Hommey wrote:
On Tue, Apr 01, 2014 at 05:32:12PM -0400, Ehsan Akhgari 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.
Actually, the thread safety bug in this code is largely the same whether the
type of mRefCnt is mozilla::Atomic or not. Compiler optimizations may
remove the bug, but it's there to begin with.
That is the code after I changed it. :-) Here is the original patch
which introduced this bug:
https://bug935778.bugzilla.mozilla.org/attachment.cgi?id=8385072 The
thread safety issue was _not_ there before that patch.
As I said in the bug, all this is saying is that thread safety is hard,
and atomics are merely one of the tools to achieve thread safety. They
are not a magic wand that fixes thread safety.
Did you read https://bugzilla.mozilla.org/show_bug.cgi?id=987887#c20?
Continuing to reduce my argument here to atomics should be a magic
wand is really depressing.
I also think that making their API not have operators like std::atomic
has would bring a false sense of security to people writing code using
them, because it would supposedly be less confusing when it really
wouldn't.
Can you please give an example of that concern? I'm not sure if I follow.
Thanks,
Ehsan
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::Atomic considered harmful
On Tue, Apr 01, 2014 at 08:34:01PM -0400, Ehsan Akhgari wrote:
On 2014-04-01, 7:58 PM, Mike Hommey wrote:
On Tue, Apr 01, 2014 at 05:32:12PM -0400, Ehsan Akhgari 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.
Actually, the thread safety bug in this code is largely the same whether the
type of mRefCnt is mozilla::Atomic or not. Compiler optimizations may
remove the bug, but it's there to begin with.
That is the code after I changed it. :-) Here is the original patch which
introduced this bug:
https://bug935778.bugzilla.mozilla.org/attachment.cgi?id=8385072 The
thread safety issue was _not_ there before that patch.
I'm not saying there was a thread safety issue before. I'm saying that in
your example, there is largely the same thread safety issue whether the
code uses a plain int or an atomic one. The use of atomics is _not_ hiding
this bug.
As I said in the bug, all this is saying is that thread safety is hard,
and atomics are merely one of the tools to achieve thread safety. They
are not a magic wand that fixes thread safety.
Did you read https://bugzilla.mozilla.org/show_bug.cgi?id=987887#c20?
Did you read my answer to that comment?
Continuing to reduce my argument here to atomics should be a magic wand is
really depressing.
I also think that making their API not have operators like std::atomic
has would bring a false sense of security to people writing code using
them, because it would supposedly be less confusing when it really
wouldn't.
Can you please give an example of that concern? I'm not sure if I follow.
Just take the original patch which introduced the bug. I seriously don't
believe if the patch had been using FetchAndAdd instead of ++/-- the issue
would have had more chance of being caught at review. And I don't
believe it would make people more likely to make atomic-using code
thread safe.
Back to Kyle's comment from the bug, the core issue is that we don't
have anything that tells us that $function is expected to be called from
different threads at the same time, and that as such it _needs_ to be
thread safe or reentrant in some way. Maybe we need to invent those
markers, and to have ways to test when they are missing (like special
builds that ensure that functions without those markers are never called
from two threads simultaneously).
Mike
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::Atomic considered harmful
On Wednesday 2014-04-02 12:50 +0900, Mike Hommey wrote: On Tue, Apr 01, 2014 at 08:34:01PM -0400, Ehsan Akhgari wrote: That is the code after I changed it. :-) Here is the original patch which introduced this bug: https://bug935778.bugzilla.mozilla.org/attachment.cgi?id=8385072 The thread safety issue was _not_ there before that patch. I'm not saying there was a thread safety issue before. I'm saying that in your example, there is largely the same thread safety issue whether the code uses a plain int or an atomic one. The use of atomics is _not_ hiding this bug. That's not the issue. The issue is that the syntax is hiding the use of atomics. And whether the code would have had a thread-safety bug if it hadn't been using atomics is also not relevant; lots of single-threaded code has thread-safety bugs if you magically decide to violate its threading invariants and use the same object on multiple threads when it wasn't designed for that. The issue here is whether this particular way of writing threadsafe code leads people modifying that code to make mistakes because they don't even notice that it's threadsafe code. As I said in the bug, all this is saying is that thread safety is hard, and atomics are merely one of the tools to achieve thread safety. They are not a magic wand that fixes thread safety. Did you read https://bugzilla.mozilla.org/show_bug.cgi?id=987887#c20? Did you read my answer to that comment? I think you're continuing to ignore the fact that using operator overloading obscures what those operators are doing underneath, and when that difference is important to the reader of the code, overloading might not be a good idea. Continuing to reduce my argument here to atomics should be a magic wand is really depressing. I also think that making their API not have operators like std::atomic has would bring a false sense of security to people writing code using them, because it would supposedly be less confusing when it really wouldn't. Can you please give an example of that concern? I'm not sure if I follow. Just take the original patch which introduced the bug. I seriously don't believe if the patch had been using FetchAndAdd instead of ++/-- the issue would have had more chance of being caught at review. And I don't believe it would make people more likely to make atomic-using code thread safe. You might claim that the increased chance of being caught is small, but claiming it's zero is laughable. -David -- 턞 L. David Baron http://dbaron.org/ 턂 턢 Mozilla https://www.mozilla.org/ 턂 Before I built a wall I'd ask to know What I was walling in or walling out, And to whom I was like to give offense. - Robert Frost, Mending Wall (1914) signature.asc Description: Digital signature ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::Atomic considered harmful
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. 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. 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. Taking a look through most of the uses of atomics in our codebase, 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). -- 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
Re: mozilla::Atomic considered harmful
On Wed, Apr 2, 2014 at 12:12 PM, L. David Baron dba...@dbaron.org wrote: The issue here is whether this particular way of writing threadsafe code leads people modifying that code to make mistakes because they don't even notice that it's threadsafe code. I completely agree. And because using the current Atomic code obscures the need to be concerned about threadsafety, I strongly support Ehsan's proposal. As I said in the bug, all this is saying is that thread safety is hard, and atomics are merely one of the tools to achieve thread safety. They are not a magic wand that fixes thread safety. Did you read https://bugzilla.mozilla.org/show_bug.cgi?id=987887#c20? Did you read my answer to that comment? I think you're continuing to ignore the fact that using operator overloading obscures what those operators are doing underneath, and when that difference is important to the reader of the code, overloading might not be a good idea. +Infinity - Kyle ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::Atomic considered harmful
On Wed, Apr 2, 2014 at 12:11 AM, Joshua Cranmer pidgeo...@gmail.comwrote: 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. AFAICT Ehsan is proposing that we continue using atomic types, and his explicit functions would only operate on those atomic types, so his proposal would preserve this aspect of your design. I'm in favor of his proposal. Rob -- Jtehsauts tshaei dS,o n Wohfy Mdaon yhoaus eanuttehrotraiitny eovni le atrhtohu gthot sf oirng iyvoeu rs ihnesa.rt sS?o Whhei csha iids teoa stiheer :p atroa lsyazye,d 'mYaonu,r sGients uapr,e tfaokreg iyvoeunr, 'm aotr atnod sgaoy ,h o'mGee.t uTph eann dt hwea lmka'n? gBoutt uIp waanndt wyeonut thoo mken.o w ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
