Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2187333002:
Loosen/improve AtomicFlag semantics. (Closed)
DescriptionLoosen/improve AtomicFlag semantics.
In particular don't attach to construction thread, the requirement
was I think trying to articulate that it doesn't make sense to call
Set() from different threads as it's then not clear which thread
IsSet() is memory consistent with.
But:
1) What's required is *sequence* not *thread* checking.
2) Calls to Set() need to be sequenced w.r.t. to each other, but not
with respect to construction (AtomicFlag is not modified before Set()).
Update restrictions to reflect that and update comments to reflect the
guarantees provided by them + its usage of atomics/barriers.
Also expose UnsafeReset() outside of "testing", it will be used in
https://codereview.chromium.org/2135413003/.
BUG=629716
Committed: https://crrev.com/d0cc00da87f2e1ccae783ad876848518b1c389f0
Cr-Commit-Position: refs/heads/master@{#409020}
Patch Set 1 #Patch Set 2 : self-review #Patch Set 3 : side-effects #
Total comments: 9
Patch Set 4 : =>UnsafeResetForTesting and BusyWaitUntilFlagIsSet() comment #Patch Set 5 : fix compile post merge #
Dependent Patchsets: Messages
Total messages: 44 (29 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from
==========
Loosen/improve AtomicFlag semantics.
BUG=629716
==========
to
==========
Loosen/improve AtomicFlag semantics.
In particular don't attach to construction thread, the requirement
was I think trying to articulate that it doesn't make sense to call
Set() from different threads as it's then not clear which thread
IsSet() is memory consistent with.
But:
1) What's required is *sequence* not *thread* checking.
2) Calls to Set() need to be sequenced w.r.t. to each other, but not
with respect to construction (AtomicFlag is not modified before Set()).
Update restrictions to reflect that and update comments to reflect the
guarantees provided by them + its usage of atomics/barriers.
BUG=629716
==========
Description was changed from
==========
Loosen/improve AtomicFlag semantics.
In particular don't attach to construction thread, the requirement
was I think trying to articulate that it doesn't make sense to call
Set() from different threads as it's then not clear which thread
IsSet() is memory consistent with.
But:
1) What's required is *sequence* not *thread* checking.
2) Calls to Set() need to be sequenced w.r.t. to each other, but not
with respect to construction (AtomicFlag is not modified before Set()).
Update restrictions to reflect that and update comments to reflect the
guarantees provided by them + its usage of atomics/barriers.
BUG=629716
==========
to
==========
Loosen/improve AtomicFlag semantics.
In particular don't attach to construction thread, the requirement
was I think trying to articulate that it doesn't make sense to call
Set() from different threads as it's then not clear which thread
IsSet() is memory consistent with.
But:
1) What's required is *sequence* not *thread* checking.
2) Calls to Set() need to be sequenced w.r.t. to each other, but not
with respect to construction (AtomicFlag is not modified before Set()).
Update restrictions to reflect that and update comments to reflect the
guarantees provided by them + its usage of atomics/barriers.
Also expose UnsafeReset() outside of "testing", it will be used in
https://codereview.chromium.org/2135413003/.
BUG=629716
==========
gab@chromium.org changed reviewers: + danakj@chromium.org
Dana PTAL, this is now a precursor to https://codereview.chromium.org/2135413003/ Thanks!
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I agree about the constructor, I was wondering about it in the first place :) But I'm not super excited about the UnsafeReset change, and I wonder when it would actually be needed. I left a comment on the Thread CL about that use case. https://codereview.chromium.org/2187333002/diff/90001/base/synchronization/at... File base/synchronization/atomic_flag_unittest.cc (right): https://codereview.chromium.org/2187333002/diff/90001/base/synchronization/at... base/synchronization/atomic_flag_unittest.cc:37: void BusyWaitUntilFlagIsSet(AtomicFlag* flag, const base::Closure& callback) { nit: this would be easier to read if you just passed the bool* and flag* here instead of a callback.
On 2016/07/28 19:57:22, danakj wrote: > I agree about the constructor, I was wondering about it in the first place :) > But I'm not super excited about the UnsafeReset change, and I wonder when it > would actually be needed. I left a comment on the Thread CL about that use case. I guess the other option for that use case is for people to put the AtomicFlag inside a std::unique_ptr and reset it with a new instance (if it's safe to call UnsafeReset() -- i.e. all callers synchronize to that point before using it again -- then it's safe to reset the unique_ptr as well). That doesn't work well for the base::LazyInstance<base::AtomicFlag>::Leaky instances that would desire to reset though (but the only one of those only needs to reset for testing right now -- after_startup_task_utils.cc). > > https://codereview.chromium.org/2187333002/diff/90001/base/synchronization/at... > File base/synchronization/atomic_flag_unittest.cc (right): > > https://codereview.chromium.org/2187333002/diff/90001/base/synchronization/at... > base/synchronization/atomic_flag_unittest.cc:37: void > BusyWaitUntilFlagIsSet(AtomicFlag* flag, const base::Closure& callback) { > nit: this would be easier to read if you just passed the bool* and flag* here > instead of a callback.
fdoray@chromium.org changed reviewers: + fdoray@chromium.org
https://codereview.chromium.org/2187333002/diff/90001/base/synchronization/at... File base/synchronization/atomic_flag.h (right): https://codereview.chromium.org/2187333002/diff/90001/base/synchronization/at... base/synchronization/atomic_flag.h:40: SequenceChecker set_sequence_checker_; Given that there are no guarantees about the synchronization of memory operations made after the first call to Set(), I'm not convinced that SequenceChecker is useful (Why should we disallow calls to Set() from different sequences if IsSet() callers cannot really assume anything about memory synchronization when multiple calls to Set() are possible?) I'm not strongly against keeping the SequenceChecker if you think it's useful.
On 2016/07/28 23:39:56, gab wrote: > On 2016/07/28 19:57:22, danakj wrote: > > I agree about the constructor, I was wondering about it in the first place :) > > But I'm not super excited about the UnsafeReset change, and I wonder when it > > would actually be needed. I left a comment on the Thread CL about that use > case. > > I guess the other option for that use case is for people to put the AtomicFlag > inside a std::unique_ptr and reset it with a new instance (if it's safe to call > UnsafeReset() -- i.e. all callers synchronize to that point before using it > again -- then it's safe to reset the unique_ptr as well). > > That doesn't work well for the base::LazyInstance<base::AtomicFlag>::Leaky > instances that would desire to reset though (but the only one of those only > needs to reset for testing right now -- after_startup_task_utils.cc). I don't need this for https://codereview.chromium.org/2135413003/ anymore but I'm happy to complete this CL if you think it has value. I can leave UnsafeResetForTesting() as-is for now if you prefer. https://codereview.chromium.org/2187333002/diff/90001/base/synchronization/at... File base/synchronization/atomic_flag.h (right): https://codereview.chromium.org/2187333002/diff/90001/base/synchronization/at... base/synchronization/atomic_flag.h:40: SequenceChecker set_sequence_checker_; On 2016/07/29 20:41:37, fdoray wrote: > Given that there are no guarantees about the synchronization of memory > operations made after the first call to Set(), I'm not convinced that > SequenceChecker is useful (Why should we disallow calls to Set() from different > sequences if IsSet() callers cannot really assume anything about memory > synchronization when multiple calls to Set() are possible?) > > I'm not strongly against keeping the SequenceChecker if you think it's useful. I think it's useful because the IsSet() callers can at least know which sequence they're synchronizing with. If there are multiple callers to Set() than there might as well not be barriers as the IsSet() callers can't assume anything about the state of memory anyways (they're synced with "a" thread/sequence but don't know which one...).
lgtm https://codereview.chromium.org/2187333002/diff/90001/base/synchronization/at... File base/synchronization/atomic_flag.h (right): https://codereview.chromium.org/2187333002/diff/90001/base/synchronization/at... base/synchronization/atomic_flag.h:36: void UnsafeReset(); Keep ForTesting() if you no longer need to call this outside of tests. https://codereview.chromium.org/2187333002/diff/90001/base/synchronization/at... base/synchronization/atomic_flag.h:40: SequenceChecker set_sequence_checker_; On 2016/08/01 15:08:29, gab wrote: > On 2016/07/29 20:41:37, fdoray wrote: > > Given that there are no guarantees about the synchronization of memory > > operations made after the first call to Set(), I'm not convinced that > > SequenceChecker is useful (Why should we disallow calls to Set() from > different > > sequences if IsSet() callers cannot really assume anything about memory > > synchronization when multiple calls to Set() are possible?) > > > > I'm not strongly against keeping the SequenceChecker if you think it's useful. > > I think it's useful because the IsSet() callers can at least know which sequence > they're synchronizing with. If there are multiple callers to Set() than there > might as well not be barriers as the IsSet() callers can't assume anything about > the state of memory anyways (they're synced with "a" thread/sequence but don't > know which one...). If there are multiple callers to Set(), IsSet() callers can't assume anything about the state of memory even if there is a SequenceChecker. Because of that, I don't think that making sure that all calls to Set() occur on the same sequence is useful. However, SequenceChecker prevents Set() from being called from a PARALLEL task, which is nice because it forces users to think about memory synchronization.
On 2016/08/01 16:15:51, fdoray wrote: > lgtm > > https://codereview.chromium.org/2187333002/diff/90001/base/synchronization/at... > File base/synchronization/atomic_flag.h (right): > > https://codereview.chromium.org/2187333002/diff/90001/base/synchronization/at... > base/synchronization/atomic_flag.h:36: void UnsafeReset(); > Keep ForTesting() if you no longer need to call this outside of tests. I'm happy to, but I could also see it being fine as part of the API even if only used in test for now. Don't feel strongly, I'll go with whatever Dana prefers. > > https://codereview.chromium.org/2187333002/diff/90001/base/synchronization/at... > base/synchronization/atomic_flag.h:40: SequenceChecker set_sequence_checker_; > On 2016/08/01 15:08:29, gab wrote: > > On 2016/07/29 20:41:37, fdoray wrote: > > > Given that there are no guarantees about the synchronization of memory > > > operations made after the first call to Set(), I'm not convinced that > > > SequenceChecker is useful (Why should we disallow calls to Set() from > > different > > > sequences if IsSet() callers cannot really assume anything about memory > > > synchronization when multiple calls to Set() are possible?) > > > > > > I'm not strongly against keeping the SequenceChecker if you think it's > useful. > > > > I think it's useful because the IsSet() callers can at least know which > sequence > > they're synchronizing with. If there are multiple callers to Set() than there > > might as well not be barriers as the IsSet() callers can't assume anything > about > > the state of memory anyways (they're synced with "a" thread/sequence but don't > > know which one...). > > If there are multiple callers to Set(), IsSet() callers can't assume anything > about the state of memory even if there is a SequenceChecker. Because of that, I > don't think that making sure that all calls to Set() occur on the same sequence > is useful. I disagree, they can still assume things that had to be set on that sequence before any call to Set(). Imagine a scenario where a component sets non-atomic state before setting the flag (always in the same way), then the IsSet() caller can assume that state. > > However, SequenceChecker prevents Set() from being called from a PARALLEL task, > which is nice because it forces users to think about memory synchronization.
https://codereview.chromium.org/2187333002/diff/90001/base/synchronization/at... File base/synchronization/atomic_flag.h (right): https://codereview.chromium.org/2187333002/diff/90001/base/synchronization/at... base/synchronization/atomic_flag.h:33: // UnsafeReset(), the AtomicFlag is safe and regains it's initial properties I dont like the comment saying it becomes safe by calling an unsafe method. https://codereview.chromium.org/2187333002/diff/90001/base/synchronization/at... base/synchronization/atomic_flag.h:36: void UnsafeReset(); On 2016/08/01 16:15:51, fdoray wrote: > Keep ForTesting() if you no longer need to call this outside of tests. I agree.
Patchset #4 (id:110001) has been deleted
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Done, PTAnL, thanks! https://codereview.chromium.org/2187333002/diff/90001/base/synchronization/at... File base/synchronization/atomic_flag.h (right): https://codereview.chromium.org/2187333002/diff/90001/base/synchronization/at... base/synchronization/atomic_flag.h:33: // UnsafeReset(), the AtomicFlag is safe and regains it's initial properties On 2016/08/01 17:14:56, danakj_OOO_back_8.8 wrote: > I dont like the comment saying it becomes safe by calling an unsafe method. Well it's "unsafe" unless external measures (as described) are taken to make it safe. But okay to keep it as ForTesting for now. https://codereview.chromium.org/2187333002/diff/90001/base/synchronization/at... File base/synchronization/atomic_flag_unittest.cc (right): https://codereview.chromium.org/2187333002/diff/90001/base/synchronization/at... base/synchronization/atomic_flag_unittest.cc:37: void BusyWaitUntilFlagIsSet(AtomicFlag* flag, const base::Closure& callback) { On 2016/07/28 19:57:22, danakj_OOO_back_8.8 wrote: > nit: this would be easier to read if you just passed the bool* and flag* here > instead of a callback. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
gab@chromium.org changed reviewers: + thestig@chromium.org
@thestig in Dana's absence (no pending comments remaining, merely need final stamp), thanks!
OOO LGTM :)
The CQ bit was unchecked by gab@chromium.org
gab@chromium.org changed reviewers: - thestig@chromium.org
On 2016/08/01 18:57:32, danakj_OOO_back_8.8 wrote: > OOO LGTM :) Thanks, Lei to CC :-)
The CQ bit was checked by gab@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fdoray@chromium.org Link to the patchset: https://codereview.chromium.org/2187333002/#ps150001 (title: "fix compile post merge")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from
==========
Loosen/improve AtomicFlag semantics.
In particular don't attach to construction thread, the requirement
was I think trying to articulate that it doesn't make sense to call
Set() from different threads as it's then not clear which thread
IsSet() is memory consistent with.
But:
1) What's required is *sequence* not *thread* checking.
2) Calls to Set() need to be sequenced w.r.t. to each other, but not
with respect to construction (AtomicFlag is not modified before Set()).
Update restrictions to reflect that and update comments to reflect the
guarantees provided by them + its usage of atomics/barriers.
Also expose UnsafeReset() outside of "testing", it will be used in
https://codereview.chromium.org/2135413003/.
BUG=629716
==========
to
==========
Loosen/improve AtomicFlag semantics.
In particular don't attach to construction thread, the requirement
was I think trying to articulate that it doesn't make sense to call
Set() from different threads as it's then not clear which thread
IsSet() is memory consistent with.
But:
1) What's required is *sequence* not *thread* checking.
2) Calls to Set() need to be sequenced w.r.t. to each other, but not
with respect to construction (AtomicFlag is not modified before Set()).
Update restrictions to reflect that and update comments to reflect the
guarantees provided by them + its usage of atomics/barriers.
Also expose UnsafeReset() outside of "testing", it will be used in
https://codereview.chromium.org/2135413003/.
BUG=629716
==========
Message was sent while issue was closed.
Committed patchset #5 (id:150001)
Message was sent while issue was closed.
Description was changed from
==========
Loosen/improve AtomicFlag semantics.
In particular don't attach to construction thread, the requirement
was I think trying to articulate that it doesn't make sense to call
Set() from different threads as it's then not clear which thread
IsSet() is memory consistent with.
But:
1) What's required is *sequence* not *thread* checking.
2) Calls to Set() need to be sequenced w.r.t. to each other, but not
with respect to construction (AtomicFlag is not modified before Set()).
Update restrictions to reflect that and update comments to reflect the
guarantees provided by them + its usage of atomics/barriers.
Also expose UnsafeReset() outside of "testing", it will be used in
https://codereview.chromium.org/2135413003/.
BUG=629716
==========
to
==========
Loosen/improve AtomicFlag semantics.
In particular don't attach to construction thread, the requirement
was I think trying to articulate that it doesn't make sense to call
Set() from different threads as it's then not clear which thread
IsSet() is memory consistent with.
But:
1) What's required is *sequence* not *thread* checking.
2) Calls to Set() need to be sequenced w.r.t. to each other, but not
with respect to construction (AtomicFlag is not modified before Set()).
Update restrictions to reflect that and update comments to reflect the
guarantees provided by them + its usage of atomics/barriers.
Also expose UnsafeReset() outside of "testing", it will be used in
https://codereview.chromium.org/2135413003/.
BUG=629716
Committed: https://crrev.com/d0cc00da87f2e1ccae783ad876848518b1c389f0
Cr-Commit-Position: refs/heads/master@{#409020}
==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/d0cc00da87f2e1ccae783ad876848518b1c389f0 Cr-Commit-Position: refs/heads/master@{#409020} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
