Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2590443005:
Add TaskTraits::MayBlock and TaskTraits::WithSyncPrimitives. (Closed)
DescriptionAdd TaskTraits::MayBlock and TaskTraits::WithSyncPrimitives.
See https://docs.google.com/a/chromium.org/document/d/1ynRvQKah3Cx_eLqPS7KzIp8u73HeKIjX7HQqwSWOnF8/edit?usp=sharing
TBR=danakj@chromium.org
BUG=553459
Committed: https://crrev.com/35ff8f3750ab7b8a796ab7c0b43e5784c76c84b8
Cr-Commit-Position: refs/heads/master@{#439949}
Patch Set 1 #Patch Set 2 : self-review #
Total comments: 27
Patch Set 3 : CR gab #4 #Patch Set 4 : fix build error #
Total comments: 4
Patch Set 5 : CR robliao #7 #
Total comments: 10
Patch Set 6 : CR #
Messages
Total messages: 24 (9 generated)
fdoray@chromium.org changed reviewers: + gab@chromium.org, robliao@chromium.org
PTAL
https://codereview.chromium.org/2590443005/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/2590443005/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl_unittest.cc:174: ? BACKGROUND_MAY_BLOCK_WORKER_POOL naming scheme here and elsewhere: s/may block/blocking ? https://codereview.chromium.org/2590443005/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_traits.cc (right): https://codereview.chromium.org/2590443005/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_traits.cc:18: // request defaults if the behavior is critical to the task. Move this comment and member initialization to members in header (only keep = default constructor here). https://codereview.chromium.org/2590443005/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_traits.h (right): https://codereview.chromium.org/2590443005/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_traits.h:102: // variables and to join threads and processes. This trait implies MayBlock(). s/and to/as well as/ https://codereview.chromium.org/2590443005/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_traits.h:104: // In general, you should not use this trait. // This trait should generally not be used. (to avoid "you") https://codereview.chromium.org/2590443005/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_traits.h:105: // Check with jam@ whether we should make this trait require friend'ing (to have OWNERS control over usage like for thread_restrictions.h). Otherwise we leak a way to make a task be allowed to wait without it requiring a new friend decl. https://codereview.chromium.org/2590443005/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_traits.h:107: // work that you would have done after the wait in a callback and post this s/that you would have done/that should happen/ https://codereview.chromium.org/2590443005/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_traits.h:107: // work that you would have done after the wait in a callback and post this s/this callback/that callback/ https://codereview.chromium.org/2590443005/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_traits.h:108: // callback from where you would have signaled the waitable event or condition s/you would have signaled the WE or CV/from where the WE or CV would have been signaled/ https://codereview.chromium.org/2590443005/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_traits.h:110: // base::BarrierClosure. // If something needs to be scheduled after many tasks have executed, use ... https://codereview.chromium.org/2590443005/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_traits.h:116: // need a thread, make it non-joinable and add cleanup work at the end of the If a thread is really needed, https://codereview.chromium.org/2590443005/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_traits.h:117: // thread's main function (if you use base::Thread, override Cleanup()). if using base::Thread, ... https://codereview.chromium.org/2590443005/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_traits.h:121: // TODO(fdoray): Remove this crbug.com/675660 Removes this as part of crbug... ^^^^^^^^^^
fdoray@chromium.org changed reviewers: + jam@chromium.org
gab@: PTAnL jam@: Please see comment addressed to you. https://codereview.chromium.org/2590443005/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/2590443005/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl_unittest.cc:174: ? BACKGROUND_MAY_BLOCK_WORKER_POOL On 2016/12/19 20:20:14, gab wrote: > naming scheme here and elsewhere: s/may block/blocking ? Done. https://codereview.chromium.org/2590443005/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_traits.cc (right): https://codereview.chromium.org/2590443005/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_traits.cc:18: // request defaults if the behavior is critical to the task. On 2016/12/19 20:20:14, gab wrote: > Move this comment and member initialization to members in header (only keep = > default constructor here). Unfortunately, these is an include cycle if I include scoped_set_task_priority_for_current_thread.h in task_traits.h https://codereview.chromium.org/2590443005/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_traits.h (right): https://codereview.chromium.org/2590443005/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_traits.h:102: // variables and to join threads and processes. This trait implies MayBlock(). On 2016/12/19 20:20:14, gab wrote: > s/and to/as well as/ Done. https://codereview.chromium.org/2590443005/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_traits.h:104: // In general, you should not use this trait. On 2016/12/19 20:20:14, gab wrote: > // This trait should generally not be used. > > (to avoid "you") Done. https://codereview.chromium.org/2590443005/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_traits.h:105: // On 2016/12/19 20:20:14, gab wrote: > Check with jam@ whether we should make this trait require friend'ing (to have > OWNERS control over usage like for thread_restrictions.h). Otherwise we leak a > way to make a task be allowed to wait without it requiring a new friend decl. jam@: wdyt? Note that today, you only need to be a friend of ThreadRestrictions to wait from a thread that explicitly disallows waiting. Tasks with .WithSyncPrimitives() a scheduled in a pool dedicated to blocking tasks, so we're not really leaking a way to wait from somewhere you're not supposed to. https://codereview.chromium.org/2590443005/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_traits.h:107: // work that you would have done after the wait in a callback and post this On 2016/12/19 20:20:14, gab wrote: > s/this callback/that callback/ Done. https://codereview.chromium.org/2590443005/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_traits.h:107: // work that you would have done after the wait in a callback and post this On 2016/12/19 20:20:14, gab wrote: > s/that you would have done/that should happen/ Done. https://codereview.chromium.org/2590443005/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_traits.h:108: // callback from where you would have signaled the waitable event or condition On 2016/12/19 20:20:14, gab wrote: > s/you would have signaled the WE or CV/from where the WE or CV would have been > signaled/ Done. https://codereview.chromium.org/2590443005/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_traits.h:110: // base::BarrierClosure. On 2016/12/19 20:20:14, gab wrote: > // If something needs to be scheduled after many tasks have executed, use ... Done. https://codereview.chromium.org/2590443005/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_traits.h:116: // need a thread, make it non-joinable and add cleanup work at the end of the On 2016/12/19 20:20:14, gab wrote: > If a thread is really needed, Done. https://codereview.chromium.org/2590443005/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_traits.h:117: // thread's main function (if you use base::Thread, override Cleanup()). On 2016/12/19 20:20:14, gab wrote: > if using base::Thread, ... Done. https://codereview.chromium.org/2590443005/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_traits.h:121: // TODO(fdoray): Remove this crbug.com/675660 On 2016/12/19 20:20:14, gab wrote: > Removes this as part of crbug... > ^^^^^^^^^^ Done.
https://codereview.chromium.org/2590443005/diff/60001/base/task_scheduler/tas... File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/2590443005/diff/60001/base/task_scheduler/tas... base/task_scheduler/task_tracker.cc:229: const bool previous_wait_allowed = Rename this while we're here. https://codereview.chromium.org/2590443005/diff/60001/base/task_scheduler/tas... File base/task_scheduler/task_traits.h (right): https://codereview.chromium.org/2590443005/diff/60001/base/task_scheduler/tas... base/task_scheduler/task_traits.h:96: // directory, etc. It is not necessary to use this trait to use locks. For s/It is not necessary to use this trait to use/Do not use this trait just to use
https://codereview.chromium.org/2590443005/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_traits.h (right): https://codereview.chromium.org/2590443005/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_traits.h:105: // On 2016/12/19 21:22:43, fdoray wrote: > On 2016/12/19 20:20:14, gab wrote: > > Check with jam@ whether we should make this trait require friend'ing (to have > > OWNERS control over usage like for thread_restrictions.h). Otherwise we leak a > > way to make a task be allowed to wait without it requiring a new friend decl. > > jam@: wdyt? > > Note that today, you only need to be a friend of ThreadRestrictions to wait from > a thread that explicitly disallows waiting. Tasks with .WithSyncPrimitives() a > scheduled in a pool dedicated to blocking tasks, so we're not really leaking a > way to wait from somewhere you're not supposed to. If it's only used on non UI/IO threads that are meant to have blocking tasks, no friending needed. The thread_Restrictions friend this was done specifically to not allow blocking the UI/IO threads. Thanks for checking
PTAnL https://codereview.chromium.org/2590443005/diff/60001/base/task_scheduler/tas... File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/2590443005/diff/60001/base/task_scheduler/tas... base/task_scheduler/task_tracker.cc:229: const bool previous_wait_allowed = On 2016/12/19 22:04:56, robliao - OOO 12-21 to 1-2017 wrote: > Rename this while we're here. This variable still contains the previous value of the "wait allowed" thread restriction. I plan to rename ThreadRestrictions::SetWaitAllowed and this variable in a separate CL. https://codereview.chromium.org/2590443005/diff/60001/base/task_scheduler/tas... File base/task_scheduler/task_traits.h (right): https://codereview.chromium.org/2590443005/diff/60001/base/task_scheduler/tas... base/task_scheduler/task_traits.h:96: // directory, etc. It is not necessary to use this trait to use locks. For On 2016/12/19 22:04:56, robliao - OOO 12-21 to 1-2017 wrote: > s/It is not necessary to use this trait to use/Do not use this trait just to use Done.
lgtm w/ comments https://codereview.chromium.org/2590443005/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/2590443005/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl_unittest.cc:174: ? BACKGROUND_MAY_BLOCK_WORKER_POOL On 2016/12/19 21:22:43, fdoray wrote: > On 2016/12/19 20:20:14, gab wrote: > > naming scheme here and elsewhere: s/may block/blocking ? > > Done. Should we not also rename pools in TaskSchedulerImpl? https://codereview.chromium.org/2590443005/diff/80001/base/task_scheduler/tas... File base/task_scheduler/task_traits.h (right): https://codereview.chromium.org/2590443005/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_traits.h:95: // disk, a pipe or a socket, rename or delete a file, enumerate files in a interact with a pipe or a socket ^^^^^^^^ (i.e. every enumeration begins with a verb and this one should too) https://codereview.chromium.org/2590443005/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_traits.h:96: // directory, etc. Do not use this trait just to use locks. For tasks that s/Do not use this trait just to use locks./This trait isn't required for the mere use of locks./ ? https://codereview.chromium.org/2590443005/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_traits.h:97: // join threads or processes or wait on a waitable event or condition threads/processes or wait on a WaitableEvent/ConditionVariable (to try to reduce "or"s ?)
lgtm https://codereview.chromium.org/2590443005/diff/80001/base/task_scheduler/tas... File base/task_scheduler/task_traits.h (right): https://codereview.chromium.org/2590443005/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_traits.h:96: // directory, etc. Do not use this trait just to use locks. For tasks that On 2016/12/20 17:13:53, gab wrote: > s/Do not use this trait just to use locks./This trait isn't required for the > mere use of locks./ ? I strengthened the wording to make it clear that if you're just using locks, don't use this trait. If we're concerned about skimmers, we also need to be very strong in our language. https://codereview.chromium.org/2590443005/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_traits.h:97: // join threads or processes or wait on a waitable event or condition On 2016/12/20 17:13:54, gab wrote: > threads/processes or wait on a WaitableEvent/ConditionVariable > > (to try to reduce "or"s ?) Could be For tasks that block on synchronization primitives (thread handles, waitable events, condition variables), see WithSyncPrimitives(). Or For tasks that interact with synchronization primitives like joining against a thread, waiting on WaitableEvent, or waiting on a ConditionVariable, see WithSyncPrimitives().
https://codereview.chromium.org/2590443005/diff/80001/chrome/browser/prefs/in... File chrome/browser/prefs/incognito_mode_prefs.cc (left): https://codereview.chromium.org/2590443005/diff/80001/chrome/browser/prefs/in... chrome/browser/prefs/incognito_mode_prefs.cc:78: base::ThreadRestrictions::AssertWaitAllowed(); Nit: This change isn't particularly topical to this CL. Move this to a separate CL.
Description was changed from ========== Add TaskTraits::MayBlock and TaskTraits::WithSyncPrimitives. See https://docs.google.com/a/chromium.org/document/d/1ynRvQKah3Cx_eLqPS7KzIp8u73... BUG=553459 ========== to ========== Add TaskTraits::MayBlock and TaskTraits::WithSyncPrimitives. See https://docs.google.com/a/chromium.org/document/d/1ynRvQKah3Cx_eLqPS7KzIp8u73... TBR=danakj@chromium.org BUG=553459 ==========
fdoray@chromium.org changed reviewers: + danakj@chromium.org
TBR danakj@ for sequenced_worker_pool.cc https://codereview.chromium.org/2590443005/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/2590443005/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl_unittest.cc:174: ? BACKGROUND_MAY_BLOCK_WORKER_POOL On 2016/12/20 17:13:53, gab wrote: > On 2016/12/19 21:22:43, fdoray wrote: > > On 2016/12/19 20:20:14, gab wrote: > > > naming scheme here and elsewhere: s/may block/blocking ? > > > > Done. > > Should we not also rename pools in TaskSchedulerImpl? In a separate CL, because histograms depend on this. https://codereview.chromium.org/2590443005/diff/80001/base/task_scheduler/tas... File base/task_scheduler/task_traits.h (right): https://codereview.chromium.org/2590443005/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_traits.h:95: // disk, a pipe or a socket, rename or delete a file, enumerate files in a On 2016/12/20 17:13:54, gab wrote: > interact with a pipe or a socket > ^^^^^^^^ > > (i.e. every enumeration begins with a verb and this one should too) Done. https://codereview.chromium.org/2590443005/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_traits.h:96: // directory, etc. Do not use this trait just to use locks. For tasks that On 2016/12/20 17:13:53, gab wrote: > s/Do not use this trait just to use locks./This trait isn't required for the > mere use of locks./ ? Done. https://codereview.chromium.org/2590443005/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_traits.h:97: // join threads or processes or wait on a waitable event or condition On 2016/12/20 18:27:50, robliao - OOO 12-21 to 1-2017 wrote: > On 2016/12/20 17:13:54, gab wrote: > > threads/processes or wait on a WaitableEvent/ConditionVariable > > > > (to try to reduce "or"s ?) > > Could be > > For tasks that block on synchronization primitives (thread handles, waitable > events, condition variables), see WithSyncPrimitives(). > > Or > > For tasks that interact with synchronization primitives like joining against a > thread, waiting on WaitableEvent, or waiting on a ConditionVariable, see > WithSyncPrimitives(). Done. https://codereview.chromium.org/2590443005/diff/80001/chrome/browser/prefs/in... File chrome/browser/prefs/incognito_mode_prefs.cc (left): https://codereview.chromium.org/2590443005/diff/80001/chrome/browser/prefs/in... chrome/browser/prefs/incognito_mode_prefs.cc:78: base::ThreadRestrictions::AssertWaitAllowed(); On 2016/12/20 18:28:30, robliao - OOO 12-21 to 1-2017 wrote: > Nit: This change isn't particularly topical to this CL. Move this to a separate > CL. Done.
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robliao@chromium.org, gab@chromium.org Link to the patchset: https://codereview.chromium.org/2590443005/#ps100001 (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": 100001, "attempt_start_ts": 1482267494409270,
"parent_rev": "84af02eadc766d95c90eb457c3cba43f59e86198", "commit_rev":
"476688d37c62162af296c83bca7b0d169965382b"}
Message was sent while issue was closed.
Description was changed from ========== Add TaskTraits::MayBlock and TaskTraits::WithSyncPrimitives. See https://docs.google.com/a/chromium.org/document/d/1ynRvQKah3Cx_eLqPS7KzIp8u73... TBR=danakj@chromium.org BUG=553459 ========== to ========== Add TaskTraits::MayBlock and TaskTraits::WithSyncPrimitives. See https://docs.google.com/a/chromium.org/document/d/1ynRvQKah3Cx_eLqPS7KzIp8u73... TBR=danakj@chromium.org BUG=553459 Review-Url: https://codereview.chromium.org/2590443005 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Add TaskTraits::MayBlock and TaskTraits::WithSyncPrimitives. See https://docs.google.com/a/chromium.org/document/d/1ynRvQKah3Cx_eLqPS7KzIp8u73... TBR=danakj@chromium.org BUG=553459 Review-Url: https://codereview.chromium.org/2590443005 ========== to ========== Add TaskTraits::MayBlock and TaskTraits::WithSyncPrimitives. See https://docs.google.com/a/chromium.org/document/d/1ynRvQKah3Cx_eLqPS7KzIp8u73... TBR=danakj@chromium.org BUG=553459 Committed: https://crrev.com/35ff8f3750ab7b8a796ab7c0b43e5784c76c84b8 Cr-Commit-Position: refs/heads/master@{#439949} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/35ff8f3750ab7b8a796ab7c0b43e5784c76c84b8 Cr-Commit-Position: refs/heads/master@{#439949}
Message was sent while issue was closed.
LGTM |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
