Re: mozilla::Atomic considered harmful

2014-04-03 Thread Ehsan Akhgari
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

Re: mozilla::Atomic considered harmful

2014-04-03 Thread Ehsan Akhgari
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

Re: mozilla::Atomic considered harmful

2014-04-02 Thread Neil
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

Re: mozilla::Atomic considered harmful

2014-04-02 Thread Nicolas B. Pierron
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

Re: mozilla::Atomic considered harmful

2014-04-02 Thread Honza Bambas
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

Re: mozilla::Atomic considered harmful

2014-04-02 Thread Trevor Saunders
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

Re: mozilla::Atomic considered harmful

2014-04-02 Thread Honza Bambas
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

Re: mozilla::Atomic considered harmful

2014-04-02 Thread Trevor Saunders
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,

Re: mozilla::Atomic considered harmful

2014-04-02 Thread Ehsan Akhgari
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.

Re: mozilla::Atomic considered harmful

2014-04-02 Thread Ehsan Akhgari
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

Re: mozilla::Atomic considered harmful

2014-04-02 Thread Ehsan Akhgari
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

Re: mozilla::Atomic considered harmful

2014-04-02 Thread Joshua Cranmer 
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

Re: mozilla::Atomic considered harmful

2014-04-02 Thread Ehsan Akhgari
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

Re: mozilla::Atomic considered harmful

2014-04-02 Thread Martin Thomson
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

Re: mozilla::Atomic considered harmful

2014-04-02 Thread Benoit Jacob
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

Re: mozilla::Atomic considered harmful

2014-04-02 Thread Joshua Cranmer 
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

mozilla::Atomic considered harmful

2014-04-01 Thread Ehsan Akhgari
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

Re: mozilla::Atomic considered harmful

2014-04-01 Thread Rob Arnold
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

Re: mozilla::Atomic considered harmful

2014-04-01 Thread Martin Thomson
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

Re: mozilla::Atomic considered harmful

2014-04-01 Thread Jeff Walden
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

Re: mozilla::Atomic considered harmful

2014-04-01 Thread Benoit Jacob
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

Re: mozilla::Atomic considered harmful

2014-04-01 Thread Ehsan Akhgari
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

Re: mozilla::Atomic considered harmful

2014-04-01 Thread Martin Thomson
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

Re: mozilla::Atomic considered harmful

2014-04-01 Thread Ehsan Akhgari
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

Re: mozilla::Atomic considered harmful

2014-04-01 Thread Ehsan Akhgari
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.

Re: mozilla::Atomic considered harmful

2014-04-01 Thread Mike Hommey
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

Re: mozilla::Atomic considered harmful

2014-04-01 Thread Martin Thomson
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

Re: mozilla::Atomic considered harmful

2014-04-01 Thread Ehsan Akhgari
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

Re: mozilla::Atomic considered harmful

2014-04-01 Thread Mike Hommey
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

Re: mozilla::Atomic considered harmful

2014-04-01 Thread L. David Baron
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

Re: mozilla::Atomic considered harmful

2014-04-01 Thread Joshua Cranmer 
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

Re: mozilla::Atomic considered harmful

2014-04-01 Thread Kyle Huey
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

Re: mozilla::Atomic considered harmful

2014-04-01 Thread Robert O'Callahan
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