mozilla::Atomic considered harmful

[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 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/>

[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 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

> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>

[Martin Thomson]

On 2014-04-01, at 15:15, Rob Arnold <tel...@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.

[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 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

[Benoit Jacob]

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

[Ehsan Akhgari]

On 2014-04-01, 6:59 PM, Benoit Jacob wrote:
> 2014-04-01 18:40 GMT-04:00 Jeff Walden <jwald...@mit.edu>:
>

> 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

[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 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

[Martin Thomson]

On 2014-04-01, at 16:17, Ehsan Akhgari <ehsan....@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 Atomic<uint32_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.

[Ehsan Akhgari]

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

[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 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

[Martin Thomson]

On 2014-04-01, at 16:48, Ehsan Akhgari <ehsan....@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;
}

[Ehsan Akhgari]

On 2014-04-01, 8:10 PM, Martin Thomson wrote:
>
> On 2014-04-01, at 16:48, Ehsan Akhgari <ehsan....@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.

Yes, agreed. But I'm claiming this example against the one in my
previous email!

> More verbosity isn’t going to change this. This might:
>
> 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?

Ehsan

[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 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

[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 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

[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 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

[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
> > 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)

[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 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

[Robert O'Callahan]

On Wed, Apr 2, 2014 at 12:11 AM, Joshua Cranmer 🐧 <Pidg...@gmail.com>wrote:

> 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.r"t 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

[Neil]

Would WARN_UNUSED_RESULT help here, so that you remember to use the
result of the -- operator?

--
Warning: May contain traces of nuts.

[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 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::AtomicHandler<T>
mozilla::lock(mozilla::Atomic<T> &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

[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 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 Atomic<T> to make sure it is threadsafe?

Trev

>
> -hb-

[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 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 Atomic<T> 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-

[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, 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 Atomic<T> 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

template<typename 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

[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. <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

[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 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

[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 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

[Joshua Cranmer 🐧]

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.

[Ehsan Akhgari]

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. :-)

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/>

[Martin Thomson]

On 2014-04-02, at 14:01, Ehsan Akhgari <ehsan....@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.

[Benoit Jacob]

2014-04-02 11:03 GMT-04:00 Honza Bambas <honza...@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

[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 on them. I'm actually surprised that
C++11 has global memory functions here, considering that they only
accept atomic<T>* 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::Atomic<bool> 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.

[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 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 atomic<T>* 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::Atomic<bool> 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

[Ehsan Akhgari]

On 2014-04-02, 6:53 PM, Martin Thomson wrote:
> On 2014-04-02, at 14:01, Ehsan Akhgari <ehsan....@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