[Android Crash Reporting] Mark mCancelUpload as volatile.

[Android Crash Reporting] Mark mCancelUpload as volatile.

It is otherwise theoretically possible for the compiler to hoist the variable
read out of the loop, which would cause updates sent by the writing thread to
never be seen.

BUG=694884
TEST=none
R=tobiasjs@chromium.org

Review-Url: https://codereview.chromium.org/2721023004
Cr-Commit-Position: refs/heads/master@{#454451}
Committed: https://chromium.googlesource.com/chromium/src/+/37716386769b230a22b267a9789d68946b094552

[Tobias Sargeant, 2017-03-02 12:30:21]

On 2017/03/01 00:15:50, Ilya Sherman wrote:

LGTM. I think it's mostly a theoretical concern, but this is one of the few
cases where the use of volatile in Java is correct and well supported.

[Torne, 2017-03-02 14:11:19]

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

[Torne, 2017-03-02 14:11:19]

The description here is a bit weird and misleading, even though the use of
volatile is correct. It has to be volatile to prevent the compiler (which here
means either javac, or the actual VM at runtime) from moving the read outside
the loop. It's not really about whether the write by one thread is visible to
the other or not - if the reading thread actually does the read, it *will* see
the change.

(and yes, in practise it's not going to do this optimisation for a variety of
reasons and so the code will work fine without the volatile anyway, but hey)

[Ilya Sherman, 2017-03-02 17:35:27]

On 2017/03/02 14:11:19, Torne wrote:
> The description here is a bit weird and misleading, even though the use of
> volatile is correct. It has to be volatile to prevent the compiler (which here
> means either javac, or the actual VM at runtime) from moving the read outside
> the loop. It's not really about whether the write by one thread is visible to
> the other or not - if the reading thread actually does the read, it *will* see
> the change.
> 
> (and yes, in practise it's not going to do this optimisation for a variety of
> reasons and so the code will work fine without the volatile anyway, but hey)

So, I freely admit that I am not an expert in the Java memory model.  However,
in C++, a lack of "volatile" *could* have this impact, and I'm a little
surprised if the same isn't true in Java.  Here's how:

The two threads could be scheduled on two different CPUs, which are going to
have caches, and reads and writes happen within those caches until either (a)
they're marked dirty, or (b) the cache line is flushed to make room for
something else, or is updated to update something else.  In C++, I'd want to add
memory barriers to force the cached values to be correctly synchronized across
CPUs.  Does Java not have a similar consideration?  If not... how does that
work?  Does the JVM basically put memory barriers around all variables that
happen to be used on multiple CPUs?

[Torne, 2017-03-02 17:45:09]

On 2017/03/02 17:35:27, Ilya Sherman wrote:
> On 2017/03/02 14:11:19, Torne wrote:
> > The description here is a bit weird and misleading, even though the use of
> > volatile is correct. It has to be volatile to prevent the compiler (which
here
> > means either javac, or the actual VM at runtime) from moving the read
outside
> > the loop. It's not really about whether the write by one thread is visible
to
> > the other or not - if the reading thread actually does the read, it *will*
see
> > the change.
> > 
> > (and yes, in practise it's not going to do this optimisation for a variety
of
> > reasons and so the code will work fine without the volatile anyway, but hey)
> 
> So, I freely admit that I am not an expert in the Java memory model.  However,
> in C++, a lack of "volatile" *could* have this impact, and I'm a little
> surprised if the same isn't true in Java.  Here's how:
> 
> The two threads could be scheduled on two different CPUs, which are going to
> have caches, and reads and writes happen within those caches until either (a)
> they're marked dirty, or (b) the cache line is flushed to make room for
> something else, or is updated to update something else.  In C++, I'd want to
add
> memory barriers to force the cached values to be correctly synchronized across
> CPUs.  Does Java not have a similar consideration?  If not... how does that
> work?  Does the JVM basically put memory barriers around all variables that
> happen to be used on multiple CPUs?

This is also not possible in C++, and that's not what memory barriers do :)

Caches on general-purpose CPUs are automatically coherent, implemented in
hardware. It's never permitted for a cacheline to be in two caches at once
unless both copies are unmodified.

What *is* possible is that the observable order of multiple different writes may
not be consistent when viewed from different CPUs' perspectives, because of
out-of-order execution. Memory barriers prevent this by forcing all pending
memory accesses to finish before execution is allowed to continue.

Since this is setting a single flag, and there's no other relevant communication
between the two threads, then ordering is not actually relevant, and there's no
need for a barrier.

[Ilya Sherman, 2017-03-02 17:51:06]

On 2017/03/02 17:45:09, Torne wrote:
> On 2017/03/02 17:35:27, Ilya Sherman wrote:
> > On 2017/03/02 14:11:19, Torne wrote:
> > > The description here is a bit weird and misleading, even though the use of
> > > volatile is correct. It has to be volatile to prevent the compiler (which
> here
> > > means either javac, or the actual VM at runtime) from moving the read
> outside
> > > the loop. It's not really about whether the write by one thread is visible
> to
> > > the other or not - if the reading thread actually does the read, it *will*
> see
> > > the change.
> > > 
> > > (and yes, in practise it's not going to do this optimisation for a variety
> of
> > > reasons and so the code will work fine without the volatile anyway, but
hey)
> > 
> > So, I freely admit that I am not an expert in the Java memory model. 
However,
> > in C++, a lack of "volatile" *could* have this impact, and I'm a little
> > surprised if the same isn't true in Java.  Here's how:
> > 
> > The two threads could be scheduled on two different CPUs, which are going to
> > have caches, and reads and writes happen within those caches until either
(a)
> > they're marked dirty, or (b) the cache line is flushed to make room for
> > something else, or is updated to update something else.  In C++, I'd want to
> add
> > memory barriers to force the cached values to be correctly synchronized
across
> > CPUs.  Does Java not have a similar consideration?  If not... how does that
> > work?  Does the JVM basically put memory barriers around all variables that
> > happen to be used on multiple CPUs?
> 
> This is also not possible in C++, and that's not what memory barriers do :)
> 
> Caches on general-purpose CPUs are automatically coherent, implemented in
> hardware. It's never permitted for a cacheline to be in two caches at once
> unless both copies are unmodified.
> 
> What *is* possible is that the observable order of multiple different writes
may
> not be consistent when viewed from different CPUs' perspectives, because of
> out-of-order execution. Memory barriers prevent this by forcing all pending
> memory accesses to finish before execution is allowed to continue.
> 
> Since this is setting a single flag, and there's no other relevant
communication
> between the two threads, then ordering is not actually relevant, and there's
no
> need for a barrier.

Ah, I see!  Thanks for the clarification.  I hadn't realized that CPUs guarantee
coherent cache lines -- good to know!

[Ilya Sherman, 2017-03-02 17:53:04]

Description was changed from

==========
[Android Crash Reporting] Mark mCancelUpload as volatile.

This variable is written on one thread, and read on another. 'volatile' is the
minimum requirement to ensure that updates are *ever* seen by the reading
thread.

BUG=694884
TEST=none
R=tobiasjs@chromium.org
==========

to

==========
[Android Crash Reporting] Mark mCancelUpload as volatile.

It is otherwise theoretically possible for the compiler to hoist the variable
read out of
the loop, which would cause updates sent by the writing thread to never be seen.

BUG=694884
TEST=none
R=tobiasjs@chromium.org
==========

[Ilya Sherman, 2017-03-02 17:53:43]

Description was changed from

==========
[Android Crash Reporting] Mark mCancelUpload as volatile.

It is otherwise theoretically possible for the compiler to hoist the variable
read out of
the loop, which would cause updates sent by the writing thread to never be seen.

BUG=694884
TEST=none
R=tobiasjs@chromium.org
==========

to

==========
[Android Crash Reporting] Mark mCancelUpload as volatile.

It is otherwise theoretically possible for the compiler to hoist the variable
read out of the loop, which would cause updates sent by the writing thread to
never be seen.

BUG=694884
TEST=none
R=tobiasjs@chromium.org
==========

[Ilya Sherman, 2017-03-02 23:34:35]

The CQ bit was checked by isherman@chromium.org

[Ilya Sherman, 2017-03-02 23:34:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
tobiasjs@chromium.org
Link to the patchset: https://codereview.chromium.org/2721023004/#ps20001
(title: "Rebase")

[commit-bot: I haz the power, 2017-03-02 23:35:10]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-03 00:24:52]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1488497675096720,
"parent_rev": "573bd7b3152aba99ff74ad02ae1aa846ad32a448", "commit_rev":
"37716386769b230a22b267a9789d68946b094552"}

[commit-bot: I haz the power, 2017-03-03 00:25:41]

Description was changed from

==========
[Android Crash Reporting] Mark mCancelUpload as volatile.

It is otherwise theoretically possible for the compiler to hoist the variable
read out of the loop, which would cause updates sent by the writing thread to
never be seen.

BUG=694884
TEST=none
R=tobiasjs@chromium.org
==========

to

==========
[Android Crash Reporting] Mark mCancelUpload as volatile.

It is otherwise theoretically possible for the compiler to hoist the variable
read out of the loop, which would cause updates sent by the writing thread to
never be seen.

BUG=694884
TEST=none
R=tobiasjs@chromium.org

Review-Url: https://codereview.chromium.org/2721023004
Cr-Commit-Position: refs/heads/master@{#454451}
Committed:
https://chromium.googlesource.com/chromium/src/+/37716386769b230a22b267a9789d...
==========

[commit-bot: I haz the power, 2017-03-03 00:25:43]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/37716386769b230a22b267a9789d...