Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2902753003:
Implement Shared SingleThreadTaskRunners in the Task Scheduler (Closed)
DescriptionImplement Shared SingleThreadTaskRunners in the Task Scheduler
This change provides one shared SingleThreadTaskRunner per Trait+COM
combination by reusing dedicated SingleThreadTaskRunners.
In production, these SingleThreadTaskRunners will never be reclaimed.
In tests, JoinForTesting() cleans these up.
BUG=720058
Review-Url: https://codereview.chromium.org/2902753003
Cr-Commit-Position: refs/heads/master@{#474813}
Committed: https://chromium.googlesource.com/chromium/src/+/e96185d01c286ffb9892e3f7dff6765bea1dc81b
Patch Set 1 #
Total comments: 16
Patch Set 2 : CR Feedback + Shared SchedulerWorker #Patch Set 3 : Added A Thread Name Unit Test #Patch Set 4 : Change Dependent Change #
Total comments: 41
Depends on Patchset: Messages
Total messages: 26 (18 generated)
The CQ bit was checked by robliao@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...
robliao@chromium.org changed reviewers: + fdoray@chromium.org, gab@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Read this comment first: https://codereview.chromium.org/2902753003/diff/1/base/task_scheduler/schedul... https://codereview.chromium.org/2902753003/diff/1/base/task_scheduler/schedul... File base/task_scheduler/scheduler_single_thread_task_runner_manager.cc (right): https://codereview.chromium.org/2902753003/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:286: // SchedulerSingleThreadTaskRunner: Update comment. This isn't an override. https://codereview.chromium.org/2902753003/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:390: ThreadPriority priority_hint, Now that this file includes "base/task_scheduler/environment_config.h", it could get the name and priority hint itself instead of receiving them from the caller. https://codereview.chromium.org/2902753003/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:416: SingleThreadTaskRunnerThreadMode thread_mode) { I would assert that WithBaseSyncPrimitives() is not used for SHARED SingleThreadTaskRunners. It could lead to deadlocks (waiting for an event to be signaled by a task that must run on the same shared thread) or jank. https://codereview.chromium.org/2902753003/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:417: // To simply the code, |dedicated_task_runner| is a local only variable that To *simplify* https://codereview.chromium.org/2902753003/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:436: processed_name, priority_hint, traits); Unfortunately, if someone creates a SingleThreadTaskRunner which maps to the same task environment as an existing SingleThreadTaskRunner but which has different traits, these new traits won't be honored. E.g. tr1 = CreateSingleThreadTaskRunnerWithTraits({base::TaskShutdownBehavior::BLOCK_SHUTDOWN}); tr2 = CreateSingleThreadTaskRunnerWithTraits({base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN}); EXPECT_NE(tr1, tr2); // fails You can reuse SchedulerWorkers, but not SingleThreadTaskRunners. https://codereview.chromium.org/2902753003/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:443: task_runner->Start(); What do you think of rewriting this method as: template <typename DelegateType> scoped_refptr< SchedulerSingleThreadTaskRunnerManager::SchedulerSingleThreadTaskRunner> SchedulerSingleThreadTaskRunnerManager::CreateTaskRunnerWithTraitsImpl( const std::string& name, ThreadPriority priority_hint, const TaskTraits& traits, SingleThreadTaskRunnerThreadMode thread_mode) { // To simply the code, |dedicated_task_runner| is a local only variable that // allows the code to treat both the DEDICATED and SHARED cases similarly. In // DEDICATED, the scoped_refptr is backed by a local variable and in SHARED, // the scoped_refptr is backed by a member variable. scoped_refptr<SchedulerSingleThreadTaskRunner> dedicated_task_runner; scoped_refptr<SchedulerSingleThreadTaskRunner>& task_runner = thread_mode == SingleThreadTaskRunnerThreadMode::DEDICATED ? dedicated_task_runner : GetSharedTaskRunnerForTraits<DelegateType>(traits); -- begin changes -- scoped_refptr<SchedulerWorker> worker; bool started; { AutoSchedulerLock auto_lock(lock_); if (!task_runner) { std::string processed_name = thread_mode == SingleThreadTaskRunnerThreadMode::DEDICATED ? name : "Shared" + name; worker = CreateAndRegisterSchedulerWorker<DelegateType>( name, priority_hint); task_runner = new SchedulerSingleThreadTaskRunner(this, traits, worker); } started = started_; } if (worker && started) worker->Start(); -- end changes -- return task_runner; } This allows you to delete SchedulerSingleThreadTaskRunnerManager::CreateSingleThreadTaskRunnerWithDelegate() and SchedulerSingleThreadTaskRunner::Start() and leads to less lines of code overall. https://codereview.chromium.org/2902753003/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:449: ReleaseSharedTaskRunners(); Why is this needed? SchedulerWorker::JoinForTesting() should work even if shared task runners aren't released. Not releasing shared task runners would allows tasks running during JoinForTesting() to get shared task runners that already exist without hitting the DCHECK below, which isn't a bad thing. https://codereview.chromium.org/2902753003/diff/1/base/task_scheduler/schedul... File base/task_scheduler/scheduler_single_thread_task_runner_manager.h (right): https://codereview.chromium.org/2902753003/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.h:44: // SingleThreadTaskRunners for each COM+Trait combination. These SingleThreadTaskRunnerThreadMode::SHARED (make it clear that this doesn't apply to DEDICATED) SingleThreadTaskRunners with the same *task environment* (not traits) and COM state run their tasks on the same thread. These shared threads (the threads are joined, not the SingleThreadTaskRunners) are only reclaimed during JoinForTesting(), even if all external references to the SingleThreadTaskRunners are released before that.
Patchset #2 (id:20001) has been deleted
Thanks for the review! https://codereview.chromium.org/2902753003/diff/1/base/task_scheduler/schedul... File base/task_scheduler/scheduler_single_thread_task_runner_manager.cc (right): https://codereview.chromium.org/2902753003/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:286: // SchedulerSingleThreadTaskRunner: On 2017/05/24 13:25:45, fdoray wrote: > Update comment. This isn't an override. The headers are not strictly used for overrides, but rather to indicate sections for different classes. Without this header, you can't tell when SingleThreadTaskRunner ends. https://codereview.chromium.org/2902753003/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:390: ThreadPriority priority_hint, On 2017/05/24 13:25:45, fdoray wrote: > Now that this file includes "base/task_scheduler/environment_config.h", it could > get the name and priority hint itself instead of receiving them from the caller. Done. https://codereview.chromium.org/2902753003/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:416: SingleThreadTaskRunnerThreadMode thread_mode) { On 2017/05/24 13:25:45, fdoray wrote: > I would assert that WithBaseSyncPrimitives() is not used for SHARED > SingleThreadTaskRunners. It could lead to deadlocks (waiting for an event to be > signaled by a task that must run on the same shared thread) or jank. Sounds reasonable to me. Done. https://codereview.chromium.org/2902753003/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:417: // To simply the code, |dedicated_task_runner| is a local only variable that On 2017/05/24 13:25:45, fdoray wrote: > To *simplify* Done. https://codereview.chromium.org/2902753003/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:436: processed_name, priority_hint, traits); On 2017/05/24 13:25:45, fdoray wrote: > Unfortunately, if someone creates a SingleThreadTaskRunner which maps to the > same task environment as an existing SingleThreadTaskRunner but which has > different traits, these new traits won't be honored. > > E.g. > > tr1 = > CreateSingleThreadTaskRunnerWithTraits({base::TaskShutdownBehavior::BLOCK_SHUTDOWN}); > tr2 = > CreateSingleThreadTaskRunnerWithTraits({base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN}); > EXPECT_NE(tr1, tr2); // fails > > You can reuse SchedulerWorkers, but not SingleThreadTaskRunners. Nice catch. Fixed to reuse SchedulerWorkers. https://codereview.chromium.org/2902753003/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:443: task_runner->Start(); On 2017/05/24 13:25:45, fdoray wrote: > What do you think of rewriting this method as: > > template <typename DelegateType> > scoped_refptr< > SchedulerSingleThreadTaskRunnerManager::SchedulerSingleThreadTaskRunner> > SchedulerSingleThreadTaskRunnerManager::CreateTaskRunnerWithTraitsImpl( > const std::string& name, > ThreadPriority priority_hint, > const TaskTraits& traits, > SingleThreadTaskRunnerThreadMode thread_mode) { > // To simply the code, |dedicated_task_runner| is a local only variable that > // allows the code to treat both the DEDICATED and SHARED cases similarly. In > // DEDICATED, the scoped_refptr is backed by a local variable and in SHARED, > // the scoped_refptr is backed by a member variable. > scoped_refptr<SchedulerSingleThreadTaskRunner> dedicated_task_runner; > scoped_refptr<SchedulerSingleThreadTaskRunner>& task_runner = > thread_mode == SingleThreadTaskRunnerThreadMode::DEDICATED > ? dedicated_task_runner > : GetSharedTaskRunnerForTraits<DelegateType>(traits); > > -- begin changes -- > > scoped_refptr<SchedulerWorker> worker; > bool started; > { > AutoSchedulerLock auto_lock(lock_); > if (!task_runner) { > std::string processed_name = > thread_mode == SingleThreadTaskRunnerThreadMode::DEDICATED > ? name > : "Shared" + name; > worker = CreateAndRegisterSchedulerWorker<DelegateType>( > name, priority_hint); > task_runner = new SchedulerSingleThreadTaskRunner(this, traits, worker); > } > started = started_; > } > if (worker && started) > worker->Start(); > > -- end changes -- > > return task_runner; > } > > This allows you to delete > SchedulerSingleThreadTaskRunnerManager::CreateSingleThreadTaskRunnerWithDelegate() > and SchedulerSingleThreadTaskRunner::Start() and leads to less lines of code > overall. sgtm. Done. https://codereview.chromium.org/2902753003/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:449: ReleaseSharedTaskRunners(); On 2017/05/24 13:25:45, fdoray wrote: > Why is this needed? SchedulerWorker::JoinForTesting() should work even if shared > task runners aren't released. Not releasing shared task runners would allows > tasks running during JoinForTesting() to get shared task runners that already > exist without hitting the DCHECK below, which isn't a bad thing. This is now ReleaseSharedSchedulerWorkers() and is needed because the expectation is that all STTRs are gone by the time ~SchedulerSingleThreadTaskRunnerManager returns. Without this, the workers remain registered (even though they are joined) and we'll assert at destruction. https://codereview.chromium.org/2902753003/diff/1/base/task_scheduler/schedul... File base/task_scheduler/scheduler_single_thread_task_runner_manager.h (right): https://codereview.chromium.org/2902753003/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.h:44: // SingleThreadTaskRunners for each COM+Trait combination. These On 2017/05/24 13:25:45, fdoray wrote: > SingleThreadTaskRunnerThreadMode::SHARED (make it clear that this doesn't apply > to DEDICATED) SingleThreadTaskRunners with the same *task environment* (not > traits) and COM state run their tasks on the same thread. These shared threads > (the threads are joined, not the SingleThreadTaskRunners) are only reclaimed > during JoinForTesting(), even if all external references to the > SingleThreadTaskRunners are released before that. Went with // SingleThreadTaskRunners using SingleThreadTaskRunnerThreadMode::SHARED, are // backed by shared SchedulerWorkers for each COM+task environment combination. // These workers are only reclaimed during JoinForTesting().
The CQ bit was checked by robliao@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: 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_...)
Patchset #4 (id:80001) has been deleted
The CQ bit was checked by robliao@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: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
lgtm https://codereview.chromium.org/2902753003/diff/100001/base/task_scheduler/en... File base/task_scheduler/environment_config.h (right): https://codereview.chromium.org/2902753003/diff/100001/base/task_scheduler/en... base/task_scheduler/environment_config.h:40: size_t BASE_EXPORT GetEnvironmentIndexForTraits(const TaskTraits& traits); #include "base/base_export.h" https://codereview.chromium.org/2902753003/diff/100001/base/task_scheduler/sc... File base/task_scheduler/scheduler_single_thread_task_runner_manager.cc (right): https://codereview.chromium.org/2902753003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:344: arraysize(shared_scheduler_workers_) == ENVIRONMENT_COUNT, The static_assert is redundant if this is defined as SchedulerWorker* shared_scheduler_workers_[ENVIRONMENT_COUNT]; in the header file. https://codereview.chromium.org/2902753003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:463: return new SchedulerSingleThreadTaskRunner(this, traits, worker, thread_mode); MakeRefCounted<SchedulerSingleThreadTaskRunner> https://codereview.chromium.org/2902753003/diff/100001/base/task_scheduler/sc... File base/task_scheduler/scheduler_single_thread_task_runner_manager.h (right): https://codereview.chromium.org/2902753003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_single_thread_task_runner_manager.h:43: // SingleThreadTaskRunners using SingleThreadTaskRunnerThreadMode::SHARED, are no comma https://codereview.chromium.org/2902753003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_single_thread_task_runner_manager.h:125: SchedulerWorker* shared_scheduler_workers_[4]; s/4/ENVIRONMENT_COUNT/ https://codereview.chromium.org/2902753003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_single_thread_task_runner_manager.h:128: SchedulerWorker* shared_com_scheduler_workers_[4]; s/4/ENVIRONMENT_COUNT/
lgtm w/ comments Will CQ now to land this before branch see Rob is OOO. @robliao: please address comments in a follow-up CL next week. Thanks! https://codereview.chromium.org/2902753003/diff/100001/base/task_scheduler/sc... File base/task_scheduler/scheduler_single_thread_task_runner_manager.cc (right): https://codereview.chromium.org/2902753003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:338: shared_com_scheduler_workers_ {} Use member initialization for these two as mentioned in .h, also avoids ifdefs in initializer list :) https://codereview.chromium.org/2902753003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:344: arraysize(shared_scheduler_workers_) == ENVIRONMENT_COUNT, On 2017/05/25 17:02:59, fdoray wrote: > The static_assert is redundant if this is defined as > SchedulerWorker* shared_scheduler_workers_[ENVIRONMENT_COUNT]; > in the header file. Agreed and with the initialization moving to member initialization these asserts in the constructor would feel slightly out of place. I think it's safe to drop them. https://codereview.chromium.org/2902753003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:430: "may cause deadlocks. Either reevaluate your usage pattern or use " "... your usage patterns (e.g. use SequencedTaskRunner) or ..." https://codereview.chromium.org/2902753003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:573: local_shared_com_scheduler_workers[i] = shared_com_scheduler_workers_[i]; Why do we need a local list? Doesn't seem like anything touches the shared pointers during UnregisterSchedulerWorker? Also, shall we set the shared pointers to nullptr when done for good measure? https://codereview.chromium.org/2902753003/diff/100001/base/task_scheduler/sc... File base/task_scheduler/scheduler_single_thread_task_runner_manager.h (right): https://codereview.chromium.org/2902753003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_single_thread_task_runner_manager.h:45: // These workers are only reclaimed during JoinForTesting(). "These workers are lazily instantiated and then only reclaimed during JoinForTesting()" https://codereview.chromium.org/2902753003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_single_thread_task_runner_manager.h:62: // named "TaskSchedulerSingleThread[Shared]" + |name| + I'd say TaskScheduler[Shared]SingleThread so that sharing is more obvious in the groupings (especially when the names are truncated) https://codereview.chromium.org/2902753003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_single_thread_task_runner_manager.h:66: // runner, otherwise it could be shared with other SingleThreadTaskRunners. I think the rest of this comment is redundant starting from \"Shared\" on 64. An enum is self-documenting, don't think we need to redefine the enum in different words everywhere it's used (e.g. say we added a category, having to update all comments would result in useless churn -- and even more likely in outdated comments). https://codereview.chromium.org/2902753003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_single_thread_task_runner_manager.h:78: // runner, otherwise it could be shared with other SingleThreadTaskRunners. ditto from \"Shared\" https://codereview.chromium.org/2902753003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_single_thread_task_runner_manager.h:107: SchedulerWorker*& GetSharedSchedulerWorkerForTraits(const TaskTraits& traits); ** is more common and less surprising than *& IMO, especially when non-const (or maybe it should be const? didn't check usage yet) https://codereview.chromium.org/2902753003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_single_thread_task_runner_manager.h:125: SchedulerWorker* shared_scheduler_workers_[4]; = {}; to zero-initialize inline https://codereview.chromium.org/2902753003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_single_thread_task_runner_manager.h:128: SchedulerWorker* shared_com_scheduler_workers_[4]; ditto https://codereview.chromium.org/2902753003/diff/100001/base/task_scheduler/sc... File base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc (left): https://codereview.chromium.org/2902753003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc:195: RunsTasksOnCurrentThread) { Should stay here as a Common test, no? https://codereview.chromium.org/2902753003/diff/100001/base/task_scheduler/sc... File base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc (right): https://codereview.chromium.org/2902753003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc:149: RunsTasksOnCurrentThread) { This test should work in both modes, parametrize as a TaskSchedulerSingleThreadTaskRunnerManagerCommonTest as well? https://codereview.chromium.org/2902753003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc:463: TEST_F(TaskSchedulerSingleThreadTaskRunnerManagerTest, COMSTASameThreadUsed) { Seems like this is testing an implementation detail. I don't think we want to test this for SHARED (we do for DEDICATED). https://codereview.chromium.org/2902753003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc:536: SingleThreadTaskRunnerThreadMode::DEDICATED); Why isn't this also Common? https://codereview.chromium.org/2902753003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc:582: PostTaskBeforeStart) { Why isn't this also Common?
The CQ bit was checked by gab@chromium.org
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": 1495738026020920,
"parent_rev": "0a2e1e6a2e2b18ad724be638b25b14ebf73063e3", "commit_rev":
"e96185d01c286ffb9892e3f7dff6765bea1dc81b"}
Message was sent while issue was closed.
Description was changed from ========== Implement Shared SingleThreadTaskRunners in the Task Scheduler This change provides one shared SingleThreadTaskRunner per Trait+COM combination by reusing dedicated SingleThreadTaskRunners. In production, these SingleThreadTaskRunners will never be reclaimed. In tests, JoinForTesting() cleans these up. BUG=720058 ========== to ========== Implement Shared SingleThreadTaskRunners in the Task Scheduler This change provides one shared SingleThreadTaskRunner per Trait+COM combination by reusing dedicated SingleThreadTaskRunners. In production, these SingleThreadTaskRunners will never be reclaimed. In tests, JoinForTesting() cleans these up. BUG=720058 Review-Url: https://codereview.chromium.org/2902753003 Cr-Commit-Position: refs/heads/master@{#474813} Committed: https://chromium.googlesource.com/chromium/src/+/e96185d01c286ffb9892e3f7dff6... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001) as https://chromium.googlesource.com/chromium/src/+/e96185d01c286ffb9892e3f7dff6...
Message was sent while issue was closed.
Thanks for the CQ. Please make all responses at https://codereview.chromium.org/2917433002/ to make following threads manageable. https://codereview.chromium.org/2902753003/diff/100001/base/task_scheduler/en... File base/task_scheduler/environment_config.h (right): https://codereview.chromium.org/2902753003/diff/100001/base/task_scheduler/en... base/task_scheduler/environment_config.h:40: size_t BASE_EXPORT GetEnvironmentIndexForTraits(const TaskTraits& traits); On 2017/05/25 17:02:59, fdoray wrote: > #include "base/base_export.h" Done. https://codereview.chromium.org/2902753003/diff/100001/base/task_scheduler/sc... File base/task_scheduler/scheduler_single_thread_task_runner_manager.cc (right): https://codereview.chromium.org/2902753003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:338: shared_com_scheduler_workers_ {} On 2017/05/25 18:47:00, gab wrote: > Use member initialization for these two as mentioned in .h, also avoids ifdefs > in initializer list :) Done. https://codereview.chromium.org/2902753003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:344: arraysize(shared_scheduler_workers_) == ENVIRONMENT_COUNT, On 2017/05/25 18:47:00, gab wrote: > On 2017/05/25 17:02:59, fdoray wrote: > > The static_assert is redundant if this is defined as > > SchedulerWorker* shared_scheduler_workers_[ENVIRONMENT_COUNT]; > > in the header file. > > Agreed and with the initialization moving to member initialization these asserts > in the constructor would feel slightly out of place. I think it's safe to drop > them. Done. Keeping the one below though since that certainly needs to remain true and isn't trivially true by construction. https://codereview.chromium.org/2902753003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:430: "may cause deadlocks. Either reevaluate your usage pattern or use " On 2017/05/25 18:47:00, gab wrote: > "... your usage patterns (e.g. use SequencedTaskRunner) or ..." added the e.g. The singular usage is appropriate here, however. https://codereview.chromium.org/2902753003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:463: return new SchedulerSingleThreadTaskRunner(this, traits, worker, thread_mode); On 2017/05/25 17:02:59, fdoray wrote: > MakeRefCounted<SchedulerSingleThreadTaskRunner> Done. https://codereview.chromium.org/2902753003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:573: local_shared_com_scheduler_workers[i] = shared_com_scheduler_workers_[i]; On 2017/05/25 18:47:00, gab wrote: > Why do we need a local list? Doesn't seem like anything touches the shared > pointers during UnregisterSchedulerWorker? > > Also, shall we set the shared pointers to nullptr when done for good measure? The local list is needed to avoid unregistration within the lock. Setting nullptr seemed unnecessary since we make no guarantees after JoinForTesting. I'll go ahead and set them here to root a discussion. https://codereview.chromium.org/2902753003/diff/100001/base/task_scheduler/sc... File base/task_scheduler/scheduler_single_thread_task_runner_manager.h (right): https://codereview.chromium.org/2902753003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_single_thread_task_runner_manager.h:43: // SingleThreadTaskRunners using SingleThreadTaskRunnerThreadMode::SHARED, are On 2017/05/25 17:02:59, fdoray wrote: > no comma Removed. https://codereview.chromium.org/2902753003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_single_thread_task_runner_manager.h:45: // These workers are only reclaimed during JoinForTesting(). On 2017/05/25 18:47:00, gab wrote: > "These workers are lazily instantiated and then only reclaimed during > JoinForTesting()" Done. https://codereview.chromium.org/2902753003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_single_thread_task_runner_manager.h:62: // named "TaskSchedulerSingleThread[Shared]" + |name| + On 2017/05/25 18:47:00, gab wrote: > I'd say TaskScheduler[Shared]SingleThread so that sharing is more obvious in the > groupings (especially when the names are truncated) This is an artifact of preserving TaskSchedulerSingleThread together. Piping shared to the middle would complicate the worker creation path by forcing us to move the prefix outside of the worker creation. Let's see how this goes first. https://codereview.chromium.org/2902753003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_single_thread_task_runner_manager.h:66: // runner, otherwise it could be shared with other SingleThreadTaskRunners. On 2017/05/25 18:47:00, gab wrote: > I think the rest of this comment is redundant starting from \"Shared\" on 64. > > An enum is self-documenting, don't think we need to redefine the enum in > different words everywhere it's used (e.g. say we added a category, having to > update all comments would result in useless churn -- and even more likely in > outdated comments). I certainly agree with this sentiment. I would prefer to omit it, but then we wouldn't have a |thread_mode| comment other than point it to the enum. I'm going to remove it in the update CL so we have a root for discussion instead of branching the thread here. https://codereview.chromium.org/2902753003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_single_thread_task_runner_manager.h:78: // runner, otherwise it could be shared with other SingleThreadTaskRunners. On 2017/05/25 18:47:00, gab wrote: > ditto from \"Shared\" Agreed. See above. https://codereview.chromium.org/2902753003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_single_thread_task_runner_manager.h:107: SchedulerWorker*& GetSharedSchedulerWorkerForTraits(const TaskTraits& traits); On 2017/05/25 18:47:00, gab wrote: > ** is more common and less surprising than *& IMO, especially when non-const (or > maybe it should be const? didn't check usage yet) I wanted to enforce the non-nullable property for SchedulerWorker*& worker = thread_mode == SingleThreadTaskRunnerThreadMode::DEDICATED ? dedicated_worker : GetSharedSchedulerWorkerForTraits<DelegateType>(traits); This was the main way to make it work with a template. https://codereview.chromium.org/2902753003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_single_thread_task_runner_manager.h:125: SchedulerWorker* shared_scheduler_workers_[4]; On 2017/05/25 18:47:00, gab wrote: > = {}; > > to zero-initialize inline Done. https://codereview.chromium.org/2902753003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_single_thread_task_runner_manager.h:128: SchedulerWorker* shared_com_scheduler_workers_[4]; On 2017/05/25 18:47:00, gab wrote: > ditto Done. https://codereview.chromium.org/2902753003/diff/100001/base/task_scheduler/sc... File base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc (left): https://codereview.chromium.org/2902753003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc:195: RunsTasksOnCurrentThread) { On 2017/05/25 18:47:01, gab wrote: > Should stay here as a Common test, no? See above. https://codereview.chromium.org/2902753003/diff/100001/base/task_scheduler/sc... File base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc (right): https://codereview.chromium.org/2902753003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc:149: RunsTasksOnCurrentThread) { On 2017/05/25 18:47:00, gab wrote: > This test should work in both modes, parametrize as a > TaskSchedulerSingleThreadTaskRunnerManagerCommonTest as well? Since we do check the thread handle for the COM case, it's not guaranteed that this test will succeed when the mode is SHARED. In fact, the EXPECT_FALSEs below become EXPECT_TRUEs. https://codereview.chromium.org/2902753003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc:463: TEST_F(TaskSchedulerSingleThreadTaskRunnerManagerTest, COMSTASameThreadUsed) { On 2017/05/25 18:47:01, gab wrote: > Seems like this is testing an implementation detail. I don't think we want to > test this for SHARED (we do for DEDICATED). This is currently the only reasonable way to verify reuse. If/when the implementation changes, then we can update this test. https://codereview.chromium.org/2902753003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc:536: SingleThreadTaskRunnerThreadMode::DEDICATED); On 2017/05/25 18:47:01, gab wrote: > Why isn't this also Common? This uses TaskSchedulerSingleThreadTaskRunnerTestWin. Creating a common for this seemed overkill. https://codereview.chromium.org/2902753003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc:582: PostTaskBeforeStart) { On 2017/05/25 18:47:01, gab wrote: > Why isn't this also Common? See above. This is a different class similar to the COM case. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
