Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2531883002:
TaskScheduler: Set the IO allowed bit in TaskTracker::RunTask(). (Closed)
DescriptionTaskScheduler: Set the IO allowed bit in TaskTracker::RunTask().
Before this CL, tasks without the WithFileIO() trait were allowed to
perform synchronous file IO operations if they were mapped to a pool
created with IORestriction::ALLOWED (this happens with a TaskScheduler
created by CreateAndSetSimpleTaskScheduler().
With this CL, the IO allowed bit is always updated before running a
task, based on the presence of the WithFileIO() trait.
BUG=553459
Committed: https://crrev.com/f7d7e2f5926a928e472a06fcffb7c7a7dd154b32
Cr-Commit-Position: refs/heads/master@{#435942}
Patch Set 1 #
Total comments: 5
Patch Set 2 : update ScopedTaskScheduler #Patch Set 3 : CR gab #12 + fix build error #Patch Set 4 : fix test error #
Total comments: 2
Patch Set 5 : CR robliao #22 (reorder reset) #
Total comments: 8
Patch Set 6 : CR danakj #32 (WrapUnique) #Patch Set 7 : rebase #Messages
Total messages: 51 (29 generated)
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...
fdoray@chromium.org changed reviewers: + gab@chromium.org, robliao@chromium.org
PTAL This is important to prevent tasks without the WithFileIO() trait to AssertIOAllowed() in a single-pool TaskScheduler (CreateAndSetSimpleTaskScheduler, ScopedTaskScheduler).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...)
On 2016/11/25 18:50:48, fdoray wrote: > PTAL > > This is important to prevent tasks without the WithFileIO() trait to > AssertIOAllowed() in a single-pool TaskScheduler > (CreateAndSetSimpleTaskScheduler, ScopedTaskScheduler). This seems to open a door where partitioned thread pools like what we had before can now potentially execute IO tasks if the mapping function is not handled correctly. Are we okay with this?
https://codereview.chromium.org/2531883002/diff/1/base/task_scheduler/schedul... File base/task_scheduler/scheduler_worker_pool_params.h (left): https://codereview.chromium.org/2531883002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_worker_pool_params.h:22: }; ScopedTaskScheduler needs updating.
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
PTAnL
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm % nits and compile :) https://codereview.chromium.org/2531883002/diff/1/base/task_scheduler/task_tr... File base/task_scheduler/task_tracker_unittest.cc (right): https://codereview.chromium.org/2531883002/diff/1/base/task_scheduler/task_tr... base/task_scheduler/task_tracker_unittest.cc:441: std::unique_ptr<Task> task_with_file_io(new Task( = MakeUnique<Task>(...) https://codereview.chromium.org/2531883002/diff/1/base/task_scheduler/task_tr... base/task_scheduler/task_tracker_unittest.cc:453: std::unique_ptr<Task> task_without_file_io(new Task( ditto
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
robliao@: PTAL https://codereview.chromium.org/2531883002/diff/1/base/task_scheduler/task_tr... File base/task_scheduler/task_tracker_unittest.cc (right): https://codereview.chromium.org/2531883002/diff/1/base/task_scheduler/task_tr... base/task_scheduler/task_tracker_unittest.cc:441: std::unique_ptr<Task> task_with_file_io(new Task( On 2016/11/28 16:45:47, gab wrote: > = MakeUnique<Task>(...) Done. https://codereview.chromium.org/2531883002/diff/1/base/task_scheduler/task_tr... base/task_scheduler/task_tracker_unittest.cc:453: std::unique_ptr<Task> task_without_file_io(new Task( On 2016/11/28 16:45:47, gab wrote: > ditto Done.
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: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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 https://codereview.chromium.org/2531883002/diff/60001/base/task_scheduler/tas... File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/2531883002/diff/60001/base/task_scheduler/tas... base/task_scheduler/task_tracker.cc:264: ThreadRestrictions::SetSingletonAllowed(previous_singleton_allowed); I presume this will change with the other CL that touches this?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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/2531883002/diff/60001/base/task_scheduler/tas... File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/2531883002/diff/60001/base/task_scheduler/tas... base/task_scheduler/task_tracker.cc:264: ThreadRestrictions::SetSingletonAllowed(previous_singleton_allowed); On 2016/11/28 22:17:41, robliao wrote: > I presume this will change with the other CL that touches this? Done. Reordered in this CL.
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 14:50:24, robliao wrote: > On 2016/11/25 18:50:48, fdoray wrote: > > PTAL > > > > This is important to prevent tasks without the WithFileIO() trait to > > AssertIOAllowed() in a single-pool TaskScheduler > > (CreateAndSetSimpleTaskScheduler, ScopedTaskScheduler). > > This seems to open a door where partitioned thread pools like what we had before > can now potentially execute IO tasks if the mapping function is not handled > correctly. Are we okay with this? What happened with this question?
https://codereview.chromium.org/2531883002/diff/80001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_pool_impl.cc (right): https://codereview.chromium.org/2531883002/diff/80001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl.cc:312: new SchedulerWorkerPoolImpl(params.name(), fwiw auto & makeunique would be nice here https://codereview.chromium.org/2531883002/diff/80001/base/task_scheduler/tas... File base/task_scheduler/task_tracker.cc (left): https://codereview.chromium.org/2531883002/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_tracker.cc:222: // All tasks run through here and the scheduler itself doesn't use This comment is no less true now is it? So IOAllowed should be able to follow the same pattern and just leave the thread in whatever state since the next task on the thread will come through here? Just making sure I understand, and then wondering why the change? https://codereview.chromium.org/2531883002/diff/80001/base/task_scheduler/tas... File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/2531883002/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_tracker.cc:227: ThreadRestrictions::SetIOAllowed(task->traits.with_file_io()); Is reading and writing thread-local storage before every task going to show up in profiling? Seems like since threads are owned by task scheduler it could do something cheaper.
On 2016/11/28 22:38:19, danakj wrote: > On 2016/11/28 14:50:24, robliao wrote: > > On 2016/11/25 18:50:48, fdoray wrote: > > > PTAL > > > > > > This is important to prevent tasks without the WithFileIO() trait to > > > AssertIOAllowed() in a single-pool TaskScheduler > > > (CreateAndSetSimpleTaskScheduler, ScopedTaskScheduler). > > > > This seems to open a door where partitioned thread pools like what we had > before > > can now potentially execute IO tasks if the mapping function is not handled > > correctly. Are we okay with this? > > What happened with this question? Offline discussion: It's up to the code that initializes TaskScheduler to decide how TaskTraits are mapped to different pools. TaskScheduler shouldn't enforce that WithFileIO() tasks run in a different pool than non-WithFileIO() tasks, nor that BACKGROUND tasks run in a different pool than USER_BLOCKING tasks. It must be possible to initialize a TaskScheduler that runs all tasks on a single thread (see ScopedTaskScheduler). TaskScheduler is only responsible for making sure that tasks are posted with proper traits (e.g. a task can't do file i/o without WithFileIO()).
On Mon, Nov 28, 2016 at 3:00 PM, <fdoray@chromium.org> wrote: > On 2016/11/28 22:38:19, danakj wrote: > > On 2016/11/28 14:50:24, robliao wrote: > > > On 2016/11/25 18:50:48, fdoray wrote: > > > > PTAL > > > > > > > > This is important to prevent tasks without the WithFileIO() trait to > > > > AssertIOAllowed() in a single-pool TaskScheduler > > > > (CreateAndSetSimpleTaskScheduler, ScopedTaskScheduler). > > > > > > This seems to open a door where partitioned thread pools like what we > had > > before > > > can now potentially execute IO tasks if the mapping function is not > handled > > > correctly. Are we okay with this? > > > > What happened with this question? > > Offline discussion: It's up to the code that initializes TaskScheduler to > decide > how TaskTraits are mapped to different pools. TaskScheduler shouldn't > enforce > that WithFileIO() tasks run in a different pool than non-WithFileIO() > tasks, nor > that BACKGROUND tasks run in a different pool than USER_BLOCKING tasks. It > must > be possible to initialize a TaskScheduler that runs all tasks on a single > thread > (see ScopedTaskScheduler). TaskScheduler is only responsible for making > sure > that tasks are posted with proper traits (e.g. a task can't do file i/o > without > WithFileIO()). > Makes sense, but also makes me wonder if now is a good time to write unit tests for VariationsParamsToBrowserSchedulerWorkerPoolParams(). -- 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.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
danakj@: PTAnL https://codereview.chromium.org/2531883002/diff/80001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_pool_impl.cc (right): https://codereview.chromium.org/2531883002/diff/80001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl.cc:312: new SchedulerWorkerPoolImpl(params.name(), On 2016/11/28 22:55:56, danakj wrote: > fwiw auto & makeunique would be nice here Done. WrapUnique instead of MakeUnique because the constructor is private. https://codereview.chromium.org/2531883002/diff/80001/base/task_scheduler/tas... File base/task_scheduler/task_tracker.cc (left): https://codereview.chromium.org/2531883002/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_tracker.cc:222: // All tasks run through here and the scheduler itself doesn't use On 2016/11/28 22:55:56, danakj wrote: > This comment is no less true now is it? So IOAllowed should be able to follow > the same pattern and just leave the thread in whatever state since the next task > on the thread will come through here? Just making sure I understand, and then > wondering why the change? I decided to reset ThreadRestrictions in RunTask() to avoid having singletons and/or IO disallowed on the main thread after running TaskTracker tests. We could also reset ThreadRestrictions from TaskSchedulerTaskTrackerTest::TearDown(). I don't like this solution because once we add wait restrictions: - We would need a ScopedWaitAllowed in SchedulerWorker::WaitForWork(). Might as well reset the state from RunTask(). - - TaskSchedulerTaskTrackerTest would need to be a friend of ThreadRestrictions to reset the wait allowed bit. https://codereview.chromium.org/2531883002/diff/80001/base/task_scheduler/tas... File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/2531883002/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_tracker.cc:227: ThreadRestrictions::SetIOAllowed(task->traits.with_file_io()); On 2016/11/28 22:55:56, danakj wrote: > Is reading and writing thread-local storage before every task going to show up > in profiling? Seems like since threads are owned by task scheduler it could do > something cheaper. I don't think we need to optimize this since ThreadRestrictions methods are no-ops in non-DCHECK builds. In release with DCHECK builds, SetSingletonAllowed() takes 10 ns on my Windows Z840. TimeTicks start = TimeTicks::Now(); constexpr size_t kIterations = 1000000000; for (size_t i = 0; i < kIterations; ++i) ThreadRestrictions::SetSingletonAllowed(i % 2 == 0); TimeTicks end = TimeTicks::Now(); LOG(ERROR) << "Elapsed: " << (end - start);
> Makes sense, but also makes me wonder if now is a good time to write unit > tests for VariationsParamsToBrowserSchedulerWorkerPoolParams(). > robliao@ is refactoring this code in a separate CL https://codereview.chromium.org/2539263003/
https://codereview.chromium.org/2531883002/diff/80001/base/task_scheduler/tas... File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/2531883002/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_tracker.cc:227: ThreadRestrictions::SetIOAllowed(task->traits.with_file_io()); On 2016/11/29 15:41:20, fdoray wrote: > On 2016/11/28 22:55:56, danakj wrote: > > Is reading and writing thread-local storage before every task going to show up > > in profiling? Seems like since threads are owned by task scheduler it could do > > something cheaper. > > I don't think we need to optimize this since ThreadRestrictions methods are > no-ops in non-DCHECK builds. The assert methods are no-ops. The setters are not. https://cs.chromium.org/chromium/src/base/threading/thread_restrictions.cc?rc... > In release with DCHECK builds, > SetSingletonAllowed() takes 10 ns on my Windows Z840. While that's quick per run, hundreds of tasks run per frame in chrome, perhaps? So thats getting into microseconds out of a 6ms frame budget with 144hz monitors. Is this wise? > TimeTicks start = TimeTicks::Now(); > constexpr size_t kIterations = 1000000000; > for (size_t i = 0; i < kIterations; ++i) > ThreadRestrictions::SetSingletonAllowed(i % 2 == 0); > TimeTicks end = TimeTicks::Now(); > LOG(ERROR) << "Elapsed: " << (end - start);
Ptanl https://codereview.chromium.org/2531883002/diff/80001/base/task_scheduler/tas... File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/2531883002/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_tracker.cc:227: ThreadRestrictions::SetIOAllowed(task->traits.with_file_io()); On 2016/12/01 21:59:04, danakj wrote: > On 2016/11/29 15:41:20, fdoray wrote: > > On 2016/11/28 22:55:56, danakj wrote: > > > Is reading and writing thread-local storage before every task going to show > up > > > in profiling? Seems like since threads are owned by task scheduler it could > do > > > something cheaper. > > > > I don't think we need to optimize this since ThreadRestrictions methods are > > no-ops in non-DCHECK builds. > > The assert methods are no-ops. The setters are not. > https://cs.chromium.org/chromium/src/base/threading/thread_restrictions.cc?rc... > > > In release with DCHECK builds, > > SetSingletonAllowed() takes 10 ns on my Windows Z840. > > While that's quick per run, hundreds of tasks run per frame in chrome, perhaps? > So thats getting into microseconds out of a 6ms frame budget with 144hz > monitors. Is this wise? > > > TimeTicks start = TimeTicks::Now(); > > constexpr size_t kIterations = 1000000000; > > for (size_t i = 0; i < kIterations; ++i) > > ThreadRestrictions::SetSingletonAllowed(i % 2 == 0); > > TimeTicks end = TimeTicks::Now(); > > LOG(ERROR) << "Elapsed: " << (end - start); > Yes they are. The whole implementation file is guarded by a DCHECK_IS_ON() https://cs.chromium.org/chromium/src/base/threading/thread_restrictions.cc?rc.... There is a no-op implementation when !DCHECK_IS_ON() https://cs.chromium.org/chromium/src/base/threading/thread_restrictions.h?sq=...
Ahhh ok thanks. LGTM
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, robliao@chromium.org Link to the patchset: https://codereview.chromium.org/2531883002/#ps100001 (title: "CR danakj #32 (WrapUnique)")
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
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...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
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, danakj@chromium.org, gab@chromium.org Link to the patchset: https://codereview.chromium.org/2531883002/#ps120001 (title: "rebase")
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": 1480685515413630,
"parent_rev": "939a5726cef1c11b7e471c4e1b06958c81c3a9b8", "commit_rev":
"39d32f1e3591a9dabf2d9ab42e6ca6ca12872f67"}
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== TaskScheduler: Set the IO allowed bit in TaskTracker::RunTask(). Before this CL, tasks without the WithFileIO() trait were allowed to perform synchronous file IO operations if they were mapped to a pool created with IORestriction::ALLOWED (this happens with a TaskScheduler created by CreateAndSetSimpleTaskScheduler(). With this CL, the IO allowed bit is always updated before running a task, based on the presence of the WithFileIO() trait. BUG=553459 ========== to ========== TaskScheduler: Set the IO allowed bit in TaskTracker::RunTask(). Before this CL, tasks without the WithFileIO() trait were allowed to perform synchronous file IO operations if they were mapped to a pool created with IORestriction::ALLOWED (this happens with a TaskScheduler created by CreateAndSetSimpleTaskScheduler(). With this CL, the IO allowed bit is always updated before running a task, based on the presence of the WithFileIO() trait. BUG=553459 Committed: https://crrev.com/f7d7e2f5926a928e472a06fcffb7c7a7dd154b32 Cr-Commit-Position: refs/heads/master@{#435942} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/f7d7e2f5926a928e472a06fcffb7c7a7dd154b32 Cr-Commit-Position: refs/heads/master@{#435942} |
