Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2593803002:
Make sync's change processors sequence-affine. (Closed)
DescriptionMake sync's change processors sequence-affine.
These classes are thread-unsafe not thread-affine. They require
external synchronization between tasks; which a sequence brings them
implicitly. They do not need to be used from a single thread.
Note: Classes that do not depend on a constant thread id (implies that
they do not use thread-local storage) are usually not thread-affine.
BUG=675800, 675631
Committed: https://crrev.com/ab2ab7ec3a72fcb6c023f0ccbf7971af1031805a
Cr-Commit-Position: refs/heads/master@{#440195}
Patch Set 1 #
Total comments: 6
Patch Set 2 : CR gab #3 #
Total comments: 6
Patch Set 3 : CR #
Messages
Total messages: 27 (16 generated)
Description was changed from ========== history bug BUG= ========== to ========== Make sync's change processors sequence-affine. These classes are thread-unsafe not thread-affine (i.e. they require external synchronization between tasks; which a sequence brings them implicitly). BUG=675800, 675631 ==========
gab@chromium.org changed reviewers: + gab@chromium.org
lgtm w/ comments, also added a CL desc, thanks :) https://codereview.chromium.org/2593803002/diff/1/components/sync/driver/shar... File components/sync/driver/shared_change_processor.cc (right): https://codereview.chromium.org/2593803002/diff/1/components/sync/driver/shar... components/sync/driver/shared_change_processor.cc:10: #include "base/threading/thread_task_runner_handle.h" rm? https://codereview.chromium.org/2593803002/diff/1/components/sync/driver/shar... File components/sync/driver/shared_change_processor.h (right): https://codereview.chromium.org/2593803002/diff/1/components/sync/driver/shar... components/sync/driver/shared_change_processor.h:16: #include "base/single_thread_task_runner.h" rm? https://codereview.chromium.org/2593803002/diff/1/components/sync/driver/shar... components/sync/driver/shared_change_processor.h:83: // sequence. After this, all attempts to interact with the change processor by s/May be called from any thread./This method is thread-safe./
Description was changed from ========== Make sync's change processors sequence-affine. These classes are thread-unsafe not thread-affine (i.e. they require external synchronization between tasks; which a sequence brings them implicitly). BUG=675800, 675631 ========== to ========== Make sync's change processors sequence-affine. These classes are thread-unsafe not thread-affine. They require external synchronization between tasks; which a sequence brings them implicitly. They do not need to always be used from the same thread. BUG=675800, 675631 ==========
https://codereview.chromium.org/2593803002/diff/1/components/sync/driver/shar... File components/sync/driver/shared_change_processor.cc (right): https://codereview.chromium.org/2593803002/diff/1/components/sync/driver/shar... components/sync/driver/shared_change_processor.cc:10: #include "base/threading/thread_task_runner_handle.h" On 2016/12/20 20:11:29, gab wrote: > rm? Still needed to initialize |frontend_task_runner_|. https://codereview.chromium.org/2593803002/diff/1/components/sync/driver/shar... File components/sync/driver/shared_change_processor.h (right): https://codereview.chromium.org/2593803002/diff/1/components/sync/driver/shar... components/sync/driver/shared_change_processor.h:16: #include "base/single_thread_task_runner.h" On 2016/12/20 20:11:29, gab wrote: > rm? Still needed for |frontend_task_runner_|. https://codereview.chromium.org/2593803002/diff/1/components/sync/driver/shar... components/sync/driver/shared_change_processor.h:83: // sequence. After this, all attempts to interact with the change processor by On 2016/12/20 20:11:29, gab wrote: > s/May be called from any thread./This method is thread-safe./ Done.
fdoray@chromium.org changed reviewers: + maxbogue@chromium.org
maxbogue@: PTAL
Description was changed from ========== Make sync's change processors sequence-affine. These classes are thread-unsafe not thread-affine. They require external synchronization between tasks; which a sequence brings them implicitly. They do not need to always be used from the same thread. BUG=675800, 675631 ========== to ========== Make sync's change processors sequence-affine. These classes are thread-unsafe not thread-affine. They require external synchronization between tasks; which a sequence brings them implicitly. They do not need to be used from a single thread. BUG=675800, 675631 ==========
Description was changed from ========== Make sync's change processors sequence-affine. These classes are thread-unsafe not thread-affine. They require external synchronization between tasks; which a sequence brings them implicitly. They do not need to be used from a single thread. BUG=675800, 675631 ========== to ========== Make sync's change processors sequence-affine. These classes are thread-unsafe not thread-affine. They require external synchronization between tasks; which a sequence brings them implicitly. They do not need to be used from a single thread. Note: Classes that do not depend on a constant thread id (implies that they do not use thread-local storage) are usually not thread-affine. BUG=675800, 675631 ==========
fdoray@chromium.org changed reviewers: + stanisc@chromium.org
+stanisc@: PTAL (maxbogue@ OOO)
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.
lgtm with a couple of suggestions to consider https://codereview.chromium.org/2593803002/diff/20001/components/sync/driver/... File components/sync/driver/generic_change_processor.h (left): https://codereview.chromium.org/2593803002/diff/20001/components/sync/driver/... components/sync/driver/generic_change_processor.h:51: public base::NonThreadSafe { I think it would be nice to have a new class for sequence checking that is a replacement for NonThreadSafe. That would be more convenient (less code, easier to read) than using sequence_checkher_ member. https://codereview.chromium.org/2593803002/diff/20001/components/sync/driver/... File components/sync/driver/shared_change_processor.h (right): https://codereview.chromium.org/2593803002/diff/20001/components/sync/driver/... components/sync/driver/shared_change_processor.h:147: // The sequence that all methods except the constructor, destructor, and I personally find the term sequence confusing in this context - "sequence that a method should be called on". This term is way too general and usually used for something completely different (e.g. data sequence). Thread might not be the right term anymore but at least it was less ambiguous and easier to understand.
https://codereview.chromium.org/2593803002/diff/20001/components/sync/driver/... File components/sync/driver/generic_change_processor.h (left): https://codereview.chromium.org/2593803002/diff/20001/components/sync/driver/... components/sync/driver/generic_change_processor.h:51: public base::NonThreadSafe { On 2016/12/21 18:48:09, stanisc wrote: > I think it would be nice to have a new class for sequence checking that is a > replacement for NonThreadSafe. That would be more convenient (less code, easier > to read) than using sequence_checkher_ member. Note that composition is prefered to inheritance by style guide, even for this, as was discussed here: https://groups.google.com/a/chromium.org/d/topic/chromium-dev/TivgMWBuTKI/dis... Also, FWIW, sequences provide thread-safety. So maybe we should just update NonThreadSafe to be sequence-affine instead of thread-affine (requires survey of codebase to make sure no one uses this in thread-affine scenarios...). https://codereview.chromium.org/2593803002/diff/20001/components/sync/driver/... File components/sync/driver/shared_change_processor.h (right): https://codereview.chromium.org/2593803002/diff/20001/components/sync/driver/... components/sync/driver/shared_change_processor.h:147: // The sequence that all methods except the constructor, destructor, and On 2016/12/21 18:48:09, stanisc wrote: > I personally find the term sequence confusing in this context - "sequence that a > method should be called on". This term is way too general and usually used for > something completely different (e.g. data sequence). > > Thread might not be the right term anymore but at least it was less ambiguous > and easier to understand. "execution sequence"?
https://codereview.chromium.org/2593803002/diff/20001/components/sync/driver/... File components/sync/driver/shared_change_processor.h (right): https://codereview.chromium.org/2593803002/diff/20001/components/sync/driver/... components/sync/driver/shared_change_processor.h:147: // The sequence that all methods except the constructor, destructor, and On 2016/12/21 18:52:17, gab wrote: > On 2016/12/21 18:48:09, stanisc wrote: > > I personally find the term sequence confusing in this context - "sequence that > a > > method should be called on". This term is way too general and usually used for > > something completely different (e.g. data sequence). > > > > Thread might not be the right term anymore but at least it was less ambiguous > > and easier to understand. > > "execution sequence"? execution sequence is better. Or task sequence. Or something new that is shorter and less general.
https://codereview.chromium.org/2593803002/diff/20001/components/sync/driver/... File components/sync/driver/shared_change_processor.h (right): https://codereview.chromium.org/2593803002/diff/20001/components/sync/driver/... components/sync/driver/shared_change_processor.h:147: // The sequence that all methods except the constructor, destructor, and On 2016/12/21 18:57:27, stanisc wrote: > On 2016/12/21 18:52:17, gab wrote: > > On 2016/12/21 18:48:09, stanisc wrote: > > > I personally find the term sequence confusing in this context - "sequence > that > > a > > > method should be called on". This term is way too general and usually used > for > > > something completely different (e.g. data sequence). > > > > > > Thread might not be the right term anymore but at least it was less > ambiguous > > > and easier to understand. > > > > "execution sequence"? > > execution sequence is better. Or task sequence. > Or something new that is shorter and less general. Done.
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org, stanisc@chromium.org Link to the patchset: https://codereview.chromium.org/2593803002/#ps40001 (title: "CR")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1482347575110510,
"parent_rev": "ce8254e32ae0188b5b5b37199fe431a3fff0fcd7", "commit_rev":
"b50710c63e2715882c38c6c2652b7f341cb15d84"}
Message was sent while issue was closed.
Description was changed from ========== Make sync's change processors sequence-affine. These classes are thread-unsafe not thread-affine. They require external synchronization between tasks; which a sequence brings them implicitly. They do not need to be used from a single thread. Note: Classes that do not depend on a constant thread id (implies that they do not use thread-local storage) are usually not thread-affine. BUG=675800, 675631 ========== to ========== Make sync's change processors sequence-affine. These classes are thread-unsafe not thread-affine. They require external synchronization between tasks; which a sequence brings them implicitly. They do not need to be used from a single thread. Note: Classes that do not depend on a constant thread id (implies that they do not use thread-local storage) are usually not thread-affine. BUG=675800, 675631 Review-Url: https://codereview.chromium.org/2593803002 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Make sync's change processors sequence-affine. These classes are thread-unsafe not thread-affine. They require external synchronization between tasks; which a sequence brings them implicitly. They do not need to be used from a single thread. Note: Classes that do not depend on a constant thread id (implies that they do not use thread-local storage) are usually not thread-affine. BUG=675800, 675631 Review-Url: https://codereview.chromium.org/2593803002 ========== to ========== Make sync's change processors sequence-affine. These classes are thread-unsafe not thread-affine. They require external synchronization between tasks; which a sequence brings them implicitly. They do not need to be used from a single thread. Note: Classes that do not depend on a constant thread id (implies that they do not use thread-local storage) are usually not thread-affine. BUG=675800, 675631 Committed: https://crrev.com/ab2ab7ec3a72fcb6c023f0ccbf7971af1031805a Cr-Commit-Position: refs/heads/master@{#440195} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/ab2ab7ec3a72fcb6c023f0ccbf7971af1031805a Cr-Commit-Position: refs/heads/master@{#440195} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
