Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2721553003:
Remove SingleThreadTaskRunner Support from SchedulerWorkerPoolImpl (Closed)
DescriptionRemove SingleThreadTaskRunner Support from SchedulerWorkerPoolImpl
With SchedulerSingleThreadTaskRunnerManager, SchedulerWorkerPoolImpl
no longer needs to handle SingleThreadTaskRunners.
BUG=694823
Review-Url: https://codereview.chromium.org/2721553003
Cr-Commit-Position: refs/heads/master@{#457124}
Committed: https://chromium.googlesource.com/chromium/src/+/94b6750c5377b3e4079ea0d74fbbb0e8912df346
Patch Set 1 #
Total comments: 7
Patch Set 2 : CR Feedback #Patch Set 3 : Rebase to 08266b3 #
Total comments: 13
Messages
Total messages: 32 (21 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...
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...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
Patchset #1 (id:1) 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...
robliao@chromium.org changed reviewers: + fdoray@chromium.org, gab@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2721553003/diff/20001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_pool.h (right): https://codereview.chromium.org/2721553003/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool.h:13: #include "base/single_thread_task_runner.h" No longer needed. https://codereview.chromium.org/2721553003/diff/20001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_pool_impl_unittest.cc (right): https://codereview.chromium.org/2721553003/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl_unittest.cc:124: default: If you have no default and you forget to handle an enum value, clang complains. E.g.: error: enumeration value 'SEQUENCED' not handled in switch [-Werror,-Wswitch] I would not add a default to make sure that we are warned at compile time if we forget to handle a value.
https://codereview.chromium.org/2721553003/diff/20001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_pool.h (right): https://codereview.chromium.org/2721553003/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool.h:13: #include "base/single_thread_task_runner.h" On 2017/03/14 18:35:47, fdoray wrote: > No longer needed. Done. https://codereview.chromium.org/2721553003/diff/20001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_pool_impl_unittest.cc (right): https://codereview.chromium.org/2721553003/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl_unittest.cc:124: default: On 2017/03/14 18:35:47, fdoray wrote: > If you have no default and you forget to handle an enum value, clang complains. > E.g.: > error: enumeration value 'SEQUENCED' not handled in switch [-Werror,-Wswitch] > > I would not add a default to make sure that we are warned at compile time if we > forget to handle a value. Indeed. I would agree if the goal here is to be exhaustive. Now that we're no longer handling the SINGLE_THREADED execution mode, it's likely we won't handle others (e.g. the COM execution mode if there is one). Since it's not clear this should be an exhaustive enumeration anymore, and having clang complain just because we added an enumeration value seems to be noisy. As a general practice, all new code is generally associated with tests, so it's unlikely we would miss this when adding a new task runner to SchedulerWorkerPoolImpl. Happy to add it back if you feel strongly about it.
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: This issue passed the CQ dry run.
lgtm
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: This issue passed the CQ dry run.
The CQ bit was checked by robliao@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": 60001, "attempt_start_ts": 1489599124812020,
"parent_rev": "88c63227018cd614c0eab270dbc2c6aea43a79ec", "commit_rev":
"94b6750c5377b3e4079ea0d74fbbb0e8912df346"}
Message was sent while issue was closed.
Description was changed from ========== Remove SingleThreadTaskRunner Support from SchedulerWorkerPoolImpl With SchedulerSingleThreadTaskRunnerManager, SchedulerWorkerPoolImpl no longer needs to handle SingleThreadTaskRunners. BUG=694823 ========== to ========== Remove SingleThreadTaskRunner Support from SchedulerWorkerPoolImpl With SchedulerSingleThreadTaskRunnerManager, SchedulerWorkerPoolImpl no longer needs to handle SingleThreadTaskRunners. BUG=694823 Review-Url: https://codereview.chromium.org/2721553003 Cr-Commit-Position: refs/heads/master@{#457124} Committed: https://chromium.googlesource.com/chromium/src/+/94b6750c5377b3e4079ea0d74fbb... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/chromium/src/+/94b6750c5377b3e4079ea0d74fbb...
Message was sent while issue was closed.
Woot, lgtm w/ comments + I think we can get rid of Sequence::Transaction as a follow-up! https://codereview.chromium.org/2721553003/diff/60001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_pool_impl.cc (right): https://codereview.chromium.org/2721553003/diff/60001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl.cc:289: // Wake up a worker to process |sequence|. rm now redundant comment https://codereview.chromium.org/2721553003/diff/60001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl.cc:396: outer_->shared_priority_queue_.BeginTransaction()); This was the only use case of transaction I believe. We can now atomically attempt to pop and do something if it came out empty. Let's do this in a follow-up CL? (it will also be easier to bring it back if it's needed later if it was stripped in its own CL) Add TODO here for now. https://codereview.chromium.org/2721553003/diff/60001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl.cc:558: // TODO(robliao): Honor StandbyThreadPolicy::ONE here and consider adding Shouldn't this TODO be in CanDetach()? https://codereview.chromium.org/2721553003/diff/60001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_pool_impl_unittest.cc (right): https://codereview.chromium.org/2721553003/diff/60001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl_unittest.cc:124: default: Keep SINGLE_THREADED and NOTREACHED() instead of having a default case
Message was sent while issue was closed.
> I think we can get rid of Sequence::Transaction as a follow-up! Yes. This CL is focused on SchedulerWorkerPoolImpl https://codereview.chromium.org/2721553003/diff/60001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_pool_impl.cc (right): https://codereview.chromium.org/2721553003/diff/60001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl.cc:289: // Wake up a worker to process |sequence|. On 2017/03/15 20:20:58, gab wrote: > rm now redundant comment This comment is consistent with the current style of the function. Removing this one means we can remove others as well. (Personally, there are quite a few comments we could remove.) https://codereview.chromium.org/2721553003/diff/60001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl.cc:396: outer_->shared_priority_queue_.BeginTransaction()); On 2017/03/15 20:20:58, gab wrote: > This was the only use case of transaction I believe. We can now atomically > attempt to pop and do something if it came out empty. > > Let's do this in a follow-up CL? (it will also be easier to bring it back if > it's needed later if it was stripped in its own CL) > > Add TODO here for now. Correct. This is focused on just SchedulerWorkerPoolImpl. Since there's a bug covering this, that's better covered there. https://codereview.chromium.org/2721553003/diff/60001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl.cc:558: // TODO(robliao): Honor StandbyThreadPolicy::ONE here and consider adding On 2017/03/15 20:20:58, gab wrote: > Shouldn't this TODO be in CanDetach()? This was the best place to put it that respected the spirit of the location. https://codereview.chromium.org/2721553003/diff/60001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_pool_impl_unittest.cc (right): https://codereview.chromium.org/2721553003/diff/60001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl_unittest.cc:124: default: On 2017/03/15 20:20:58, gab wrote: > Keep SINGLE_THREADED and NOTREACHED() instead of having a default case See https://codereview.chromium.org/2721553003/diff/20001/base/task_scheduler/sch... for rationale. The short of it: This is no longer expected to handle all cases exhaustively, so having clang warn on adding new cases is noisy and unnecessary.
Message was sent while issue was closed.
https://codereview.chromium.org/2721553003/diff/20001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_pool_impl_unittest.cc (right): https://codereview.chromium.org/2721553003/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl_unittest.cc:124: default: On 2017/03/14 22:58:28, robliao wrote: > On 2017/03/14 18:35:47, fdoray wrote: > > If you have no default and you forget to handle an enum value, clang > complains. > > E.g.: > > error: enumeration value 'SEQUENCED' not handled in switch [-Werror,-Wswitch] > > > > I would not add a default to make sure that we are warned at compile time if > we > > forget to handle a value. > > Indeed. I would agree if the goal here is to be exhaustive. Now that we're no > longer handling the SINGLE_THREADED execution mode, it's likely we won't handle > others (e.g. the COM execution mode if there is one). > > Since it's not clear this should be an exhaustive enumeration anymore, and > having clang complain just because we added an enumeration value seems to be > noisy. > > As a general practice, all new code is generally associated with tests, so it's > unlikely we would miss this when adding a new task runner to > SchedulerWorkerPoolImpl. > > Happy to add it back if you feel strongly about it. In that case we can keep default but we should still NOTREACHED if it's called with an unhandled case https://codereview.chromium.org/2721553003/diff/60001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_pool_impl.cc (right): https://codereview.chromium.org/2721553003/diff/60001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl.cc:289: // Wake up a worker to process |sequence|. On 2017/03/15 20:46:44, robliao wrote: > On 2017/03/15 20:20:58, gab wrote: > > rm now redundant comment > > This comment is consistent with the current style of the function. Removing this > one means we can remove others as well. > > (Personally, there are quite a few comments we could remove.) Hmm ok, we at least shouldn't add more (and I'm happy to see a CL to remove others). Redundant comments are never desired, even for "consistency" IMO https://codereview.chromium.org/2721553003/diff/60001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl.cc:396: outer_->shared_priority_queue_.BeginTransaction()); On 2017/03/15 20:46:44, robliao wrote: > On 2017/03/15 20:20:58, gab wrote: > > This was the only use case of transaction I believe. We can now atomically > > attempt to pop and do something if it came out empty. > > > > Let's do this in a follow-up CL? (it will also be easier to bring it back if > > it's needed later if it was stripped in its own CL) > > > > Add TODO here for now. > > Correct. This is focused on just SchedulerWorkerPoolImpl. Since there's a bug > covering this, that's better covered there. Unless you have a CL doing this now I still prefer a TODO, it's better for readability (people will read the code, not the bug list). A TODO referring to the bug is the best way to connect the two.
Message was sent while issue was closed.
https://codereview.chromium.org/2721553003/diff/20001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_pool_impl_unittest.cc (right): https://codereview.chromium.org/2721553003/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl_unittest.cc:124: default: On 2017/03/16 15:34:45, gab wrote: > On 2017/03/14 22:58:28, robliao wrote: > > On 2017/03/14 18:35:47, fdoray wrote: > > > If you have no default and you forget to handle an enum value, clang > > complains. > > > E.g.: > > > error: enumeration value 'SEQUENCED' not handled in switch > [-Werror,-Wswitch] > > > > > > I would not add a default to make sure that we are warned at compile time if > > we > > > forget to handle a value. > > > > Indeed. I would agree if the goal here is to be exhaustive. Now that we're no > > longer handling the SINGLE_THREADED execution mode, it's likely we won't > handle > > others (e.g. the COM execution mode if there is one). > > > > Since it's not clear this should be an exhaustive enumeration anymore, and > > having clang complain just because we added an enumeration value seems to be > > noisy. > > > > As a general practice, all new code is generally associated with tests, so > it's > > unlikely we would miss this when adding a new task runner to > > SchedulerWorkerPoolImpl. > > > > Happy to add it back if you feel strongly about it. > > In that case we can keep default but we should still NOTREACHED if it's called > with an unhandled case That suggests we should be using NOTREACHED() at the bottom instead of ADD_FAILURE(). Thoughts? https://codereview.chromium.org/2721553003/diff/60001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_pool_impl.cc (right): https://codereview.chromium.org/2721553003/diff/60001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl.cc:289: // Wake up a worker to process |sequence|. On 2017/03/16 15:34:46, gab wrote: > On 2017/03/15 20:46:44, robliao wrote: > > On 2017/03/15 20:20:58, gab wrote: > > > rm now redundant comment > > > > This comment is consistent with the current style of the function. Removing > this > > one means we can remove others as well. > > > > (Personally, there are quite a few comments we could remove.) > > Hmm ok, we at least shouldn't add more (and I'm happy to see a CL to remove > others). Redundant comments are never desired, even for "consistency" IMO While I agree with your opinion, the style guide is pretty clear on consistency in that it must be followed. https://google.github.io/styleguide/cppguide.html#Windows_Code (Scroll to bottom, consistency is in all caps) https://codereview.chromium.org/2721553003/diff/60001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl.cc:396: outer_->shared_priority_queue_.BeginTransaction()); On 2017/03/16 15:34:45, gab (offsite.. back Monday) wrote: > On 2017/03/15 20:46:44, robliao wrote: > > On 2017/03/15 20:20:58, gab wrote: > > > This was the only use case of transaction I believe. We can now atomically > > > attempt to pop and do something if it came out empty. > > > > > > Let's do this in a follow-up CL? (it will also be easier to bring it back if > > > it's needed later if it was stripped in its own CL) > > > > > > Add TODO here for now. > > > > Correct. This is focused on just SchedulerWorkerPoolImpl. Since there's a bug > > covering this, that's better covered there. > > Unless you have a CL doing this now I still prefer a TODO, it's better for > readability (people will read the code, not the bug list). A TODO referring to > the bug is the best way to connect the two. In progress at the moment!
Message was sent while issue was closed.
https://codereview.chromium.org/2721553003/diff/20001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_pool_impl_unittest.cc (right): https://codereview.chromium.org/2721553003/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl_unittest.cc:124: default: On 2017/03/16 22:49:03, robliao wrote: > On 2017/03/16 15:34:45, gab wrote: > > On 2017/03/14 22:58:28, robliao wrote: > > > On 2017/03/14 18:35:47, fdoray wrote: > > > > If you have no default and you forget to handle an enum value, clang > > > complains. > > > > E.g.: > > > > error: enumeration value 'SEQUENCED' not handled in switch > > [-Werror,-Wswitch] > > > > > > > > I would not add a default to make sure that we are warned at compile time > if > > > we > > > > forget to handle a value. > > > > > > Indeed. I would agree if the goal here is to be exhaustive. Now that we're > no > > > longer handling the SINGLE_THREADED execution mode, it's likely we won't > > handle > > > others (e.g. the COM execution mode if there is one). > > > > > > Since it's not clear this should be an exhaustive enumeration anymore, and > > > having clang complain just because we added an enumeration value seems to be > > > noisy. > > > > > > As a general practice, all new code is generally associated with tests, so > > it's > > > unlikely we would miss this when adding a new task runner to > > > SchedulerWorkerPoolImpl. > > > > > > Happy to add it back if you feel strongly about it. > > > > In that case we can keep default but we should still NOTREACHED if it's called > > with an unhandled case > > That suggests we should be using NOTREACHED() at the bottom instead of > ADD_FAILURE(). Thoughts? Ah, had missed this was in a unittest. ADD_FAILURE() is preferred to NOTREACHED() then. But I'd prefer to move both the ADD_FAILURE() and return nullptr to the default: case (I think that will compile and not complain about missing return statement?) https://codereview.chromium.org/2721553003/diff/60001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_pool_impl.cc (right): https://codereview.chromium.org/2721553003/diff/60001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl.cc:396: outer_->shared_priority_queue_.BeginTransaction()); On 2017/03/16 22:49:03, robliao wrote: > On 2017/03/16 15:34:45, gab (offsite.. back Monday) wrote: > > On 2017/03/15 20:46:44, robliao wrote: > > > On 2017/03/15 20:20:58, gab wrote: > > > > This was the only use case of transaction I believe. We can now atomically > > > > attempt to pop and do something if it came out empty. > > > > > > > > Let's do this in a follow-up CL? (it will also be easier to bring it back > if > > > > it's needed later if it was stripped in its own CL) > > > > > > > > Add TODO here for now. > > > > > > Correct. This is focused on just SchedulerWorkerPoolImpl. Since there's a > bug > > > covering this, that's better covered there. > > > > Unless you have a CL doing this now I still prefer a TODO, it's better for > > readability (people will read the code, not the bug list). A TODO referring to > > the bug is the best way to connect the two. > > In progress at the moment! Ok but for future reference a comment in code referring to bug is always preferable :). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
