Replace Acquire_Store calls with Release_Store as these stores should have the release ordering/sem…

Replace Acquire_Store calls with Release_Store as these stores should have the release ordering/semantics. Also remove a couple of unnecessary memory barriers.

BUG=420970
Committed: https://crrev.com/a278594a43c76544cb762ad3ded470664fb9c465
Cr-Commit-Position: refs/heads/master@{#298618}

[lcwu1, 2014-10-07 19:08:38]

lcwu@chromium.org changed reviewers:
+ damienv@chromium.org

[damienv1, 2014-10-07 19:22:31]

https://codereview.chromium.org/631283002/diff/1/chromecast/media/cma/ipc/med...
File chromecast/media/cma/ipc/media_message_fifo.cc (right):

https://codereview.chromium.org/631283002/diff/1/chromecast/media/cma/ipc/med...
chromecast/media/cma/ipc/media_message_fifo.cc:373: // Make sure the read
pointer has been updated before sending a notification.
The comment should be updated (just to mention it's safe to send the
notification: the other thread will get the latest value for the read offset
through an Acquire_Load).

https://codereview.chromium.org/631283002/diff/1/chromecast/media/cma/ipc/med...
chromecast/media/cma/ipc/media_message_fifo.cc:384: // Make sure the write
pointer has been updated before sending a notification.
Same: update the comment.
Also, could you please make sure that running TSAN on the unit test is still
fine.

[servolk, 2014-10-07 19:43:37]

servolk@chromium.org changed reviewers:
+ servolk@chromium.org

[servolk, 2014-10-07 19:43:37]

https://codereview.chromium.org/631283002/diff/1/chromecast/media/cma/ipc/med...
File chromecast/media/cma/ipc/media_message_fifo.cc (right):

https://codereview.chromium.org/631283002/diff/1/chromecast/media/cma/ipc/med...
chromecast/media/cma/ipc/media_message_fifo.cc:126:
base::subtle::Release_Store(rd_offset_, 0);
The bug referenced in the description
(https://code.google.com/p/chromium/issues/detail?id=420970) talks about
removing BOTH Acquire_Store and Release_Store and replacing them with proper
C++11 atomics. So why not use C++11 atomics here (std::atomic_store, I suppose)?
Or is that something that Chromecast can't support yet due to being on older
GCC?

[lcwu1, 2014-10-07 21:30:17]

https://codereview.chromium.org/631283002/diff/1/chromecast/media/cma/ipc/med...
File chromecast/media/cma/ipc/media_message_fifo.cc (right):

https://codereview.chromium.org/631283002/diff/1/chromecast/media/cma/ipc/med...
chromecast/media/cma/ipc/media_message_fifo.cc:126:
base::subtle::Release_Store(rd_offset_, 0);
On 2014/10/07 19:43:37, servolk wrote:
> The bug referenced in the description
> (https://code.google.com/p/chromium/issues/detail?id=420970) talks about
> removing BOTH Acquire_Store and Release_Store and replacing them with proper
> C++11 atomics. So why not use C++11 atomics here (std::atomic_store, I
suppose)?
> Or is that something that Chromecast can't support yet due to being on older
> GCC?

No, the bug was talking about removing Acquire_Store and Release_Load (not store
here) because the new C++ memory model doesn't define their behavior. (C++
memory model only defines the behavior of Release_Store and Acquire_Load, both
of which are most commonly used.)

Basically the upstream is working on replacing the implementation of
base/atomicops operations with C++11's new std::atomics so that the existing
calls to base/atomicops don't have to change. But our code would stop to work if
we don't upgrade to a new toolchain that supports C++11 atomics.

https://codereview.chromium.org/631283002/diff/1/chromecast/media/cma/ipc/med...
chromecast/media/cma/ipc/media_message_fifo.cc:373: // Make sure the read
pointer has been updated before sending a notification.
On 2014/10/07 19:22:31, damienv1 wrote:
> The comment should be updated (just to mention it's safe to send the
> notification: the other thread will get the latest value for the read offset
> through an Acquire_Load).

Done.

https://codereview.chromium.org/631283002/diff/1/chromecast/media/cma/ipc/med...
chromecast/media/cma/ipc/media_message_fifo.cc:384: // Make sure the write
pointer has been updated before sending a notification.
On 2014/10/07 19:22:30, damienv1 wrote:
> Same: update the comment.
> Also, could you please make sure that running TSAN on the unit test is still
> fine.

Ran Tsan on cast_media_unittests and everything is fine. (To make tsan run, I
had to fix a couple of issues in ozone, which I will prepare another CL for.)

[damienv1, 2014-10-07 23:30:56]

lgtm

[lcwu1, 2014-10-08 00:06:27]

The CQ bit was checked by lcwu@chromium.org

[commit-bot: I haz the power, 2014-10-08 00:08:17]

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

[commit-bot: I haz the power, 2014-10-08 00:31:07]

Committed patchset #2 (id:20001) as 9403d8e259b808ace88a6123e7bc6c577439dc14

[commit-bot: I haz the power, 2014-10-08 00:31:40]

Patchset 2 (id:??) landed as
https://crrev.com/a278594a43c76544cb762ad3ded470664fb9c465
Cr-Commit-Position: refs/heads/master@{#298618}