Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2531663003:
TaskScheduler: Add TaskTraits::WithWait(). (Closed)
DescriptionTaskScheduler: Add TaskTraits::WithWait().
WithWait() tasks are allowed to wait on other things than file I/O. In
particular, they may wait on a WaitableEvent or a ConditionVariable, join a
thread or a process, or make a blocking system call that doesn't involve
interactions with the file system.
In upcoming CLs, this trait will be used in scheduling decisions (more
details on the bug).
BUG=669247
Committed: https://crrev.com/19509c12031a760169479b34fe6408a093b71900
Cr-Commit-Position: refs/heads/master@{#435054}
Patch Set 1 #Patch Set 2 : CR gab #3 (enforce AssertWaitAllowed requires WithWait) #
Total comments: 6
Patch Set 3 : CR robliao #9 (wording, order of ThreadRestrictions reset) #
Total comments: 2
Patch Set 4 : CR danakj #14 (no TEST_P) #
Total comments: 4
Patch Set 5 : rebase #Patch Set 6 : CR danakj #22 (TEST_F -> TEST) #Patch Set 7 : CR danakj #22 (AssertWaitAllowed) #
Messages
Total messages: 33 (17 generated)
fdoray@chromium.org changed reviewers: + gab@chromium.org
PTAL
Don't we want to also enforce this in this CL? (otherwise the CL that tries to enforce it later might bump into use cases where this trait was used (or not used) incorrectly and have to fix those after the fact..)
fdoray@chromium.org changed reviewers: + robliao@chromium.org
PTAnL
The CQ bit was checked by fdoray@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...
lgtm
lgtm + nits. https://codereview.chromium.org/2531663003/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/2531663003/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_tracker.cc:268: ThreadRestrictions::SetWaitAllowed(previous_wait_allowed); Nit: Order above SetIOAllowed for symmetry. We should probably do the same with SetSingletonAllowed. https://codereview.chromium.org/2531663003/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_traits.h (right): https://codereview.chromium.org/2531663003/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_traits.h:95: // Allows tasks with these traits to wait on other things than file I/O. In Nit: wait on other things that file I/O -> wait on things other than file I/O https://codereview.chromium.org/2531663003/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_traits.h:110: // Returns true if waiting on other things than file I/O is allowed by these Same as above.
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
fdoray@chromium.org changed reviewers: + danakj@chromium.org
danakj@: PTAL https://codereview.chromium.org/2531663003/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/2531663003/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_tracker.cc:268: ThreadRestrictions::SetWaitAllowed(previous_wait_allowed); On 2016/11/28 20:09:50, robliao wrote: > Nit: Order above SetIOAllowed for symmetry. We should probably do the same with > SetSingletonAllowed. Done. https://codereview.chromium.org/2531663003/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_traits.h (right): https://codereview.chromium.org/2531663003/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_traits.h:95: // Allows tasks with these traits to wait on other things than file I/O. In On 2016/11/28 20:09:50, robliao wrote: > Nit: wait on other things that file I/O -> wait on things other than file I/O Done. https://codereview.chromium.org/2531663003/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_traits.h:110: // Returns true if waiting on other things than file I/O is allowed by these On 2016/11/28 20:09:50, robliao wrote: > Same as above. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I don't see at a high level why we want this. Will we schedule things WithWait differently in the future? https://codereview.chromium.org/2531663003/diff/40001/base/task_scheduler/tas... File base/task_scheduler/task_tracker_unittest.cc (right): https://codereview.chromium.org/2531663003/diff/40001/base/task_scheduler/tas... base/task_scheduler/task_tracker_unittest.cc:498: const TaskShutdownBehavior shutdown_behavior_; What does this have to do with what is being tested here? (why is this a TEST_P)
The CQ bit was checked by fdoray@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...
On 2016/11/28 21:59:16, danakj wrote: > I don't see at a high level why we want this. Will we schedule things WithWait > differently in the future? Yes. 1. We will update our mapping function https://cs.chromium.org/chromium/src/components/task_scheduler_util/initializ... so that WithWait() tasks run in the same pool as WithFileIO() tasks. That way, a task that waits for a long time (e.g. https://cs.chromium.org/chromium/src/chrome/browser/component_updater/recover...) won't prevent short CPU tasks from being scheduled. 2. We will add logic to create a new thread when a WithWait() task takes too much time to run. E.g. A pool has max_threads = 3. It starts running a WithWait() task. If the WithWait() task is still running after a timeout, the task is considered to be blocked. The pool will schedule tasks on a 4th thread until the task completes. Note: The heuristic described in (2) works on all platforms. On Windows and Mac, we have plans to replace it with calls to OS APIs that tell us whether tasks are really blocked.
On Mon, Nov 28, 2016 at 2:33 PM, <fdoray@chromium.org> wrote: > On 2016/11/28 21:59:16, danakj wrote: > > I don't see at a high level why we want this. Will we schedule things > WithWait > > differently in the future? > > Yes. > 1. We will update our mapping function > https://cs.chromium.org/chromium/src/components/task_ > scheduler_util/initialization_util.cc?l=94 > so that WithWait() tasks run in the same pool as WithFileIO() tasks. That > way, a > task that waits for a long time (e.g. > https://cs.chromium.org/chromium/src/chrome/browser/ > component_updater/recovery_component_installer.cc?l=148) > won't prevent short CPU tasks from being scheduled. > 2. We will add logic to create a new thread when a WithWait() task takes > too > much time to run. E.g. A pool has max_threads = 3. It starts running a > WithWait() task. If the WithWait() task is still running after a timeout, > the > task is considered to be blocked. The pool will schedule tasks on a 4th > thread > until the task completes. > > Note: The heuristic described in (2) works on all platforms. On Windows > and Mac, > we have plans to replace it with calls to OS APIs that tell us whether > tasks are > really blocked. > Ok cool, thanks. Can you drop this into the CL description (since the bug is a bit of a meta bug) or also point this at a smaller bug about this task? > > https://codereview.chromium.org/2531663003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/2531663003/diff/40001/base/task_scheduler/tas... File base/task_scheduler/task_tracker_unittest.cc (right): https://codereview.chromium.org/2531663003/diff/40001/base/task_scheduler/tas... base/task_scheduler/task_tracker_unittest.cc:498: const TaskShutdownBehavior shutdown_behavior_; On 2016/11/28 21:59:16, danakj wrote: > What does this have to do with what is being tested here? (why is this a TEST_P) Done.
Description was changed from ========== TaskScheduler: Add TaskTraits::WithWait(). WithWait() tasks are allowed to wait on other things than file I/O. In particular, they may wait on a WaitableEvent or a ConditionVariable, join a thread or a process, or make a blocking system call that doesn't involve interactions with the file system. BUG=553459 ========== to ========== TaskScheduler: Add TaskTraits::WithWait(). WithWait() tasks are allowed to wait on other things than file I/O. In particular, they may wait on a WaitableEvent or a ConditionVariable, join a thread or a process, or make a blocking system call that doesn't involve interactions with the file system. In upcoming CLs, this trait will be used in scheduling decisions (more details on the bug). BUG=669247 ==========
On 2016/11/28 22:36:45, danakj wrote: > On Mon, Nov 28, 2016 at 2:33 PM, <mailto:fdoray@chromium.org> wrote: > > > On 2016/11/28 21:59:16, danakj wrote: > > > I don't see at a high level why we want this. Will we schedule things > > WithWait > > > differently in the future? > > > > Yes. > > 1. We will update our mapping function > > https://cs.chromium.org/chromium/src/components/task_ > > scheduler_util/initialization_util.cc?l=94 > > so that WithWait() tasks run in the same pool as WithFileIO() tasks. That > > way, a > > task that waits for a long time (e.g. > > https://cs.chromium.org/chromium/src/chrome/browser/ > > component_updater/recovery_component_installer.cc?l=148) > > won't prevent short CPU tasks from being scheduled. > > 2. We will add logic to create a new thread when a WithWait() task takes > > too > > much time to run. E.g. A pool has max_threads = 3. It starts running a > > WithWait() task. If the WithWait() task is still running after a timeout, > > the > > task is considered to be blocked. The pool will schedule tasks on a 4th > > thread > > until the task completes. > > > > Note: The heuristic described in (2) works on all platforms. On Windows > > and Mac, > > we have plans to replace it with calls to OS APIs that tell us whether > > tasks are > > really blocked. > > > > Ok cool, thanks. Can you drop this into the CL description (since the bug > is a bit of a meta bug) or also point this at a smaller bug about this task? > > > > > > https://codereview.chromium.org/2531663003/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. PTAnL Created a bug and updated the CL description to say that more details can be found on the bug.
LGTM https://codereview.chromium.org/2531663003/diff/60001/base/task_scheduler/tas... File base/task_scheduler/task_tracker_unittest.cc (right): https://codereview.chromium.org/2531663003/diff/60001/base/task_scheduler/tas... base/task_scheduler/task_tracker_unittest.cc:845: // Waiting is allowed by default. Expect TaskTracker to disallow it before Can you verify this by doing AssertWaitAllowed() to show it is allowed? https://codereview.chromium.org/2531663003/diff/60001/base/task_scheduler/tas... base/task_scheduler/task_tracker_unittest.cc:874: TEST_F(TaskSchedulerTaskTrackerTest, WaitAllowed) { Looks like this could just be a TEST(TaskTrackerTest, WaitAllowed) and not inherit a bunch of code it doesn't use. That'd be a bit more clear.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2531663003/diff/60001/base/task_scheduler/tas... File base/task_scheduler/task_tracker_unittest.cc (right): https://codereview.chromium.org/2531663003/diff/60001/base/task_scheduler/tas... base/task_scheduler/task_tracker_unittest.cc:845: // Waiting is allowed by default. Expect TaskTracker to disallow it before On 2016/11/28 23:01:30, danakj wrote: > Can you verify this by doing AssertWaitAllowed() to show it is allowed? Done. https://codereview.chromium.org/2531663003/diff/60001/base/task_scheduler/tas... base/task_scheduler/task_tracker_unittest.cc:874: TEST_F(TaskSchedulerTaskTrackerTest, WaitAllowed) { On 2016/11/28 23:01:30, danakj wrote: > Looks like this could just be a TEST(TaskTrackerTest, WaitAllowed) and not > inherit a bunch of code it doesn't use. That'd be a bit more clear. 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, danakj@chromium.org Link to the patchset: https://codereview.chromium.org/2531663003/#ps120001 (title: "CR danakj #22 (AssertWaitAllowed)")
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": 120001, "attempt_start_ts": 1480434957575300,
"parent_rev": "8d570d99c2c72ac961db2749c7976d77c9240b90", "commit_rev":
"2ea9d62432412788c7e67c2884f13433dbe169a6"}
Message was sent while issue was closed.
Description was changed from ========== TaskScheduler: Add TaskTraits::WithWait(). WithWait() tasks are allowed to wait on other things than file I/O. In particular, they may wait on a WaitableEvent or a ConditionVariable, join a thread or a process, or make a blocking system call that doesn't involve interactions with the file system. In upcoming CLs, this trait will be used in scheduling decisions (more details on the bug). BUG=669247 ========== to ========== TaskScheduler: Add TaskTraits::WithWait(). WithWait() tasks are allowed to wait on other things than file I/O. In particular, they may wait on a WaitableEvent or a ConditionVariable, join a thread or a process, or make a blocking system call that doesn't involve interactions with the file system. In upcoming CLs, this trait will be used in scheduling decisions (more details on the bug). BUG=669247 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== TaskScheduler: Add TaskTraits::WithWait(). WithWait() tasks are allowed to wait on other things than file I/O. In particular, they may wait on a WaitableEvent or a ConditionVariable, join a thread or a process, or make a blocking system call that doesn't involve interactions with the file system. In upcoming CLs, this trait will be used in scheduling decisions (more details on the bug). BUG=669247 ========== to ========== TaskScheduler: Add TaskTraits::WithWait(). WithWait() tasks are allowed to wait on other things than file I/O. In particular, they may wait on a WaitableEvent or a ConditionVariable, join a thread or a process, or make a blocking system call that doesn't involve interactions with the file system. In upcoming CLs, this trait will be used in scheduling decisions (more details on the bug). BUG=669247 Committed: https://crrev.com/19509c12031a760169479b34fe6408a093b71900 Cr-Commit-Position: refs/heads/master@{#435054} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/19509c12031a760169479b34fe6408a093b71900 Cr-Commit-Position: refs/heads/master@{#435054} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
