Implement store as an explicit atomic operation.

Implement store as an explicit atomic operation.

Using explicit atomic operations permits TSan to understand them and
prevents false positives.

Downgrading the atomic Load to acquire semantics. This reduces the
number of memory barriers inserted from two down to one at most.

Also renaming Load/Store to AcquireLoad/ReleaseStore.

BUG=chromium:512382
R=dvyukov@chromium.org, glider@chromium.org
TBR=tommi@webrtc.org

Committed: https://crrev.com/235c35f292f8e225a155d98a018e257a7db442c4
Cr-Commit-Position: refs/heads/master@{#9613}

[pbos-webrtc, 2015-07-21 18:41:35]

pbos@webrtc.org changed reviewers:
+ tommi@webrtc.org

[pbos-webrtc, 2015-07-21 18:41:56]

PTAL, (I'm likely to TBR= tommi@ for this).

[pbos-webrtc, 2015-07-21 18:42:04]

The CQ bit was checked by pbos@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-07-21 18:42:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1246073002/1

[pbos-webrtc, 2015-07-21 19:24:31]

On 2015/07/21 18:42:22, commit-bot: I haz the power wrote:
> Dry run: CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1246073002/1

Looks like I have to implement tsan's macros before submitting this, this is
failing/flaking on tsan with the explicit memory barrier.

[pbos-webrtc, 2015-07-21 19:39:44]

add tsan versions

[pbos-webrtc, 2015-07-21 20:14:29]

PTAL

[Alexander Potapenko, 2015-07-22 11:12:22]

glider@chromium.org changed reviewers:
+ dvyukov@chromium.org

[Alexander Potapenko, 2015-07-22 11:12:23]

As dvyukov@ noticed, it's better to use GCC/Clang __atomic builtins:
https://gcc.gnu.org/onlinedocs/gcc/_005f_005fatomic-Builtins.html
This'll let us have a single implementation for TSan and non-TSan builds,
because TSan understands them.

[Alexander Potapenko, 2015-07-22 11:15:57]

(That may require GCC 4.8, so if that's not an option we can stick to your CL)

[pbos-webrtc, 2015-07-22 12:49:30]

On 2015/07/22 11:15:57, Alexander Potapenko wrote:
> (That may require GCC 4.8, so if that's not an option we can stick to your CL)

Mailed out asking whether we have to support old compilers or not. Should we
land this for now to cooperate with TSan meanwhile?

[Alexander Potapenko, 2015-07-22 12:57:37]

https://codereview.webrtc.org/1246073002/diff/20001/webrtc/base/atomicops.h
File webrtc/base/atomicops.h (right):

https://codereview.webrtc.org/1246073002/diff/20001/webrtc/base/atomicops.h#n...
webrtc/base/atomicops.h:59: static int32_t AcquireLoad(volatile const int32_t*
i) {
It's better for TSan and non-TSan versions of these functions to have matching
prototypes.

[pbos-webrtc, 2015-07-22 13:04:40]

https://codereview.webrtc.org/1246073002/diff/20001/webrtc/base/atomicops.h
File webrtc/base/atomicops.h (right):

https://codereview.webrtc.org/1246073002/diff/20001/webrtc/base/atomicops.h#n...
webrtc/base/atomicops.h:59: static int32_t AcquireLoad(volatile const int32_t*
i) {
On 2015/07/22 12:57:37, Alexander Potapenko wrote:
> It's better for TSan and non-TSan versions of these functions to have matching
> prototypes.

I need the compiler to pick 32/64 bit depending on platform, or are you saying
that I can assume the size of an int when building under TSan?

[Alexander Potapenko, 2015-07-22 13:10:08]

https://codereview.webrtc.org/1246073002/diff/20001/webrtc/base/atomicops.h
File webrtc/base/atomicops.h (right):

https://codereview.webrtc.org/1246073002/diff/20001/webrtc/base/atomicops.h#n...
webrtc/base/atomicops.h:59: static int32_t AcquireLoad(volatile const int32_t*
i) {
On 2015/07/22 13:04:40, pbos-webrtc wrote:
> On 2015/07/22 12:57:37, Alexander Potapenko wrote:
> > It's better for TSan and non-TSan versions of these functions to have
matching
> > prototypes.
> 
> I need the compiler to pick 32/64 bit depending on platform, or are you saying
> that I can assume the size of an int when building under TSan?
Yeah, I was saying the latter. Or you can explicitly convert int to int32_t in
the call to __tsan_atomic32_load():

  static int AcquireLoad(volatile const int* i) {
    return __tsan_atomic32_load((int32_t *)i, __tsan_memory_order_acquire);
  }

[Alexander Potapenko, 2015-07-22 13:11:24]

On 2015/07/22 13:10:08, Alexander Potapenko wrote:
> https://codereview.webrtc.org/1246073002/diff/20001/webrtc/base/atomicops.h
> File webrtc/base/atomicops.h (right):
> 
>
https://codereview.webrtc.org/1246073002/diff/20001/webrtc/base/atomicops.h#n...
> webrtc/base/atomicops.h:59: static int32_t AcquireLoad(volatile const int32_t*
> i) {
> On 2015/07/22 13:04:40, pbos-webrtc wrote:
> > On 2015/07/22 12:57:37, Alexander Potapenko wrote:
> > > It's better for TSan and non-TSan versions of these functions to have
> matching
> > > prototypes.
> > 
> > I need the compiler to pick 32/64 bit depending on platform, or are you
saying
> > that I can assume the size of an int when building under TSan?
> Yeah, I was saying the latter. Or you can explicitly convert int to int32_t in
> the call to __tsan_atomic32_load():
> 
>   static int AcquireLoad(volatile const int* i) {
>     return __tsan_atomic32_load((int32_t *)i, __tsan_memory_order_acquire);
>   }

Also, base/atomicops.h has a notion of Atomic32 and Atomic64, you may want to
adopt them as well.

[pbos-webrtc, 2015-07-22 13:16:43]

gcc/clang builtin atomics

[pbos-webrtc, 2015-07-22 13:21:07]

PTAL, we might have to revert it if alarms go sounding off, but it looks worth
trying since this solution is a lot cleaner.

I want to avoid the atomic 32/64:s because our usage of AtomicOps isn't very
fancy and very limited. Feels best to do it if we actually need them and keep
our implementation simple.

[pbos-webrtc, 2015-07-22 13:23:42]

The CQ bit was checked by pbos@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-07-22 13:23:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1246073002/40001

[Alexander Potapenko, 2015-07-22 13:43:00]

LGTM

https://codereview.webrtc.org/1246073002/diff/40001/webrtc/video/video_captur...
File webrtc/video/video_capture_input.cc (right):

https://codereview.webrtc.org/1246073002/diff/40001/webrtc/video/video_captur...
webrtc/video/video_capture_input.cc:66: rtc::AtomicOps::Increment(&stop_);
Isn't rtc::AtomicOps::ReleaseStore(&stop, 1) enough here?

[pbos-webrtc, 2015-07-22 13:50:45]

s/Increment/ReleaseStore

[pbos-webrtc, 2015-07-22 13:51:04]

https://codereview.webrtc.org/1246073002/diff/40001/webrtc/video/video_captur...
File webrtc/video/video_capture_input.cc (right):

https://codereview.webrtc.org/1246073002/diff/40001/webrtc/video/video_captur...
webrtc/video/video_capture_input.cc:66: rtc::AtomicOps::Increment(&stop_);
On 2015/07/22 13:43:00, Alexander Potapenko wrote:
> Isn't rtc::AtomicOps::ReleaseStore(&stop, 1) enough here?

Done.

[Dmitry Vyukov, 2015-07-22 14:06:47]

LGTM

Yes, load and store are faster with this change. In particular, load is way more
scalable as it does not write to memory, this allows several CPUs to have the
cache line is shared state.

[pbos-webrtc, 2015-07-22 14:25:59]

Thanks, TBR=tommi@ for landing since he's OOO and he's the one I want to take a
look here.

[pbos-webrtc, 2015-07-22 14:26:04]

The CQ bit was checked by pbos@webrtc.org

[pbos-webrtc, 2015-07-22 14:26:04]

The patchset sent to the CQ was uploaded after l-g-t-m from glider@chromium.org
Link to the patchset: https://codereview.webrtc.org/1246073002/#ps60001 (title:
"s/Increment/ReleaseStore")

[commit-bot: I haz the power, 2015-07-22 14:26:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1246073002/60001

[commit-bot: I haz the power, 2015-07-22 15:35:03]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-07-22 15:35:10]

Patchset 4 (id:??) landed as
https://crrev.com/235c35f292f8e225a155d98a018e257a7db442c4
Cr-Commit-Position: refs/heads/master@{#9613}