Rename CancellationFlag to AtomicFlag.

Rename CancellationFlag to AtomicFlag.

CancellationFlag is really just a flag that can be set once from
one thread and read from a number of other threads. It is already
used to express other things than cancellation (e.g. in
after_startup_task_utils.cc) and could be used at a few places
in base/task_scheduler instead of WaitableEvent.

BUG=553459
Committed: https://crrev.com/ef191122f44b4d41a71c573bf785f6e1f68f4dfb
Cr-Commit-Position: refs/heads/master@{#407475}

[fdoray, 2016-07-20 16:24:10]

fdoray@chromium.org changed reviewers:
+ danakj@chromium.org

[fdoray, 2016-07-20 16:24:11]

PTAL

https://codereview.chromium.org/2163753004/diff/20001/base/synchronization/at...
File base/synchronization/atomic_flag_unittest.cc (right):

https://codereview.chromium.org/2163753004/diff/20001/base/synchronization/at...
base/synchronization/atomic_flag_unittest.cc:21: ASSERT_DEBUG_DEATH(flag->Set(),
"");
TODO: Use EXPECT_DCHECK_DEATH once https://codereview.chromium.org/2162053006/
lands.

[danakj, 2016-07-20 19:21:01]

Thanks, this looks like a great idea. I didn't know this exists :)

https://codereview.chromium.org/2163753004/diff/20001/base/synchronization/at...
File base/synchronization/atomic_flag.h (right):

https://codereview.chromium.org/2163753004/diff/20001/base/synchronization/at...
base/synchronization/atomic_flag.h:30: // different thread testing IsSet() may
erroneously read 'true' after this
Really? But it's using Release_Store. Is this still true?

https://codereview.chromium.org/2163753004/diff/20001/base/synchronization/at...
File base/synchronization/atomic_flag_unittest.cc (right):

https://codereview.chromium.org/2163753004/diff/20001/base/synchronization/at...
base/synchronization/atomic_flag_unittest.cc:18: void CancelHelper(AtomicFlag*
flag) {
rename to SetFlag or something?

https://codereview.chromium.org/2163753004/diff/20001/base/synchronization/at...
base/synchronization/atomic_flag_unittest.cc:67:
t.task_runner()->PostTask(FROM_HERE, Bind(&ExpectIsSet, &flag));
post task is a memory barrier so this test is kinda silly, it's not testing
anything, it would pass even without any atomics right?

you could make this test better by posting the task before calling Set, and have
it spin until it sees the flag update.

And maybe use atomics in the test to tell the other thread when it should expect
to see it updated.

https://codereview.chromium.org/2163753004/diff/20001/base/synchronization/ca...
File base/synchronization/cancellation_flag.h (right):

https://codereview.chromium.org/2163753004/diff/20001/base/synchronization/ca...
base/synchronization/cancellation_flag.h:14: class CancellationFlag : public
AtomicFlag {};
Are you going to rename the rest in another CL and remove this? If so, TODO
pointing at a bug plz.

[fdoray, 2016-07-21 13:08:33]

PTAnL

https://codereview.chromium.org/2163753004/diff/20001/base/synchronization/at...
File base/synchronization/atomic_flag.h (right):

https://codereview.chromium.org/2163753004/diff/20001/base/synchronization/at...
base/synchronization/atomic_flag.h:30: // different thread testing IsSet() may
erroneously read 'true' after this
On 2016/07/20 19:21:01, danakj wrote:
> Really? But it's using Release_Store. Is this still true?

Talked with the author of https://codereview.chromium.org/949293002/ - it seems
that it's only unsafe because of this:

if (flag.IsSet()) {
  // Can't assume that the flag is still set here
  // because of UnsafeResetForTesting().
}

https://codereview.chromium.org/2163753004/diff/20001/base/synchronization/at...
File base/synchronization/atomic_flag_unittest.cc (right):

https://codereview.chromium.org/2163753004/diff/20001/base/synchronization/at...
base/synchronization/atomic_flag_unittest.cc:18: void CancelHelper(AtomicFlag*
flag) {
On 2016/07/20 19:21:01, danakj wrote:
> rename to SetFlag or something?

Done.

https://codereview.chromium.org/2163753004/diff/20001/base/synchronization/at...
base/synchronization/atomic_flag_unittest.cc:67:
t.task_runner()->PostTask(FROM_HERE, Bind(&ExpectIsSet, &flag));
On 2016/07/20 19:21:01, danakj wrote:
> post task is a memory barrier so this test is kinda silly, it's not testing
> anything, it would pass even without any atomics right?
> 
> you could make this test better by posting the task before calling Set, and
have
> it spin until it sees the flag update.

sgtm

> 
> And maybe use atomics in the test to tell the other thread when it should
expect
> to see it updated.

If I use atomics in the test, they'll be accompanied with memory barriers and
we'll have the same problem as we have now.

https://codereview.chromium.org/2163753004/diff/20001/base/synchronization/ca...
File base/synchronization/cancellation_flag.h (right):

https://codereview.chromium.org/2163753004/diff/20001/base/synchronization/ca...
base/synchronization/cancellation_flag.h:14: class CancellationFlag : public
AtomicFlag {};
On 2016/07/20 19:21:01, danakj wrote:
> Are you going to rename the rest in another CL and remove this? If so, TODO
> pointing at a bug plz.

Done.

[gab, 2016-07-21 13:27:13]

Awesome, thanks for doing this, I love how I merely said "I wish this wasn't
named CancellationFlag on another CL" and then this happens :-).

https://codereview.chromium.org/2163753004/diff/20001/base/synchronization/at...
File base/synchronization/atomic_flag_unittest.cc (right):

https://codereview.chromium.org/2163753004/diff/20001/base/synchronization/at...
base/synchronization/atomic_flag_unittest.cc:67:
t.task_runner()->PostTask(FROM_HERE, Bind(&ExpectIsSet, &flag));
On 2016/07/21 13:08:33, fdoray wrote:
> On 2016/07/20 19:21:01, danakj wrote:
> > post task is a memory barrier so this test is kinda silly, it's not testing
> > anything, it would pass even without any atomics right?
> > 
> > you could make this test better by posting the task before calling Set, and
> have
> > it spin until it sees the flag update.
> 
> sgtm
> 
> > 
> > And maybe use atomics in the test to tell the other thread when it should
> expect
> > to see it updated.
> 
> If I use atomics in the test, they'll be accompanied with memory barriers and
> we'll have the same problem as we have now.

True, then maybe sleep here for say 10ms before calling Set() to increase
likelihood of Set() being called after the busy wait started? (with the yield
suggested in the loop it should work fine)

https://codereview.chromium.org/2163753004/diff/40001/base/synchronization/at...
File base/synchronization/atomic_flag_unittest.cc (right):

https://codereview.chromium.org/2163753004/diff/40001/base/synchronization/at...
base/synchronization/atomic_flag_unittest.cc:5: // Tests of AtomicFlag class.
rm this comment (I've never seen any other class do this)

https://codereview.chromium.org/2163753004/diff/40001/base/synchronization/at...
base/synchronization/atomic_flag_unittest.cc:27: while (!flag->IsSet()) {
PlatformThread::YieldCurrentThread() in each iteration.

https://codereview.chromium.org/2163753004/diff/40001/base/synchronization/at...
base/synchronization/atomic_flag_unittest.cc:53:
"AtomicFlagTest.ReadFromDifferentThread.ReadFromDifferentThread");
nit: double ".ReadFromDifferentThread.ReadFromDifferentThread"

[fdoray, 2016-07-21 14:05:30]

danakj@/gab@: PTAnL

https://codereview.chromium.org/2163753004/diff/20001/base/synchronization/at...
File base/synchronization/atomic_flag_unittest.cc (right):

https://codereview.chromium.org/2163753004/diff/20001/base/synchronization/at...
base/synchronization/atomic_flag_unittest.cc:67:
t.task_runner()->PostTask(FROM_HERE, Bind(&ExpectIsSet, &flag));
On 2016/07/21 13:27:12, gab wrote:
> On 2016/07/21 13:08:33, fdoray wrote:
> > On 2016/07/20 19:21:01, danakj wrote:
> > > post task is a memory barrier so this test is kinda silly, it's not
testing
> > > anything, it would pass even without any atomics right?
> > > 
> > > you could make this test better by posting the task before calling Set,
and
> > have
> > > it spin until it sees the flag update.
> > 
> > sgtm
> > 
> > > 
> > > And maybe use atomics in the test to tell the other thread when it should
> > expect
> > > to see it updated.
> > 
> > If I use atomics in the test, they'll be accompanied with memory barriers
and
> > we'll have the same problem as we have now.
> 
> True, then maybe sleep here for say 10ms before calling Set() to increase
> likelihood of Set() being called after the busy wait started? (with the yield
> suggested in the loop it should work fine)

sgtm.

I'm still a little bit worried that Sleep, YieldCurrentThread and the join in
base::Thread's destructor may act as memory barriers, but I don't think we can
do better.

https://codereview.chromium.org/2163753004/diff/40001/base/synchronization/at...
File base/synchronization/atomic_flag_unittest.cc (right):

https://codereview.chromium.org/2163753004/diff/40001/base/synchronization/at...
base/synchronization/atomic_flag_unittest.cc:5: // Tests of AtomicFlag class.
On 2016/07/21 13:27:13, gab wrote:
> rm this comment (I've never seen any other class do this)

Done.

https://codereview.chromium.org/2163753004/diff/40001/base/synchronization/at...
base/synchronization/atomic_flag_unittest.cc:27: while (!flag->IsSet()) {
On 2016/07/21 13:27:13, gab wrote:
> PlatformThread::YieldCurrentThread() in each iteration.

Done.

https://codereview.chromium.org/2163753004/diff/40001/base/synchronization/at...
base/synchronization/atomic_flag_unittest.cc:53:
"AtomicFlagTest.ReadFromDifferentThread.ReadFromDifferentThread");
On 2016/07/21 13:27:13, gab wrote:
> nit: double ".ReadFromDifferentThread.ReadFromDifferentThread"

Done.

[gab, 2016-07-21 14:16:04]

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

[gab, 2016-07-21 14:16:04]

https://codereview.chromium.org/2163753004/diff/20001/base/synchronization/at...
File base/synchronization/atomic_flag_unittest.cc (right):

https://codereview.chromium.org/2163753004/diff/20001/base/synchronization/at...
base/synchronization/atomic_flag_unittest.cc:67:
t.task_runner()->PostTask(FROM_HERE, Bind(&ExpectIsSet, &flag));
On 2016/07/21 14:05:30, fdoray wrote:
> On 2016/07/21 13:27:12, gab wrote:
> > On 2016/07/21 13:08:33, fdoray wrote:
> > > On 2016/07/20 19:21:01, danakj wrote:
> > > > post task is a memory barrier so this test is kinda silly, it's not
> testing
> > > > anything, it would pass even without any atomics right?
> > > > 
> > > > you could make this test better by posting the task before calling Set,
> and
> > > have
> > > > it spin until it sees the flag update.
> > > 
> > > sgtm
> > > 
> > > > 
> > > > And maybe use atomics in the test to tell the other thread when it
should
> > > expect
> > > > to see it updated.
> > > 
> > > If I use atomics in the test, they'll be accompanied with memory barriers
> and
> > > we'll have the same problem as we have now.
> > 
> > True, then maybe sleep here for say 10ms before calling Set() to increase
> > likelihood of Set() being called after the busy wait started? (with the
yield
> > suggested in the loop it should work fine)
> 
> sgtm.
> 
> I'm still a little bit worried that Sleep, YieldCurrentThread and the join in
> base::Thread's destructor may act as memory barriers, but I don't think we can
> do better.

I doubt Sleep and Yield result in memory barriers. Join definitely does but I
think the barrier is an rfence (Release) on the thread going away and an lfence
(Acquire) on the Join side (anything else doesn't make much sense IMO) -- i.e.,
I don't think calling Join from thread A on thread B changes anything for thread
B (which is the one we're testing atomics on so it should be okay).

https://codereview.chromium.org/2163753004/diff/60001/base/synchronization/at...
File base/synchronization/atomic_flag_unittest.cc (right):

https://codereview.chromium.org/2163753004/diff/60001/base/synchronization/at...
base/synchronization/atomic_flag_unittest.cc:28:
PlatformThread::Sleep(TimeDelta::FromMilliseconds(10));
I meant to add the Sleep() on the main thread before calling Set(). i.e. to
increase likelihood of the thread already being waiting as Start() returns
before ThreadMain hits and if the first call to IsSet() is after Set() there is
no need for an Acquire_Load as the first read will always have to fetch from
memory so we want the busy wait to start before calling Set().

Also: add to the comment that this is to test memory barriers involved.

[fdoray, 2016-07-21 15:50:02]

PTAnL

https://codereview.chromium.org/2163753004/diff/20001/base/synchronization/at...
File base/synchronization/atomic_flag_unittest.cc (right):

https://codereview.chromium.org/2163753004/diff/20001/base/synchronization/at...
base/synchronization/atomic_flag_unittest.cc:67:
t.task_runner()->PostTask(FROM_HERE, Bind(&ExpectIsSet, &flag));
On 2016/07/21 14:16:04, gab wrote:
> On 2016/07/21 14:05:30, fdoray wrote:
> > On 2016/07/21 13:27:12, gab wrote:
> > > On 2016/07/21 13:08:33, fdoray wrote:
> > > > On 2016/07/20 19:21:01, danakj wrote:
> > > > > post task is a memory barrier so this test is kinda silly, it's not
> > testing
> > > > > anything, it would pass even without any atomics right?
> > > > > 
> > > > > you could make this test better by posting the task before calling
Set,
> > and
> > > > have
> > > > > it spin until it sees the flag update.
> > > > 
> > > > sgtm
> > > > 
> > > > > 
> > > > > And maybe use atomics in the test to tell the other thread when it
> should
> > > > expect
> > > > > to see it updated.
> > > > 
> > > > If I use atomics in the test, they'll be accompanied with memory
barriers
> > and
> > > > we'll have the same problem as we have now.
> > > 
> > > True, then maybe sleep here for say 10ms before calling Set() to increase
> > > likelihood of Set() being called after the busy wait started? (with the
> yield
> > > suggested in the loop it should work fine)
> > 
> > sgtm.
> > 
> > I'm still a little bit worried that Sleep, YieldCurrentThread and the join
in
> > base::Thread's destructor may act as memory barriers, but I don't think we
can
> > do better.
> 
> I doubt Sleep and Yield result in memory barriers. Join definitely does but I
> think the barrier is an rfence (Release) on the thread going away and an
lfence
> (Acquire) on the Join side (anything else doesn't make much sense IMO) --
i.e.,
> I don't think calling Join from thread A on thread B changes anything for
thread
> B (which is the one we're testing atomics on so it should be okay).

Shouldn't the memory be synchronized when the thread is descheduled? That way,
the thread will be able to see what it wrote in memory when it is re-scheduled
on another core.

https://codereview.chromium.org/2163753004/diff/60001/base/synchronization/at...
File base/synchronization/atomic_flag_unittest.cc (right):

https://codereview.chromium.org/2163753004/diff/60001/base/synchronization/at...
base/synchronization/atomic_flag_unittest.cc:28:
PlatformThread::Sleep(TimeDelta::FromMilliseconds(10));
On 2016/07/21 14:16:04, gab wrote:
> I meant to add the Sleep() on the main thread before calling Set(). i.e. to
> increase likelihood of the thread already being waiting as Start() returns
> before ThreadMain hits and if the first call to IsSet() is after Set() there
is
> no need for an Acquire_Load as the first read will always have to fetch from
> memory so we want the busy wait to start before calling Set().
> 
> Also: add to the comment that this is to test memory barriers involved.

Done.

[gab, 2016-07-21 15:56:43]

https://codereview.chromium.org/2163753004/diff/20001/base/synchronization/at...
File base/synchronization/atomic_flag_unittest.cc (right):

https://codereview.chromium.org/2163753004/diff/20001/base/synchronization/at...
base/synchronization/atomic_flag_unittest.cc:67:
t.task_runner()->PostTask(FROM_HERE, Bind(&ExpectIsSet, &flag));
On 2016/07/21 15:50:02, fdoray wrote:
> On 2016/07/21 14:16:04, gab wrote:
> > On 2016/07/21 14:05:30, fdoray wrote:
> > > On 2016/07/21 13:27:12, gab wrote:
> > > > On 2016/07/21 13:08:33, fdoray wrote:
> > > > > On 2016/07/20 19:21:01, danakj wrote:
> > > > > > post task is a memory barrier so this test is kinda silly, it's not
> > > testing
> > > > > > anything, it would pass even without any atomics right?
> > > > > > 
> > > > > > you could make this test better by posting the task before calling
> Set,
> > > and
> > > > > have
> > > > > > it spin until it sees the flag update.
> > > > > 
> > > > > sgtm
> > > > > 
> > > > > > 
> > > > > > And maybe use atomics in the test to tell the other thread when it
> > should
> > > > > expect
> > > > > > to see it updated.
> > > > > 
> > > > > If I use atomics in the test, they'll be accompanied with memory
> barriers
> > > and
> > > > > we'll have the same problem as we have now.
> > > > 
> > > > True, then maybe sleep here for say 10ms before calling Set() to
increase
> > > > likelihood of Set() being called after the busy wait started? (with the
> > yield
> > > > suggested in the loop it should work fine)
> > > 
> > > sgtm.
> > > 
> > > I'm still a little bit worried that Sleep, YieldCurrentThread and the join
> in
> > > base::Thread's destructor may act as memory barriers, but I don't think we
> can
> > > do better.
> > 
> > I doubt Sleep and Yield result in memory barriers. Join definitely does but
I
> > think the barrier is an rfence (Release) on the thread going away and an
> lfence
> > (Acquire) on the Join side (anything else doesn't make much sense IMO) --
> i.e.,
> > I don't think calling Join from thread A on thread B changes anything for
> thread
> > B (which is the one we're testing atomics on so it should be okay).
> 
> Shouldn't the memory be synchronized when the thread is descheduled? That way,
> the thread will be able to see what it wrote in memory when it is re-scheduled
> on another core.

I don't think memory *has* to be synchronized when the thread is descheduled.
The kernel will have to if it wants to reschedule on another core, but there may
very well be heuristics that only synchronize when such a move occurs instead of
always.

The only guarantee of memory order is that a single-threaded program shouldn't
experience a difference. That doesn't mean it has to be synchronized every time
it's descheduled IMO.

[danakj, 2016-07-21 19:21:43]

LGTM

https://codereview.chromium.org/2163753004/diff/80001/base/synchronization/at...
File base/synchronization/atomic_flag.h (right):

https://codereview.chromium.org/2163753004/diff/80001/base/synchronization/at...
base/synchronization/atomic_flag.h:29: // Resets the flag. Be careful when using
this: callers might not expect
Thanks this is a great comment.

https://codereview.chromium.org/2163753004/diff/80001/base/synchronization/at...
File base/synchronization/atomic_flag_unittest.cc (right):

https://codereview.chromium.org/2163753004/diff/80001/base/synchronization/at...
base/synchronization/atomic_flag_unittest.cc:61: flag.Set();
can you leave a comment below this that quickly explains the Thread destructor
will block until the posted task completes, so the test will time out if it
fails to see the flag be set.

[fdoray, 2016-07-21 19:39:27]

fdoray@chromium.org changed reviewers:
+ jochen@chromium.org

[fdoray, 2016-07-21 19:39:28]

jochen@: Can you review chrome/browser/after_startup_task_utils.cc? Thanks!

[jochen (gone - plz use gerrit), 2016-07-22 12:24:16]

lgtm

[fdoray, 2016-07-22 12:46:21]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2016-07-22 12:46:21]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/2163753004/#ps100001
(title: "CR danakj #11 (add comment about Thread's destructor in unit test)")

[commit-bot: I haz the power, 2016-07-22 12:46:31]

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, 2016-07-22 13:13:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-22 13:13:15]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[fdoray, 2016-07-22 20:26:38]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2016-07-22 20:26:39]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/2163753004/#ps120001
(title: "restore task_tracker_unitttest.cc to make tests pass")

[commit-bot: I haz the power, 2016-07-22 20:27:25]

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, 2016-07-22 20:59:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-22 20:59:30]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[fdoray, 2016-07-25 13:38:00]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2016-07-25 13:38:00]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/2163753004/#ps140001
(title: "fix dcheck death test")

[commit-bot: I haz the power, 2016-07-25 13:38:12]

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, 2016-07-25 14:43:31]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-07-25 14:45:26]

Description was changed from

==========
Rename CancellationFlag to AtomicFlag.

CancellationFlag is really just a flag that can be set once from
one thread and read from a number of other threads. It is already
used to express other things than cancellation (e.g. in
after_startup_task_utils.cc) and could be used at a few places
in base/task_scheduler instead of WaitableEvent.

BUG=553459
==========

to

==========
Rename CancellationFlag to AtomicFlag.

CancellationFlag is really just a flag that can be set once from
one thread and read from a number of other threads. It is already
used to express other things than cancellation (e.g. in
after_startup_task_utils.cc) and could be used at a few places
in base/task_scheduler instead of WaitableEvent.

BUG=553459

Committed: https://crrev.com/ef191122f44b4d41a71c573bf785f6e1f68f4dfb
Cr-Commit-Position: refs/heads/master@{#407475}
==========

[commit-bot: I haz the power, 2016-07-25 14:45:27]

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