Proposal: Allow std::atomic et al. from <atomic>

[Jeremy Roman]

I've seen two CLs using it in the past few days, so it seems like people desperately want it. :)

https://chromium.googlesource.com/chromium/src/+/0d030275667855c06be6cfc2ac10fb434d7ed771 (reverted for unrelated reasons)

https://chromium.googlesource.com/chromium/src/+/5e58730d6fe40ef7cead75954ef5b1c05dea6d4b

and media/midi/midi_manager_win.cc has multiple TODOs asking for code to be replaced with std::atomic

[Brett Wilson]

I'm curios to what extent this can replace base/atomicops.h. Can we replace all of our file, or are there important parts left?

Brett

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACuR13cJ9LZPtnoXq%2Bay0CCvzBt3wVR2i1-KsTbOGcU0%2BtAy1A%40mail.gmail.com.

[Jeremy Roman]

Given how trivial base/atomicops_internals_portable.h (which uses <atomic>) is, I'd assume that if <atomic> works well on all of our platforms, we can use it directly where we use base/atomicops.h today. (I admit, however, that I write mostly on the Blink side and could be overlooking something.)

[Nico Weber]

There's a bug somewhere for using atomicops_portable somewhere and jfb has a cl for that on his rietveld page. It had a few red bots, but I think nobody investigated them yet in detail. It'd be good to figure out the problems there first.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACuR13dhVRg%2BmKzZXtkRk%2BiCipGb_FgUciVr7ST4Ctn%3Dz_8WRw%40mail.gmail.com.

[JF Bastien]

On Sun, Dec 6, 2015 at 5:49 AM, Nico Weber <tha...@google.com> wrote:

There's a bug somewhere for using atomicops_portable somewhere and jfb has a cl for that on his rietveld page. It had a few red bots, but I think nobody investigated them yet in detail. It'd be good to figure out the problems there first.

Indeed!

Bug: https://code.google.com/p/chromium/issues/detail?id=559247

Patch: https://codereview.chromium.org/1466833002/

I ran into issues for Windows, so I was waiting for jschuh to try his own patch out, but he had to revert it. pbos had similar Windows issues for WebRTC (accidental dependencies on windows.h). What I think we should do is:

• I'll try my patch without moving Windows for now.
• If that works then we can change Windows uses one by one (adding windows.h where it's expected). I'd appreciate some help there because I don't have a Windows machine, and using the bots for that is painfully slow.
• Migrate atomicops uses individually to std::atomic. I can volunteer to do that, it's a bit tricky in some cases, you can't just move all of them in one go. Some uses don't map cleanly, the API is slightly different, ...
  
• New uses of std::atomic should be fine, though they still need proper review to make sure they're actually correct. Atomics are a dangerous tool!

Does that sound good?

[Nico Weber]

We just discovered that std::atomic<> requires a real constructor call on Windows with MSVS2013 (since that doesn't have constexpr yet, so the standard library can't use it yet). So if you use a global std::atomic<int> then that's a static initializer. (Funnily enough, if you use a std::atomic_int then everything's fine.)

