Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2834063002:
Separate the create and start phases in TaskSchedulerImpl. (Closed)
DescriptionSeparate the create and start phases in TaskSchedulerImpl.
The task scheduler doesn't create threads before Start() is called.
Tasks can be posted before Start(), but they don't run right away.
Tasks can only run once Start() has been called.
BUG=690706
Review-Url: https://codereview.chromium.org/2834063002
Cr-Commit-Position: refs/heads/master@{#467117}
Committed: https://chromium.googlesource.com/chromium/src/+/0f358eedd390d9c0017ec60ff95c51cfa07d60a5
Patch Set 1 #Patch Set 2 : self-review #Patch Set 3 : rebase #Patch Set 4 : unique_ptr #Patch Set 5 : self-review #
Total comments: 27
Patch Set 6 : CR-robliao-gab-17-20 #Patch Set 7 : CR-robliao-25-grammar #
Messages
Total messages: 38 (27 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_chromium_tsan_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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
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...
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
fdoray@chromium.org changed reviewers: + gab@chromium.org, robliao@chromium.org
PTAL
Overall approach lg. https://codereview.chromium.org/2834063002/diff/80001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_pool_impl.h (right): https://codereview.chromium.org/2834063002/diff/80001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl.h:43: // The pool doesn't create threads before Start() is called. Tasks can be posted Nit: before -> until Apply to the other comments too. https://codereview.chromium.org/2834063002/diff/80001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl.h:44: // before Start() is called, but they don't run until Start() is called. Tasks can be posted at any time but will not run until after Start() is called. Apply to the other comments too. https://codereview.chromium.org/2834063002/diff/80001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler.h (right): https://codereview.chromium.org/2834063002/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_scheduler.h:42: // The non-static methods of this class are thread-safe. Nit: non-static -> instance https://codereview.chromium.org/2834063002/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_scheduler.h:44: // Note: all base/task_scheduler users should go through post_task.h instead of Nit: all -> All https://codereview.chromium.org/2834063002/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_scheduler.h:46: // process' instance. Nit: process's https://codereview.chromium.org/2834063002/diff/80001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler_impl.h (right): https://codereview.chromium.org/2834063002/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl.h:41: explicit TaskSchedulerImpl(StringPiece name); Readd the comment for |name| for public consistency. https://codereview.chromium.org/2834063002/diff/80001/base/test/scoped_task_s... File base/test/scoped_task_scheduler.cc (right): https://codereview.chromium.org/2834063002/diff/80001/base/test/scoped_task_s... base/test/scoped_task_scheduler.cc:169: // ScopedTaskScheduler intentionally breaks the TaskScheduler contract of not This comment should probably go at the top of TestTaskScheduler's declaration.
Nice :) https://codereview.chromium.org/2834063002/diff/80001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler.h (right): https://codereview.chromium.org/2834063002/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_scheduler.h:44: // Note: all base/task_scheduler users should go through post_task.h instead of On 2017/04/24 22:32:19, robliao wrote: > Nit: all -> All I don't think this is required (and IMO capital letter here looks weird): http://data.grammarbook.com/blog/commas/capitalization-with-colons/ "Capitalization rule with sentences after colons: If only one sentence follows the colon, it is often not necessary to capitalize the first word of the new sentence. If two or more sentences follow the colon, capitalize the first word of each sentence following." https://codereview.chromium.org/2834063002/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_scheduler.h:46: // process' instance. On 2017/04/24 22:32:19, robliao wrote: > Nit: process's Don't think this is required either (I sure always omit after 's' and have committed many comments as such): http://www.grammarbook.com/punctuation/apostro.asp https://codereview.chromium.org/2834063002/diff/80001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler_impl.cc (right): https://codereview.chromium.org/2834063002/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl.cc:73: Bind(&TaskSchedulerImpl::ReEnqueueSequenceCallback, Unretained(this)), Note for https://codereview.chromium.org/2807063007/ debate: here's an unnecessary instance of ReEnqueueSequenceCallback being carried through a refactor. As previously, it should be bound outside the loop to avoid churn. https://codereview.chromium.org/2834063002/diff/80001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler_impl.h (right): https://codereview.chromium.org/2834063002/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl.h:41: explicit TaskSchedulerImpl(StringPiece name); On 2017/04/24 22:32:19, robliao wrote: > Readd the comment for |name| for public consistency. Also mention explicitly that it now must be started (so that users of //base from outside of chrome using this to SetInstance() know when rolling passed this change that there's an extra step to fix). https://codereview.chromium.org/2834063002/diff/80001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/2834063002/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl_unittest.cc:285: // This should not hang if the task is scheduled after Start(). implicit IMO, remove comment (same below) https://codereview.chromium.org/2834063002/diff/80001/base/test/scoped_task_s... File base/test/scoped_task_scheduler.cc (right): https://codereview.chromium.org/2834063002/diff/80001/base/test/scoped_task_s... base/test/scoped_task_scheduler.cc:169: // ScopedTaskScheduler intentionally breaks the TaskScheduler contract of not On 2017/04/24 22:32:19, robliao wrote: > This comment should probably go at the top of TestTaskScheduler's declaration. Agreed, also this is another argument for getting rid of synchronous test task scheduler.
https://codereview.chromium.org/2834063002/diff/80001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler.h (right): https://codereview.chromium.org/2834063002/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_scheduler.h:159: // (ensures isolation). Actually, this is the API that needs to be updated right? How will we enforce that existing callers properly start? Will we instead introduce another call that implies you explicitly want a non-started version (still started by default to avoid affecting existing callers)? PS: I think this voids some of my previous comments, I somehow thought some callers were doing SetInstance with a TaskSchedulerImpl.
https://codereview.chromium.org/2834063002/diff/80001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler.h (right): https://codereview.chromium.org/2834063002/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_scheduler.h:159: // (ensures isolation). On 2017/04/25 15:20:01, gab wrote: > Actually, this is the API that needs to be updated right? How will we enforce > that existing callers properly start? Will we instead introduce another call > that implies you explicitly want a non-started version (still started by default > to avoid affecting existing callers)? > > PS: I think this voids some of my previous comments, I somehow thought some > callers were doing SetInstance with a TaskSchedulerImpl. Nvm, found answer @ https://codereview.chromium.org/2836033002 :) This also voids my comment on task_scheduler_impl
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2834063002/diff/80001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler.h (right): https://codereview.chromium.org/2834063002/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_scheduler.h:44: // Note: all base/task_scheduler users should go through post_task.h instead of On 2017/04/25 15:16:15, gab wrote: > On 2017/04/24 22:32:19, robliao wrote: > > Nit: all -> All > > I don't think this is required (and IMO capital letter here looks weird): > http://data.grammarbook.com/blog/commas/capitalization-with-colons/ > > "Capitalization rule with sentences after colons: If only one sentence follows > the colon, it is often not necessary to capitalize the first word of the new > sentence. If two or more sentences follow the colon, capitalize the first word > of each sentence following." http://apvschicago.com/2011/04/capitalization-after-colons.html Since the structure here is Note: [Complete Sentence and more], the sentence should be capitalized. https://codereview.chromium.org/2834063002/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_scheduler.h:46: // process' instance. On 2017/04/25 15:16:15, gab wrote: > On 2017/04/24 22:32:19, robliao wrote: > > Nit: process's > > Don't think this is required either (I sure always omit after 's' and have > committed many comments as such): > http://www.grammarbook.com/punctuation/apostro.asp In this case, because process is singular, process's is the way to go. See https://english.stackexchange.com/questions/1073/what-is-the-correct-possessi...
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
PTAnL https://codereview.chromium.org/2834063002/diff/80001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_pool_impl.h (right): https://codereview.chromium.org/2834063002/diff/80001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl.h:43: // The pool doesn't create threads before Start() is called. Tasks can be posted On 2017/04/24 22:32:19, robliao wrote: > Nit: before -> until > > Apply to the other comments too. Done. https://codereview.chromium.org/2834063002/diff/80001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl.h:44: // before Start() is called, but they don't run until Start() is called. On 2017/04/24 22:32:18, robliao wrote: > Tasks can be posted at any time but will not run until after Start() is called. > > Apply to the other comments too. Done. https://codereview.chromium.org/2834063002/diff/80001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler.h (right): https://codereview.chromium.org/2834063002/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_scheduler.h:42: // The non-static methods of this class are thread-safe. On 2017/04/24 22:32:19, robliao wrote: > Nit: non-static -> instance Done. https://codereview.chromium.org/2834063002/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_scheduler.h:44: // Note: all base/task_scheduler users should go through post_task.h instead of On 2017/04/25 17:37:28, robliao wrote: > On 2017/04/25 15:16:15, gab wrote: > > On 2017/04/24 22:32:19, robliao wrote: > > > Nit: all -> All > > > > I don't think this is required (and IMO capital letter here looks weird): > > http://data.grammarbook.com/blog/commas/capitalization-with-colons/ > > > > "Capitalization rule with sentences after colons: If only one sentence follows > > the colon, it is often not necessary to capitalize the first word of the new > > sentence. If two or more sentences follow the colon, capitalize the first word > > of each sentence following." > > http://apvschicago.com/2011/04/capitalization-after-colons.html > > Since the structure here is Note: [Complete Sentence and more], the sentence > should be capitalized. Done. https://codereview.chromium.org/2834063002/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_scheduler.h:46: // process' instance. On 2017/04/25 17:37:27, robliao wrote: > On 2017/04/25 15:16:15, gab wrote: > > On 2017/04/24 22:32:19, robliao wrote: > > > Nit: process's > > > > Don't think this is required either (I sure always omit after 's' and have > > committed many comments as such): > > http://www.grammarbook.com/punctuation/apostro.asp > > In this case, because process is singular, process's is the way to go. > See > https://english.stackexchange.com/questions/1073/what-is-the-correct-possessi... Done. https://codereview.chromium.org/2834063002/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_scheduler.h:159: // (ensures isolation). On 2017/04/25 15:21:11, gab wrote: > On 2017/04/25 15:20:01, gab wrote: > > Actually, this is the API that needs to be updated right? How will we enforce > > that existing callers properly start? Will we instead introduce another call > > that implies you explicitly want a non-started version (still started by > default > > to avoid affecting existing callers)? > > > > PS: I think this voids some of my previous comments, I somehow thought some > > callers were doing SetInstance with a TaskSchedulerImpl. > > Nvm, found answer @ https://codereview.chromium.org/2836033002 :) > > This also voids my comment on task_scheduler_impl Yes. https://codereview.chromium.org/2834063002/diff/80001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler_impl.cc (right): https://codereview.chromium.org/2834063002/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl.cc:73: Bind(&TaskSchedulerImpl::ReEnqueueSequenceCallback, Unretained(this)), On 2017/04/25 15:16:15, gab wrote: > Note for https://codereview.chromium.org/2807063007/ debate: here's an > unnecessary instance of ReEnqueueSequenceCallback being carried through a > refactor. > > As previously, it should be bound outside the loop to avoid churn. Done. https://codereview.chromium.org/2834063002/diff/80001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler_impl.h (right): https://codereview.chromium.org/2834063002/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl.h:41: explicit TaskSchedulerImpl(StringPiece name); On 2017/04/25 15:16:15, gab wrote: > On 2017/04/24 22:32:19, robliao wrote: > > Readd the comment for |name| for public consistency. Done. > > Also mention explicitly that it now must be started (so that users of //base > from outside of chrome using this to SetInstance() know when rolling passed this > change that there's an extra step to fix). I'd like to avoid duplicating the meta comment from the base class. https://codereview.chromium.org/2834063002/diff/80001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/2834063002/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl_unittest.cc:285: // This should not hang if the task is scheduled after Start(). On 2017/04/25 15:16:15, gab wrote: > implicit IMO, remove comment (same below) Done. https://codereview.chromium.org/2834063002/diff/80001/base/test/scoped_task_s... File base/test/scoped_task_scheduler.cc (right): https://codereview.chromium.org/2834063002/diff/80001/base/test/scoped_task_s... base/test/scoped_task_scheduler.cc:169: // ScopedTaskScheduler intentionally breaks the TaskScheduler contract of not On 2017/04/25 15:16:15, gab wrote: > On 2017/04/24 22:32:19, robliao wrote: > > This comment should probably go at the top of TestTaskScheduler's declaration. > > Agreed, also this is another argument for getting rid of synchronous test task > scheduler. Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by fdoray@chromium.org
The CQ bit was checked by fdoray@chromium.org
The CQ bit was unchecked by fdoray@chromium.org
lgtm! Thanks.
The CQ bit was checked by fdoray@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": 120001, "attempt_start_ts": 1493154980984430,
"parent_rev": "f232398ebba7a9074272ac9c9c5fd0f73c73a5da", "commit_rev":
"0f358eedd390d9c0017ec60ff95c51cfa07d60a5"}
Message was sent while issue was closed.
Description was changed from ========== Separate the create and start phases in TaskSchedulerImpl. The task scheduler doesn't create threads before Start() is called. Tasks can be posted before Start(), but they don't run right away. Tasks can only run once Start() has been called. BUG=690706 ========== to ========== Separate the create and start phases in TaskSchedulerImpl. The task scheduler doesn't create threads before Start() is called. Tasks can be posted before Start(), but they don't run right away. Tasks can only run once Start() has been called. BUG=690706 Review-Url: https://codereview.chromium.org/2834063002 Cr-Commit-Position: refs/heads/master@{#467117} Committed: https://chromium.googlesource.com/chromium/src/+/0f358eedd390d9c0017ec60ff95c... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/0f358eedd390d9c0017ec60ff95c... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