So it looks like general adoption of std::atomic<> is blocked on msvs2015 too :-(

[Yuri Wiitala]

I don't feel we should block it based on this.  There are plenty of cases where std::atomic<> is not used as a global variable type.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAMGbLiHH1yHv36aDopXV3%2Bfo9nmmFrGRc3DBD5Enc24fScgiZA%40mail.gmail.com.

[Antoine Labour]

On Mon, Dec 7, 2015 at 1:11 PM, Yuri Wiitala <m...@chromium.org> wrote:

I don't feel we should block it based on this.  There are plenty of cases where std::atomic<> is not used as a global variable type.

I for one am not a huge fan of std::atomic turning up all over the code base.

This stuff is really subtle, misuse can cause data races that become very platform-dependent (because ARM's memory model is weaker than x86's). They're very often misunderstood - I reviewed quite a few CLs using base::subtle::Atomic*, and more often than not they were incorrect, or unnecessarily conservative.

IMO the only legitimate way to use them is to create a higher order primitive, that is well reviewed and extremely well tested.

But we should be extremely suspicious of random uses in arbitrary places.

The fact that we're considering using std::atomic as a global variable is worrying. Such code is typically hard to test.

Antoine

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CA%2BN%2BEKYRSaj9b%2Bw1etQHobQzO6saUKze%3DhFa6Mrd1MkUX2wUtg%40mail.gmail.com.

[Nico Weber]

(Context for my comment was https://codereview.chromium.org/1463683002/ which modified blink's SpinLock implementation, and there are a few global SpinLocks.)

[JF Bastien]

On Mon, Dec 7, 2015 at 10:48 PM, Antoine Labour <pi...@google.com> wrote:

On Mon, Dec 7, 2015 at 1:11 PM, Yuri Wiitala <m...@chromium.org> wrote:

I don't feel we should block it based on this.  There are plenty of cases where std::atomic<> is not used as a global variable type.

I for one am not a huge fan of std::atomic turning up all over the code base.

This stuff is really subtle, misuse can cause data races that become very platform-dependent (because ARM's memory model is weaker than x86's). They're very often misunderstood - I reviewed quite a few CLs using base::subtle::Atomic*, and more often than not they were incorrect, or unnecessarily conservative.

IMO the only legitimate way to use them is to create a higher order primitive, that is well reviewed and extremely well tested.

But we should be extremely suspicious of random uses in arbitrary places.

Agreed, but that's even more true of base:subtle::Atomic than it is of std::atomic.

The fact that we're considering using std::atomic as a global variable is worrying. Such code is typically hard to test.

Global locks make sense in many cases, and are often easier to wrap your mind around than dynamically allocated ones.

On Mon, Dec 7, 2015 at 10:49 AM, 'Nico Weber' via cxx <c...@chromium.org> wrote:

We just discovered that std::atomic<> requires a real constructor call on Windows with MSVS2013 (since that doesn't have constexpr yet, so the standard library can't use it yet). So if you use a global std::atomic<int> then that's a static initializer. (Funnily enough, if you use a std::atomic_int then everything's fine.)

So it looks like general adoption of std::atomic<> is blocked on msvs2015 too :-(

The migration I'm doing now should be unaffected by this.

The issue jschuh ran into is indeed annoying. I think there may be a way to work around it that's not too ugly.

[Nico Weber]

On Mon, Dec 7, 2015 at 5:14 PM, JF Bastien <j...@chromium.org> wrote:

On Mon, Dec 7, 2015 at 10:48 PM, Antoine Labour <pi...@google.com> wrote:

On Mon, Dec 7, 2015 at 1:11 PM, Yuri Wiitala <m...@chromium.org> wrote:

I don't feel we should block it based on this.  There are plenty of cases where std::atomic<> is not used as a global variable type.

I for one am not a huge fan of std::atomic turning up all over the code base.

This stuff is really subtle, misuse can cause data races that become very platform-dependent (because ARM's memory model is weaker than x86's). They're very often misunderstood - I reviewed quite a few CLs using base::subtle::Atomic*, and more often than not they were incorrect, or unnecessarily conservative.

IMO the only legitimate way to use them is to create a higher order primitive, that is well reviewed and extremely well tested.

But we should be extremely suspicious of random uses in arbitrary places.

Agreed, but that's even more true of base:subtle::Atomic than it is of std::atomic.

The fact that we're considering using std::atomic as a global variable is worrying. Such code is typically hard to test.

Global locks make sense in many cases, and are often easier to wrap your mind around than dynamically allocated ones.

On Mon, Dec 7, 2015 at 10:49 AM, 'Nico Weber' via cxx <c...@chromium.org> wrote:

We just discovered that std::atomic<> requires a real constructor call on Windows with MSVS2013 (since that doesn't have constexpr yet, so the standard library can't use it yet). So if you use a global std::atomic<int> then that's a static initializer. (Funnily enough, if you use a std::atomic_int then everything's fine.)

So it looks like general adoption of std::atomic<> is blocked on msvs2015 too :-(

The migration I'm doing now should be unaffected by this.

The issue jschuh ran into is indeed annoying. I think there may be a way to work around it that's not too ugly.

Yes, we worked around that by using an std::atomic_int which doesn't have the issue that std::atomic<int> has.

[Antoine Labour]

On Mon, Dec 7, 2015 at 2:14 PM, JF Bastien <j...@chromium.org> wrote:

On Mon, Dec 7, 2015 at 10:48 PM, Antoine Labour <pi...@google.com> wrote:

On Mon, Dec 7, 2015 at 1:11 PM, Yuri Wiitala <m...@chromium.org> wrote:

I don't feel we should block it based on this.  There are plenty of cases where std::atomic<> is not used as a global variable type.

I for one am not a huge fan of std::atomic turning up all over the code base.

This stuff is really subtle, misuse can cause data races that become very platform-dependent (because ARM's memory model is weaker than x86's). They're very often misunderstood - I reviewed quite a few CLs using base::subtle::Atomic*, and more often than not they were incorrect, or unnecessarily conservative.

IMO the only legitimate way to use them is to create a higher order primitive, that is well reviewed and extremely well tested.

But we should be extremely suspicious of random uses in arbitrary places.

Agreed, but that's even more true of base:subtle::Atomic than it is of std::atomic.

The fact that we're considering using std::atomic as a global variable is worrying. Such code is typically hard to test.

Global locks make sense in many cases, and are often easier to wrap your mind around than dynamically allocated ones.

Global locks don't bother me per se (well, beyond global state in general), provided the lock implementation is well tested.

std::atomic<T> is not a lock, though.

Antoine

[JF Bastien]

On Mon, Dec 7, 2015 at 11:47 PM, Antoine Labour <pi...@google.com> wrote:

On Mon, Dec 7, 2015 at 2:14 PM, JF Bastien <j...@chromium.org> wrote:

On Mon, Dec 7, 2015 at 10:48 PM, Antoine Labour <pi...@google.com> wrote:

On Mon, Dec 7, 2015 at 1:11 PM, Yuri Wiitala <m...@chromium.org> wrote:

I don't feel we should block it based on this.  There are plenty of cases where std::atomic<> is not used as a global variable type.

I for one am not a huge fan of std::atomic turning up all over the code base.

This stuff is really subtle, misuse can cause data races that become very platform-dependent (because ARM's memory model is weaker than x86's). They're very often misunderstood - I reviewed quite a few CLs using base::subtle::Atomic*, and more often than not they were incorrect, or unnecessarily conservative.

IMO the only legitimate way to use them is to create a higher order primitive, that is well reviewed and extremely well tested.

But we should be extremely suspicious of random uses in arbitrary places.

Agreed, but that's even more true of base:subtle::Atomic than it is of std::atomic.

The fact that we're considering using std::atomic as a global variable is worrying. Such code is typically hard to test.

Global locks make sense in many cases, and are often easier to wrap your mind around than dynamically allocated ones.

Global locks don't bother me per se (well, beyond global state in general), provided the lock implementation is well tested.

std::atomic<T> is not a lock, though.

Definitely agree. Sanity there is just enforced through code review though, right? I don't know if we want something heavier than that. It may be good to flag atomic usage for review, WDYT?

[Yuri Wiitala]

How about a PRESUBMIT that requires an lgtm from a certain set of trusted reviewers?  I don't expect std::atomic to be used that often, but there are legitimate use cases (e.g., lockless concurrency on real-time threads for media).

[Antoine Labour]

On Mon, Dec 7, 2015 at 3:02 PM, JF Bastien <j...@chromium.org> wrote:

On Mon, Dec 7, 2015 at 11:47 PM, Antoine Labour <pi...@google.com> wrote:

On Mon, Dec 7, 2015 at 2:14 PM, JF Bastien <j...@chromium.org> wrote:

On Mon, Dec 7, 2015 at 10:48 PM, Antoine Labour <pi...@google.com> wrote:

On Mon, Dec 7, 2015 at 1:11 PM, Yuri Wiitala <m...@chromium.org> wrote:

I don't feel we should block it based on this.  There are plenty of cases where std::atomic<> is not used as a global variable type.

I for one am not a huge fan of std::atomic turning up all over the code base.

This stuff is really subtle, misuse can cause data races that become very platform-dependent (because ARM's memory model is weaker than x86's). They're very often misunderstood - I reviewed quite a few CLs using base::subtle::Atomic*, and more often than not they were incorrect, or unnecessarily conservative.

IMO the only legitimate way to use them is to create a higher order primitive, that is well reviewed and extremely well tested.

But we should be extremely suspicious of random uses in arbitrary places.

Agreed, but that's even more true of base:subtle::Atomic than it is of std::atomic.

The fact that we're considering using std::atomic as a global variable is worrying. Such code is typically hard to test.

Global locks make sense in many cases, and are often easier to wrap your mind around than dynamically allocated ones.

Global locks don't bother me per se (well, beyond global state in general), provided the lock implementation is well tested.

std::atomic<T> is not a lock, though.

Definitely agree. Sanity there is just enforced through code review though, right? I don't know if we want something heavier than that. It may be good to flag atomic usage for review, WDYT?

Review by whom? That CL ( https://codereview.chromium.org/1463683002/ ) indicates that the previous version may have a race. Yet I'm sure it was reviewed.

My suggestion would be to have all code that uses std::atomic be written in a specific directory (say, under base/atomic), with a good set of OWNERs. It would encourage reuse of higher-level primitives, and raise the bar WRT using atomics just because "locks are slow" (they usually aren't). Caveat venditor to produce data showing that atomics are indeed needed.

I would also point to the Google-only internal C++ primer (https://engdoc.corp.google.com/eng/doc/devguide/cpp/primer.shtml?cl=head#atomics ), which has some good restrictions.

Antoine

[JF Bastien]

On Tue, Dec 8, 2015 at 12:22 AM, Antoine Labour <pi...@google.com> wrote:

On Mon, Dec 7, 2015 at 3:02 PM, JF Bastien <j...@chromium.org> wrote:

On Mon, Dec 7, 2015 at 11:47 PM, Antoine Labour <pi...@google.com> wrote:

On Mon, Dec 7, 2015 at 2:14 PM, JF Bastien <j...@chromium.org> wrote:

On Mon, Dec 7, 2015 at 10:48 PM, Antoine Labour <pi...@google.com> wrote:

On Mon, Dec 7, 2015 at 1:11 PM, Yuri Wiitala <m...@chromium.org> wrote:

I don't feel we should block it based on this.  There are plenty of cases where std::atomic<> is not used as a global variable type.

I for one am not a huge fan of std::atomic turning up all over the code base.

This stuff is really subtle, misuse can cause data races that become very platform-dependent (because ARM's memory model is weaker than x86's). They're very often misunderstood - I reviewed quite a few CLs using base::subtle::Atomic*, and more often than not they were incorrect, or unnecessarily conservative.

IMO the only legitimate way to use them is to create a higher order primitive, that is well reviewed and extremely well tested.

But we should be extremely suspicious of random uses in arbitrary places.

Agreed, but that's even more true of base:subtle::Atomic than it is of std::atomic.

The fact that we're considering using std::atomic as a global variable is worrying. Such code is typically hard to test.

Global locks make sense in many cases, and are often easier to wrap your mind around than dynamically allocated ones.

Global locks don't bother me per se (well, beyond global state in general), provided the lock implementation is well tested.

std::atomic<T> is not a lock, though.

Definitely agree. Sanity there is just enforced through code review though, right? I don't know if we want something heavier than that. It may be good to flag atomic usage for review, WDYT?

Review by whom? That CL ( https://codereview.chromium.org/1463683002/ ) indicates that the previous version may have a race. Yet I'm sure it was reviewed.

My suggestion would be to have all code that uses std::atomic be written in a specific directory (say, under base/atomic), with a good set of OWNERs. It would encourage reuse of higher-level primitives, and raise the bar WRT using atomics just because "locks are slow" (they usually aren't). Caveat venditor to produce data showing that atomics are indeed needed.

I would also point to the Google-only internal C++ primer (https://engdoc.corp.google.com/eng/doc/devguide/cpp/primer.shtml?cl=head#atomics ), which has some good restrictions.

Yuri's suggestion seems sensible:

On Tue, Dec 8, 2015 at 12:09 AM, Yuri Wiitala <m...@chromium.org> wrote:

How about a PRESUBMIT that requires an lgtm from a certain set of trusted reviewers?  I don't expect std::atomic to be used that often, but there are legitimate use cases (e.g., lockless concurrency on real-time threads for media).

 

It certainly won't avoid all mistakes, but would allow enforcing what Antoine seems concerned about (which I don't think can otherwise be enforced programmatically).

I'm not a huge fan of forcing atomics into a single directory, but it could work out.

[Dana Jansens]

I don't really like the idea of introducing per-feature owners. These won't be as discoverable or understandable as our current OWNERS structure.

 

 

It certainly won't avoid all mistakes, but would allow enforcing what Antoine seems concerned about (which I don't think can otherwise be enforced programmatically).

I'm not a huge fan of forcing atomics into a single directory, but it could work out.

Doing it by banning it outside of a direction would resolve my above concerns about OWNERships.

[JF Bastien]

I don't think I've expressed my concern clearly: I'm volunteering to migrate existing uses of atomicops.h to std::atomic, one by one, with all the caveats this implies, unless someone beats me to individual uses. That model doesn't fit well with an extra constraint where all uses of std::atomic must be in a new subdirectory: I wouldn't just be changing the API code uses and fixing individual bugs, I'd also be moving code around which causes friction with the owner of the code as well as with whoever owns the special atomic directory. That's a situation in which the pain threshold to migrate code is too high IMO.

I don't have a strong opinion on new code that uses std::atomic, but I strongly believe that pre-existing uses of atomicops.h should get a free upgrade. If someone volunteers to move all atomicops.h users to the blessed directory then I can go on with my plan, but if I have to both upgrade and move the code then honestly it's a no-go for me. I'd rather piss off just one person by upgrading their code and telling them how wrong it was, rather than pissing off two people by also bringing to their attention new uses of atomics that they don't like.

This is why I'm suggesting a presubmit check: realistically we already have uses of atomics (call it std::atomic or atomicops.h), the simplest thing to do is to avoid further regressions while slowly improving the mess we're in. I'm quite fine with low discoverability and understandability: these are two bars one already has to cross when attempting to use atomics!

[Yuri Wiitala]

On Tue, Dec 8, 2015 at 1:46 PM, Dana Jansens <dan...@chromium.org> wrote:

Yuri's suggestion seems sensible:

On Tue, Dec 8, 2015 at 12:09 AM, Yuri Wiitala <m...@chromium.org> wrote:

How about a PRESUBMIT that requires an lgtm from a certain set of trusted reviewers?  I don't expect std::atomic to be used that often, but there are legitimate use cases (e.g., lockless concurrency on real-time threads for media).

I don't really like the idea of introducing per-feature owners. These won't be as discoverable or understandable as our current OWNERS structure.

Agreed that it's not perfect, but the alternative won't help (see below).

I'm not a huge fan of forcing atomics into a single directory, but it could work out.

Doing it by banning it outside of a direction would resolve my above concerns about OWNERships.

While Antoine suggested writing higher-level primitives in base/ (which I agree should always be "plan A"), there are cases where you are writing lockless code and nothing more complex than an std::atomic<>::load() or store() will suffice.  Yes, you could create inline wrappers in base and enforce via DEPS rules, but that would only enforce an OWNER review for the first CL in a dir that starts to use atomics.  (Actually, this is what we have in base/atomic*.h already.)

I'm not sure what to suggest here.  At some point, we have to trust developers to do the right thing; and developers will certainly make mistakes w/ atomics; and, yes, that's dangerous when they do; but those that know how to use them and have a valid reason for doing so shouldn't be banned from using them.

[Kim Gräsman]

Here's a toot from the peanut gallery.

In another thread, there was mention that you're still using libstdc++4.6.

I work on a large non-Chromium codebase also using the same libstdc++
version, and we moved off std::atomic because of a subtle weakness.

We saw occasional complete system hangs when using std::atomic from a
high-priority thread. It turned out that libstdc++'s atomic
implementation for ARM spins wildly, so it can cause starvation if
used from a high-priority thread. Other atomic implementations for
Linux/ARM use a syscall to preempt [1].

I'm pretty sure this would not be a problem for you (I'm guessing you
don't toy around with thread priorities), but I thought I'd at least
bring it to the table.

IIRC, later libstdc++/GCC versions use this implementation out of the
box, but 4.6 didn't have it.

FWIW,
- Kim

[1] http://lwn.net/Articles/314561/

[Antoine Labour]

On Wed, Dec 9, 2015 at 8:11 AM, Yuri Wiitala <m...@chromium.org> wrote:

On Tue, Dec 8, 2015 at 1:46 PM, Dana Jansens <dan...@chromium.org> wrote:

Yuri's suggestion seems sensible:

On Tue, Dec 8, 2015 at 12:09 AM, Yuri Wiitala <m...@chromium.org> wrote:

How about a PRESUBMIT that requires an lgtm from a certain set of trusted reviewers?  I don't expect std::atomic to be used that often, but there are legitimate use cases (e.g., lockless concurrency on real-time threads for media).

I don't really like the idea of introducing per-feature owners. These won't be as discoverable or understandable as our current OWNERS structure.

Agreed that it's not perfect, but the alternative won't help (see below).

I'm not a huge fan of forcing atomics into a single directory, but it could work out.

Doing it by banning it outside of a direction would resolve my above concerns about OWNERships.

While Antoine suggested writing higher-level primitives in base/ (which I agree should always be "plan A"), there are cases where you are writing lockless code and nothing more complex than an std::atomic<>::load() or store() will suffice.

What are those cases, do you have a link, and strong evidence that the complexity is warranted? Do you believe you have adequate testing and review? (See below).

 

  Yes, you could create inline wrappers in base and enforce via DEPS rules, but that would only enforce an OWNER review for the first CL in a dir that starts to use atomics.  (Actually, this is what we have in base/atomic*.h already.)

Well, using something from base::subtle immediately raises alarm bells with any reviewer today. It needs to be the same with std::atomic.

 

I'm not sure what to suggest here.  At some point, we have to trust developers to do the right thing; and developers will certainly make mistakes w/ atomics; and, yes, that's dangerous when they do; but those that know how to use them and have a valid reason for doing so shouldn't be banned from using them.

What about maintenance, if you move on from your project?

What about readability, when inevitably someone else than you finds a bug and gets stuck with the task of untangling the atomics?

Please read https://docs.google.com/document/d/1SiohmdKXfuxALrQSh9zjTmhCQooYNR9U9q_57TSzg2Q/edit (sorry, internal document again), written by very smart engineers, about the dangers of atomic operations, and with links to actual bugs in goolge3 code directly caused by the use of atomics, some of which were never found by humans. Here is their conclusion/recommendation:

"So any non-trivial lock-free algorithm requires careful review by several experts, verification with formal checkers and exhaustive stress testing on different hardware for starters."

They also list common misconceptions about the relative cost of mutex and atomic operations (compared to, say, cache misses), as well as links to alternative techniques.

Antoine

[Ojan Vafai]

Naive question: What do we gain from allowing std::atomic that we don't already have in base/atomicops.h?

My read of this thread so far is that it's got at least one or two footguns and we're considering complicated rules for allowing its usage. Could we just keep using atomicops.h instead? Then the rule is simple and easily enforcable by tooling.

--

You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAMeTaZcWXzywP8aqUdgfR83y_DA1qKiWhWoD8P8SauTy%2BUx%2BqA%40mail.gmail.com.

[Jeffrey Yasskin]

In https://code.google.com/p/chromium/codesearch/#chromium/src/base/memory/singleton.h&l=239, for example, we have to reinterpret_cast back and forth between AtomicWord and T*. std::atomic is templated on the type of the contents, so you don't have to cast as much.

base/atomicops.h also has a couple operations (Release_Load and Acquire_Store) that don't really match the memory model modern compilers are using. Their semantics are pretty much only specified by their implementations.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CANMdWTuPFtpWUdnzeanyxTTrAA397nTWP4fL-KTb_ngfBWzcBA%40mail.gmail.com.

[Nico Weber]

I think there are two parts to this:

1. It probably makes sense to implement base/atomicops.h on top of <atomic> -- it's less code (currently we have per-arch assembly), compilers know about <atomic> and can produce better code, etc.

2. Once we've done this, should we keep using base/atomicops.h instead of <atomic>? This is maybe less clear. The atomicops API offers less control over memory ordering, so it might be a bit easier to use. Also, as Antoine points out, base::subtle:: looks more appropriately scary than std::

Maybe it makes sense to only do 1, and then if needed it's easy to modify the atomicops api to address issues it might have.

[JF Bastien]

On Wed, Dec 16, 2015 at 11:19 AM, Jeffrey Yasskin <jyas...@chromium.org> wrote:

In https://code.google.com/p/chromium/codesearch/#chromium/src/base/memory/singleton.h&l=239, for example, we have to reinterpret_cast back and forth between AtomicWord and T*. std::atomic is templated on the type of the contents, so you don't have to cast as much.

base/atomicops.h also has a couple operations (Release_Load and Acquire_Store) that don't really match the memory model modern compilers are using. Their semantics are pretty much only specified by their implementations.

A few more advantages are listed in http://wg21.link/n4455

On Wed, Dec 16, 2015 at 11:30 AM, Nico Weber <tha...@google.com> wrote:

I think there are two parts to this:

1. It probably makes sense to implement base/atomicops.h on top of <atomic> -- it's less code (currently we have per-arch assembly), compilers know about <atomic> and can produce better code, etc.

That's almost the case though. The protable atomic ops are used almost everywhere, and I'll (slowly) try to fix the rest.

2. Once we've done this, should we keep using base/atomicops.h instead of <atomic>? This is maybe less clear. The atomicops API offers less control over memory ordering, so it might be a bit easier to use. Also, as Antoine points out, base::subtle:: looks more appropriately scary than std::

Maybe it makes sense to only do 1, and then if needed it's easy to modify the atomicops api to address issues it might have.

That can also be handled through automated style guide checks, e.g. we shouldn't allow memory_order_consume, and we should frown very sternly at memory_order_relaxed.

"std::" isn't scary, but "atomic" sure is!

It's also a question of writing idiomatic code which other projects can use, and undoing the sins that I committed in the portable atomicops (it's fine for now, but compilers may start being nasty in the future).

[Nico Weber]

Current status here: I tried to switch tcmalloc's atomics from custom atomicops to <atomic> on linux and it regressed perf on a bunch of bots (http://crbug.com/572525 – one dupe is 3% on sunspider). Until this is understood better, we shouldn't use <atomics>.

[jun...@google.com]

FYI, RE2 will soon be changing to C++11 atomics. Unlike in Chromium, however, the code in util/atomicops.h has "modern" GCC/Clang using the built-in __atomic_* functions and everything else using memory barriers that are questionable at best, so std::atomic<int> and std::atomic<T*> should be an improvement, but please let me know if you observe performance regressions after the change lands in your repository. Thanks!

[Nico Weber]

If RE2 will switch to std::atomics, we won't update RE2 until we understand http://crbug.com/572525 better.

[Paul Wankadia]

On Sat, Feb 6, 2016 at 8:26 AM, Nico Weber <tha...@google.com> wrote:

If RE2 will switch to std::atomics, we won't update RE2 until we understand http://crbug.com/572525 better.

Fair enough.

Tangentially, updating till f09aa93 will get you a couple of recent optimisations. :)

[jma...@google.com]

Thoughts/objections to ANGLE using std::atomic? Is std::atomic working well on the latest VS2015 update? We don't have access to the libraries Chromium uses for atomics currently, and it would be nice to not have to import them as well.

[Nico Weber]

It's currently a perf problem on arm. I think there was a bug linked upthread.

[Jamie Madill]

ANGLE doesn't ship on any ARM devices. It seems like it shouldn't be too objectionable.

[Nico Weber]

I remember spending a long time debugging android/x64 breaking due to an incompatibility between angle and android's libc++. Are we shipping angle in chrome/android/x64 but not in chrome/android/arm?

[Jamie Madill]

I don't actually know - we don't ship on ARM and won't probably for the forseeable future.

[dan...@chromium.org]

On Fri, Sep 23, 2016 at 11:17 AM, Jamie Madill <jma...@google.com> wrote:

I don't actually know - we don't ship on ARM and won't probably for the forseeable future.

This is the bug referencing angle on android/intel: https://bugs.chromium.org/p/chromium/issues/detail?id=487341

[Dmitry Skiba]

Hmm, I definitely fixed something in ANGLE to reduce memory usage on Android: https://chromium-review.googlesource.com/#/c/281411/

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAHtyhaSLHvEZYq-34bnRSLY4nvj9AQfUnxSBgBrh8XHs1ZETqg%40mail.gmail.com.

[Jamie Madill]

Oh, sorry. That's the shader translator. I need the atomic for caching histograms on Windows - in the ANGLE rendering component. The shader translator lives by different rules, it's used in other browsers and OSes.

We ship the rendering component on Windows only right now, expanding to Linux and Mac hopefully within the next couple of quarters, but mobile is far away.

Same goes for Dmitri's response.

[ser...@google.com]

Any updates on whether std::atomic should be allowed or not?

On one hand it's still listed as banned at https://chromium-cpp.appspot.com/

On the other hand I've found a few places where it's already being used in base:

https://cs.chromium.org/search/?q=%22std::atomic%3C%22+file:base+package:%5Echromium$&type=cs

[Jeremy Roman]

On Wed, May 17, 2017 at 6:15 PM, servolk via cxx <c...@chromium.org> wrote:

Any updates on whether std::atomic should be allowed or not?

On one hand it's still listed as banned at https://chromium-cpp.appspot.com/

On the other hand I've found a few places where it's already being used in base:

https://cs.chromium.org/search/?q=%22std::atomic%3C%22+file:base+package:%5Echromium$&type=cs

The latest issue I'm aware of was a revert when thakis tried to reland use of std::atomic in tcmalloc:

https://codereview.chromium.org/2818713003

I'm a little surprised it made it into base (aside from the implementation of atomicops.h) already, so I'm not really sure whether we ought to move it to the approved features in recognition of that.

Regardless, <atomic> should remain a rather uncommon feature, with higher-level abstractions used in most places.

On Saturday, December 5, 2015 at 2:09:58 PM UTC-8, Jeremy Roman wrote:

I've seen two CLs using it in the past few days, so it seems like people desperately want it. :)

https://chromium.googlesource.com/chromium/src/+/0d030275667855c06be6cfc2ac10fb434d7ed771 (reverted for unrelated reasons)

https://chromium.googlesource.com/chromium/src/+/5e58730d6fe40ef7cead75954ef5b1c05dea6d4b

and media/midi/midi_manager_win.cc has multiple TODOs asking for code to be replaced with std::atomic

--

You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/0f4b0379-c9a7-4dd5-9f5f-92db90764e64%40chromium.org.

[Primiano Tucci]

AFAIK there is also a performance problem there (nobarrier semantics get implemented with full fences) on Linux due to the obsolete sysroot. crbug.com/592903 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACuR13ft8%3Dpi7coqV0VmpaK%2BKqseQYUcWQbyJ6Jw0jphDUepWw%40mail.gmail.com.

[Jeremy Roman]

On Thu, Jun 1, 2017 at 1:38 PM, Primiano Tucci <prim...@google.com> wrote:

AFAIK there is also a performance problem there (nobarrier semantics get implemented with full fences) on Linux due to the obsolete sysroot. crbug.com/592903 

I think we've finished upgrading the sysroot (https://bugs.chromium.org/p/chromium/issues/detail?id=697494).