Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1911023002:
TaskScheduler: Add TaskRunnerHandle support to TaskScheduler tasks (Closed)
|
Created:
4 years, 8 months ago by gab Modified:
4 years, 7 months ago CC:
chromium-reviews, gab+watch_chromium.org, robliao+watch_chromium.org, fdoray+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@c1_1876363004_STTR Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTaskScheduler: Add TaskRunnerHandle support to TaskScheduler tasks.
A follow-up CL will be required to support base::SequenceChecker.
BUG=553459
Committed: https://crrev.com/a8e9e2c4ca588971ed8f3b9209a33ad3c563c586
Cr-Commit-Position: refs/heads/master@{#391051}
Patch Set 1 : #
Total comments: 11
Patch Set 2 : review:fdoray#9 #Patch Set 3 : rebase and more test logic in TestTaskFactory to cover SchedulerThreadPoolImpl changes #
Total comments: 12
Patch Set 4 : review:fdoray#17 #
Total comments: 8
Patch Set 5 : review:fdoray#19 nit #Patch Set 6 : merge up to r390397 #
Total comments: 2
Patch Set 7 : review:robliao#24 nit #Patch Set 8 : merge up to r390520 #
Total comments: 13
Patch Set 9 : review:danakj#28 #
Messages
Total messages: 37 (14 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
gab@chromium.org changed reviewers: + fdoray@chromium.org, robliao@chromium.org
Francois/Rob PTAL (this is based on top of the SingleThreadTaskRunner CL but can easily mold into wherever it ends up). Cheers! Gab
Description was changed from ========== TaskScheduler: Add TaskRunnerHandle support to TaskScheduler tasks BUG=553459 ========== to ========== TaskScheduler: Add TaskRunnerHandle support to TaskScheduler tasks A follow-up CL will be required to support base::SequenceChecker BUG=553459 ==========
Description was changed from ========== TaskScheduler: Add TaskRunnerHandle support to TaskScheduler tasks A follow-up CL will be required to support base::SequenceChecker BUG=553459 ========== to ========== TaskScheduler: Add TaskRunnerHandle support to TaskScheduler tasks. A follow-up CL will be required to support base::SequenceChecker. BUG=553459 ==========
lgtm, except for base/task_scheduler/scheduler_thread_pool.cc and base/task_scheduler/scheduler_worker_thread.cc which don't apply to our latest code. https://codereview.chromium.org/1911023002/diff/30011/base/task_scheduler/task.h File base/task_scheduler/task.h (right): https://codereview.chromium.org/1911023002/diff/30011/base/task_scheduler/tas... base/task_scheduler/task.h:48: scoped_refptr<SingleThreadTaskRunner> single_thread_task_runner_ref; Should we comment about the ownership cycle that this creates? Sequence -> Task -> TaskRunner -> Sequence -> ... We must be careful when we drop a reference to a non-empty Sequence. https://codereview.chromium.org/1911023002/diff/30011/base/threading/sequence... File base/threading/sequenced_task_runner_handle.h (right): https://codereview.chromium.org/1911023002/diff/30011/base/threading/sequence... base/threading/sequenced_task_runner_handle.h:41: DISALLOW_IMPLICIT_CONSTRUCTORS(SequencedTaskRunnerHandle); DISALLOW_IMPLICIT_CONSTRUCTORS should be used "for a class that wants to prevent anyone from instantiating it". I think you should use DISALLOW_COPY_AND_ASSIGN even though it doesn't change anything in practice. https://code.google.com/p/chromium/codesearch#chromium/src/base/macros.h&l=34 https://codereview.chromium.org/1911023002/diff/30011/base/threading/sequence... File base/threading/sequenced_task_runner_handle_unittest.cc (right): https://codereview.chromium.org/1911023002/diff/30011/base/threading/sequence... base/threading/sequenced_task_runner_handle_unittest.cc:21: #include "base/threading/sequenced_task_runner_handle.h" Already included at the top of the file.
Thanks, comments addressed, will rebase and FYI-ping here when new code lands. https://codereview.chromium.org/1911023002/diff/30011/base/task_scheduler/task.h File base/task_scheduler/task.h (right): https://codereview.chromium.org/1911023002/diff/30011/base/task_scheduler/tas... base/task_scheduler/task.h:48: scoped_refptr<SingleThreadTaskRunner> single_thread_task_runner_ref; On 2016/04/22 14:43:12, fdoray wrote: > Should we comment about the ownership cycle that this creates? > Sequence -> Task -> TaskRunner -> Sequence -> ... Done, this cycle is okay (and in fact required) as I tried to explain in the added comment. > > We must be careful when we drop a reference to a non-empty Sequence. Why? This would only happen if a TaskRunner goes away while its Sequence is non-empty and this is precisely why this cycle is required. I don't see what we have to be careful about? https://codereview.chromium.org/1911023002/diff/30011/base/threading/sequence... File base/threading/sequenced_task_runner_handle.h (right): https://codereview.chromium.org/1911023002/diff/30011/base/threading/sequence... base/threading/sequenced_task_runner_handle.h:41: DISALLOW_IMPLICIT_CONSTRUCTORS(SequencedTaskRunnerHandle); On 2016/04/22 14:43:12, fdoray wrote: > DISALLOW_IMPLICIT_CONSTRUCTORS should be used "for a class that wants to prevent > anyone from instantiating it". I think you should use DISALLOW_COPY_AND_ASSIGN > even though it doesn't change anything in practice. > > https://code.google.com/p/chromium/codesearch#chromium/src/base/macros.h&l=34 Good point, DISALLOW_IMPLICIT_CONSTRUCTORS made sense before this CL, but now that it can be instantiated DISALLOW_COPY_AND_ASSIGN is the right thing. Also added it to ThreadTaskRunnerHandle. https://codereview.chromium.org/1911023002/diff/30011/base/threading/sequence... File base/threading/sequenced_task_runner_handle_unittest.cc (right): https://codereview.chromium.org/1911023002/diff/30011/base/threading/sequence... base/threading/sequenced_task_runner_handle_unittest.cc:21: #include "base/threading/sequenced_task_runner_handle.h" On 2016/04/22 14:43:12, fdoray wrote: > Already included at the top of the file. Done.
https://codereview.chromium.org/1911023002/diff/30011/base/task_scheduler/task.h File base/task_scheduler/task.h (right): https://codereview.chromium.org/1911023002/diff/30011/base/task_scheduler/tas... base/task_scheduler/task.h:48: scoped_refptr<SingleThreadTaskRunner> single_thread_task_runner_ref; On 2016/04/25 18:31:52, gab wrote: > On 2016/04/22 14:43:12, fdoray wrote: > > Should we comment about the ownership cycle that this creates? > > Sequence -> Task -> TaskRunner -> Sequence -> ... > > Done, this cycle is okay (and in fact required) as I tried to explain in the > added comment. > > > > > We must be careful when we drop a reference to a non-empty Sequence. > > Why? This would only happen if a TaskRunner goes away while its Sequence is > non-empty and this is precisely why this cycle is required. I don't see what we > have to be careful about? We could easily make a change that creates a memory leak (e.g. add an API to cancel a Sequence, which releases a ref without emptying the Sequence first). It would be nice to DCHECK_GT(number of refs, number of Tasks in Sequence) whenever a ref is released, but RefCountedThreadSafe doesn't give us access to the number of refs.
https://codereview.chromium.org/1911023002/diff/30011/base/task_scheduler/task.h File base/task_scheduler/task.h (right): https://codereview.chromium.org/1911023002/diff/30011/base/task_scheduler/tas... base/task_scheduler/task.h:48: scoped_refptr<SingleThreadTaskRunner> single_thread_task_runner_ref; On 2016/04/25 18:53:27, fdoray wrote: > On 2016/04/25 18:31:52, gab wrote: > > On 2016/04/22 14:43:12, fdoray wrote: > > > Should we comment about the ownership cycle that this creates? > > > Sequence -> Task -> TaskRunner -> Sequence -> ... > > > > Done, this cycle is okay (and in fact required) as I tried to explain in the > > added comment. > > > > > > > > We must be careful when we drop a reference to a non-empty Sequence. > > > > Why? This would only happen if a TaskRunner goes away while its Sequence is > > non-empty and this is precisely why this cycle is required. I don't see what > we > > have to be careful about? > > We could easily make a change that creates a memory leak (e.g. add an API to > cancel a Sequence, which releases a ref without emptying the Sequence first). It > would be nice to DCHECK_GT(number of refs, number of Tasks in Sequence) whenever > a ref is released, but RefCountedThreadSafe doesn't give us access to the number > of refs. Ah I see, shall we add a DCHECK(IsEmpty()) in Sequence's destructor?
Patchset #4 (id:130001) has been deleted
Patchset #3 (id:110001) has been deleted
Rebased and added another bit of test code to have test coverage for modification to SchedulerThreadPoolImpl. PTAL. Thanks! Gab https://codereview.chromium.org/1911023002/diff/30011/base/task_scheduler/task.h File base/task_scheduler/task.h (right): https://codereview.chromium.org/1911023002/diff/30011/base/task_scheduler/tas... base/task_scheduler/task.h:48: scoped_refptr<SingleThreadTaskRunner> single_thread_task_runner_ref; On 2016/04/26 11:54:57, gab wrote: > On 2016/04/25 18:53:27, fdoray wrote: > > On 2016/04/25 18:31:52, gab wrote: > > > On 2016/04/22 14:43:12, fdoray wrote: > > > > Should we comment about the ownership cycle that this creates? > > > > Sequence -> Task -> TaskRunner -> Sequence -> ... > > > > > > Done, this cycle is okay (and in fact required) as I tried to explain in the > > > added comment. > > > > > > > > > > > We must be careful when we drop a reference to a non-empty Sequence. > > > > > > Why? This would only happen if a TaskRunner goes away while its Sequence is > > > non-empty and this is precisely why this cycle is required. I don't see what > > we > > > have to be careful about? > > > > We could easily make a change that creates a memory leak (e.g. add an API to > > cancel a Sequence, which releases a ref without emptying the Sequence first). > It > > would be nice to DCHECK_GT(number of refs, number of Tasks in Sequence) > whenever > > a ref is released, but RefCountedThreadSafe doesn't give us access to the > number > > of refs. > > Ah I see, shall we add a DCHECK(IsEmpty()) in Sequence's destructor? ping to keep this question on our mind, WDYT?
https://codereview.chromium.org/1911023002/diff/150001/base/task_scheduler/te... File base/task_scheduler/test_task_factory.cc (right): https://codereview.chromium.org/1911023002/diff/150001/base/task_scheduler/te... base/task_scheduler/test_task_factory.cc:58: switch (execution_mode_) { Note to self (realized this while doing another review and will get to it later): Expand comment on class TestTaskFactory; to add this as a test case ran by this class.
still lgtm https://codereview.chromium.org/1911023002/diff/30011/base/task_scheduler/task.h File base/task_scheduler/task.h (right): https://codereview.chromium.org/1911023002/diff/30011/base/task_scheduler/tas... base/task_scheduler/task.h:48: scoped_refptr<SingleThreadTaskRunner> single_thread_task_runner_ref; On 2016/04/26 21:20:20, gab wrote: > On 2016/04/26 11:54:57, gab wrote: > > On 2016/04/25 18:53:27, fdoray wrote: > > > On 2016/04/25 18:31:52, gab wrote: > > > > On 2016/04/22 14:43:12, fdoray wrote: > > > > > Should we comment about the ownership cycle that this creates? > > > > > Sequence -> Task -> TaskRunner -> Sequence -> ... > > > > > > > > Done, this cycle is okay (and in fact required) as I tried to explain in > the > > > > added comment. > > > > > > > > > > > > > > We must be careful when we drop a reference to a non-empty Sequence. > > > > > > > > Why? This would only happen if a TaskRunner goes away while its Sequence > is > > > > non-empty and this is precisely why this cycle is required. I don't see > what > > > we > > > > have to be careful about? > > > > > > We could easily make a change that creates a memory leak (e.g. add an API to > > > cancel a Sequence, which releases a ref without emptying the Sequence > first). > > It > > > would be nice to DCHECK_GT(number of refs, number of Tasks in Sequence) > > whenever > > > a ref is released, but RefCountedThreadSafe doesn't give us access to the > > number > > > of refs. > > > > Ah I see, shall we add a DCHECK(IsEmpty()) in Sequence's destructor? > > ping to keep this question on our mind, WDYT? It's a good idea, but it doesn't solve the problem. I think the best we can do is to warn people about the cycle + maybe add a comment that says that a non-empty Sequence must always be owned by a worker thread or a PQ (it's not ok if the only scoped_refptr to a non-empty Sequence is in a TaskRunner). https://codereview.chromium.org/1911023002/diff/150001/base/task_scheduler/ta... File base/task_scheduler/task.h (right): https://codereview.chromium.org/1911023002/diff/150001/base/task_scheduler/ta... base/task_scheduler/task.h:43: // A reference to the SequencedTaskRunner or SingleThreadedTaskRunner that SingleThreadTaskRunner ^ https://code.google.com/p/chromium/codesearch#chromium/src/base/single_thread... https://codereview.chromium.org/1911023002/diff/150001/base/task_scheduler/ta... base/task_scheduler/task.h:51: // support ThreadTaskRunnerHandles. s/ThreadTaskRunnerHandles/TaskRunnerHandles/ ? https://codereview.chromium.org/1911023002/diff/150001/base/task_scheduler/ta... base/task_scheduler/task.h:56: // Disallow copies to make sure no unecessary ref-bumps are incurred. Making unnecessary ^ https://codereview.chromium.org/1911023002/diff/150001/base/task_scheduler/ta... File base/task_scheduler/task_tracker_unittest.cc (right): https://codereview.chromium.org/1911023002/diff/150001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:17: #include "base/task_runner.h" not used? https://codereview.chromium.org/1911023002/diff/150001/base/threading/sequenc... File base/threading/sequenced_task_runner_handle.cc (right): https://codereview.chromium.org/1911023002/diff/150001/base/threading/sequenc... base/threading/sequenced_task_runner_handle.cc:38: // Return the SingleThreadedTaskRunner for the current thread otherwise. SingleThreadTaskRunner ^
Done, added a comment about the refcount cycle in sequence.h, let me know what you think. PS: I really feel we could do something better than refcounting here but I know we've had this chat before and had concluded we couldn't so it's not something I'll fix in this CL :-) https://codereview.chromium.org/1911023002/diff/30011/base/task_scheduler/task.h File base/task_scheduler/task.h (right): https://codereview.chromium.org/1911023002/diff/30011/base/task_scheduler/tas... base/task_scheduler/task.h:48: scoped_refptr<SingleThreadTaskRunner> single_thread_task_runner_ref; On 2016/04/27 18:11:20, fdoray wrote: > On 2016/04/26 21:20:20, gab wrote: > > On 2016/04/26 11:54:57, gab wrote: > > > On 2016/04/25 18:53:27, fdoray wrote: > > > > On 2016/04/25 18:31:52, gab wrote: > > > > > On 2016/04/22 14:43:12, fdoray wrote: > > > > > > Should we comment about the ownership cycle that this creates? > > > > > > Sequence -> Task -> TaskRunner -> Sequence -> ... > > > > > > > > > > Done, this cycle is okay (and in fact required) as I tried to explain in > > the > > > > > added comment. > > > > > > > > > > > > > > > > > We must be careful when we drop a reference to a non-empty Sequence. > > > > > > > > > > Why? This would only happen if a TaskRunner goes away while its Sequence > > is > > > > > non-empty and this is precisely why this cycle is required. I don't see > > what > > > > we > > > > > have to be careful about? > > > > > > > > We could easily make a change that creates a memory leak (e.g. add an API > to > > > > cancel a Sequence, which releases a ref without emptying the Sequence > > first). > > > It > > > > would be nice to DCHECK_GT(number of refs, number of Tasks in Sequence) > > > whenever > > > > a ref is released, but RefCountedThreadSafe doesn't give us access to the > > > number > > > > of refs. > > > > > > Ah I see, shall we add a DCHECK(IsEmpty()) in Sequence's destructor? > > > > ping to keep this question on our mind, WDYT? > > It's a good idea, but it doesn't solve the problem. Ah right, that's not such a great idea actually since it merely documents that Tasks will hold on to the Sequence but wouldn't fire in the case we're worried about. > I think the best we can do > is to warn people about the cycle + maybe add a comment that says that a > non-empty Sequence must always be owned by a worker thread or a PQ (it's not ok > if the only scoped_refptr to a non-empty Sequence is in a TaskRunner). Added documentation to sequence.h (it's sad to mention implementations details in that header, but I don't see where else to put that comment that would make sure people reading code relevant to this would see it). Let me know what you think :-) https://codereview.chromium.org/1911023002/diff/150001/base/task_scheduler/ta... File base/task_scheduler/task.h (right): https://codereview.chromium.org/1911023002/diff/150001/base/task_scheduler/ta... base/task_scheduler/task.h:43: // A reference to the SequencedTaskRunner or SingleThreadedTaskRunner that On 2016/04/27 18:11:20, fdoray wrote: > SingleThreadTaskRunner > ^ > https://code.google.com/p/chromium/codesearch#chromium/src/base/single_thread... Done. https://codereview.chromium.org/1911023002/diff/150001/base/task_scheduler/ta... base/task_scheduler/task.h:51: // support ThreadTaskRunnerHandles. On 2016/04/27 18:11:20, fdoray wrote: > s/ThreadTaskRunnerHandles/TaskRunnerHandles/ ? Done. https://codereview.chromium.org/1911023002/diff/150001/base/task_scheduler/ta... base/task_scheduler/task.h:56: // Disallow copies to make sure no unecessary ref-bumps are incurred. Making On 2016/04/27 18:11:20, fdoray wrote: > unnecessary > ^ Done. https://codereview.chromium.org/1911023002/diff/150001/base/task_scheduler/ta... File base/task_scheduler/task_tracker_unittest.cc (right): https://codereview.chromium.org/1911023002/diff/150001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:17: #include "base/task_runner.h" On 2016/04/27 18:11:21, fdoray wrote: > not used? Done. https://codereview.chromium.org/1911023002/diff/150001/base/task_scheduler/te... File base/task_scheduler/test_task_factory.cc (right): https://codereview.chromium.org/1911023002/diff/150001/base/task_scheduler/te... base/task_scheduler/test_task_factory.cc:58: switch (execution_mode_) { On 2016/04/27 15:26:59, gab wrote: > Note to self (realized this while doing another review and will get to it > later): Expand comment on class TestTaskFactory; to add this as a test case ran > by this class. Done. https://codereview.chromium.org/1911023002/diff/150001/base/threading/sequenc... File base/threading/sequenced_task_runner_handle.cc (right): https://codereview.chromium.org/1911023002/diff/150001/base/threading/sequenc... base/threading/sequenced_task_runner_handle.cc:38: // Return the SingleThreadedTaskRunner for the current thread otherwise. On 2016/04/27 18:11:21, fdoray wrote: > SingleThreadTaskRunner > ^ Done.
still lgtm with one nit https://codereview.chromium.org/1911023002/diff/170001/base/task_scheduler/se... File base/task_scheduler/sequence.h (right): https://codereview.chromium.org/1911023002/diff/170001/base/task_scheduler/se... base/task_scheduler/sequence.h:37: // the caller that the Sequence should be re-enqueued for execution). Nice comment :) https://codereview.chromium.org/1911023002/diff/170001/base/task_scheduler/te... File base/task_scheduler/test_task_factory.h (right): https://codereview.chromium.org/1911023002/diff/170001/base/task_scheduler/te... base/task_scheduler/test_task_factory.h:30: // - expected for the tested ExecutionMode. no hyphen on this line
gab@chromium.org changed reviewers: + danakj@chromium.org
@danakj for OWNERS (Rob I'll assume you're leaving this one up to Francois/Dana per silence?) https://codereview.chromium.org/1911023002/diff/170001/base/task_scheduler/se... File base/task_scheduler/sequence.h (right): https://codereview.chromium.org/1911023002/diff/170001/base/task_scheduler/se... base/task_scheduler/sequence.h:37: // the caller that the Sequence should be re-enqueued for execution). On 2016/04/28 14:37:52, fdoray wrote: > Nice comment :) Thanks :-)
Oops, "done" :-) @danakj for OWNERS (Rob I'll assume you're leaving this one up to Francois/Dana per silence?) https://codereview.chromium.org/1911023002/diff/170001/base/task_scheduler/te... File base/task_scheduler/test_task_factory.h (right): https://codereview.chromium.org/1911023002/diff/170001/base/task_scheduler/te... base/task_scheduler/test_task_factory.h:30: // - expected for the tested ExecutionMode. On 2016/04/28 14:37:52, fdoray wrote: > no hyphen on this line Done.
On 2016/04/28 15:43:23, gab wrote: > Oops, "done" :-) > > @danakj for OWNERS > > (Rob I'll assume you're leaving this one up to Francois/Dana per silence?) > > https://codereview.chromium.org/1911023002/diff/170001/base/task_scheduler/te... > File base/task_scheduler/test_task_factory.h (right): > > https://codereview.chromium.org/1911023002/diff/170001/base/task_scheduler/te... > base/task_scheduler/test_task_factory.h:30: // - expected for the tested > ExecutionMode. > On 2016/04/28 14:37:52, fdoray wrote: > > no hyphen on this line > > Done. It's on my queue :-)
lgtm. https://codereview.chromium.org/1911023002/diff/170001/base/task_scheduler/ta... File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/1911023002/diff/170001/base/task_scheduler/ta... base/task_scheduler/task_tracker.cc:87: DCHECK(!task->sequenced_task_runner_ref || Alternatively, you can have an else clause below with NOTREACHED() which would accomplish the same thing. https://codereview.chromium.org/1911023002/diff/210001/base/task_scheduler/ta... File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/1911023002/diff/210001/base/task_scheduler/ta... base/task_scheduler/task_tracker.cc:85: std::unique_ptr<SequencedTaskRunnerHandle> sequenced_trh; Nit: trh should be expanded.
Thanks, @rob: 1 done, 1 reply. @danakj for OWNER, thanks! https://codereview.chromium.org/1911023002/diff/170001/base/task_scheduler/ta... File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/1911023002/diff/170001/base/task_scheduler/ta... base/task_scheduler/task_tracker.cc:87: DCHECK(!task->sequenced_task_runner_ref || On 2016/04/28 17:16:42, robliao wrote: > Alternatively, you can have an else clause below with NOTREACHED() which would > accomplish the same thing. An else clause below would be a DCHECK(task->sequenced_task_runner_ref || task->single_thread_task_runner_ref); which is not what I want (it's okay to have no task_runner_ref). What this is checking is that |task| has either 1 or 0 (not 2) task_runner_refs set. It's equivalent to this (which I could use if it's clearer): DCHECK(!(task->sequenced_task_runner_ref && task->single_thread_task_runner_ref)); https://codereview.chromium.org/1911023002/diff/210001/base/task_scheduler/ta... File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/1911023002/diff/210001/base/task_scheduler/ta... base/task_scheduler/task_tracker.cc:85: std::unique_ptr<SequencedTaskRunnerHandle> sequenced_trh; On 2016/04/28 17:16:42, robliao wrote: > Nit: trh should be expanded. Done.
https://codereview.chromium.org/1911023002/diff/170001/base/task_scheduler/ta... File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/1911023002/diff/170001/base/task_scheduler/ta... base/task_scheduler/task_tracker.cc:87: DCHECK(!task->sequenced_task_runner_ref || On 2016/04/28 17:55:07, gab wrote: > On 2016/04/28 17:16:42, robliao wrote: > > Alternatively, you can have an else clause below with NOTREACHED() which would > > accomplish the same thing. > > An else clause below would be a > > DCHECK(task->sequenced_task_runner_ref || > task->single_thread_task_runner_ref); > > which is not what I want (it's okay to have no task_runner_ref). > > What this is checking is that |task| has either 1 or 0 (not 2) task_runner_refs > set. > > It's equivalent to this (which I could use if it's clearer): > > DCHECK(!(task->sequenced_task_runner_ref && > task->single_thread_task_runner_ref)); Ah yes, my misreading. I think Chrome tends to prefer the nongrouped demorganized form, so it's fine as is. Alternatively then you could distribute the DCHECK. if (task->sequenced_task_runner_ref) { DCHECK(!task->single_Thread_task_runner_ref) } ... (The else if doesn't need the DCHECK as sequenced_task_runner_ref must be false) Your choice.
@danakj, all yours :-)! https://codereview.chromium.org/1911023002/diff/170001/base/task_scheduler/ta... File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/1911023002/diff/170001/base/task_scheduler/ta... base/task_scheduler/task_tracker.cc:87: DCHECK(!task->sequenced_task_runner_ref || On 2016/04/28 18:06:47, robliao wrote: > On 2016/04/28 17:55:07, gab wrote: > > On 2016/04/28 17:16:42, robliao wrote: > > > Alternatively, you can have an else clause below with NOTREACHED() which > would > > > accomplish the same thing. > > > > An else clause below would be a > > > > DCHECK(task->sequenced_task_runner_ref || > > task->single_thread_task_runner_ref); > > > > which is not what I want (it's okay to have no task_runner_ref). > > > > What this is checking is that |task| has either 1 or 0 (not 2) > task_runner_refs > > set. > > > > It's equivalent to this (which I could use if it's clearer): > > > > DCHECK(!(task->sequenced_task_runner_ref && > > task->single_thread_task_runner_ref)); > > Ah yes, my misreading. I think Chrome tends to prefer the nongrouped > demorganized form, so it's fine as is. > Alternatively then you could distribute the DCHECK. > if (task->sequenced_task_runner_ref) { > DCHECK(!task->single_Thread_task_runner_ref) > } ... > (The else if doesn't need the DCHECK as sequenced_task_runner_ref must be false) > Your choice. I prefer the current one as it reads more as "the two of them can't be set at same time; if the first is do X; if the second is do Y" which I prefer to "if the first is, confirm that the second isn't, do X; if the second is do Y".
https://codereview.chromium.org/1911023002/diff/250001/base/task_scheduler/se... File base/task_scheduler/sequence.h (right): https://codereview.chromium.org/1911023002/diff/250001/base/task_scheduler/se... base/task_scheduler/sequence.h:29: // this is okay so long as the other owners of Sequence (PriorityQueue and "This" https://codereview.chromium.org/1911023002/diff/250001/base/task_scheduler/ta... File base/task_scheduler/task_tracker_unittest.cc (right): https://codereview.chromium.org/1911023002/diff/250001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:330: void RunTaskRunnerHandleVerificationTask(TaskTracker* tracker, static https://codereview.chromium.org/1911023002/diff/250001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:347: void VerifyNoTaskRunnerHandle() { static https://codereview.chromium.org/1911023002/diff/250001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:362: void VerifySequencedTaskRunnerHandle( static https://codereview.chromium.org/1911023002/diff/250001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:385: void VerifyThreadTaskRunnerHandle( static https://codereview.chromium.org/1911023002/diff/250001/base/threading/sequenc... File base/threading/sequenced_task_runner_handle.cc (right): https://codereview.chromium.org/1911023002/diff/250001/base/threading/sequenc... base/threading/sequenced_task_runner_handle.cc:32: // Return the SequencedTaskRunner obtained from SequencedWorkerPool, if any. These can't combine right? Maybe DCHECK this is null in the return block above? Can SequencedWorkerPool use this new mechanism to bind itself to the thread?
@Dana: all done, PTAL, thanks! https://codereview.chromium.org/1911023002/diff/250001/base/task_scheduler/se... File base/task_scheduler/sequence.h (right): https://codereview.chromium.org/1911023002/diff/250001/base/task_scheduler/se... base/task_scheduler/sequence.h:29: // this is okay so long as the other owners of Sequence (PriorityQueue and On 2016/04/29 22:36:34, danakj wrote: > "This" Done. https://codereview.chromium.org/1911023002/diff/250001/base/task_scheduler/ta... File base/task_scheduler/task_tracker_unittest.cc (right): https://codereview.chromium.org/1911023002/diff/250001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:330: void RunTaskRunnerHandleVerificationTask(TaskTracker* tracker, On 2016/04/29 22:36:34, danakj wrote: > static Ah interesting rare use of the static keyword in its alternate meaning (make this local to the .cc) since these aren't in the anonymous namespace :-). I'll use this paradigm for such helpers from now on, thanks! https://codereview.chromium.org/1911023002/diff/250001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:347: void VerifyNoTaskRunnerHandle() { On 2016/04/29 22:36:34, danakj wrote: > static Done. https://codereview.chromium.org/1911023002/diff/250001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:362: void VerifySequencedTaskRunnerHandle( On 2016/04/29 22:36:34, danakj wrote: > static Done. https://codereview.chromium.org/1911023002/diff/250001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:385: void VerifyThreadTaskRunnerHandle( On 2016/04/29 22:36:34, danakj wrote: > static Done. https://codereview.chromium.org/1911023002/diff/250001/base/threading/sequenc... File base/threading/sequenced_task_runner_handle.cc (right): https://codereview.chromium.org/1911023002/diff/250001/base/threading/sequenc... base/threading/sequenced_task_runner_handle.cc:32: // Return the SequencedTaskRunner obtained from SequencedWorkerPool, if any. On 2016/04/29 22:36:34, danakj wrote: > These can't combine right? Maybe DCHECK this is null in the return block above? Good point, DCHECKs added. > > Can SequencedWorkerPool use this new mechanism to bind itself to the thread? SequencedWorkerPool doesn't have a reference to the TaskRunner that posted the task so it actually creates a new one on demand (ref. SequencedWorkerPool::GetSequencedTaskRunnerForCurrentThread()). Therefore I don't think we want to have it preemptively create one for every task just in case. And any other solution to keep a ref to the TaskRunner would be overly involved IMO given it's soon being deprecated by TaskScheduler anyways :-).
LGTM https://codereview.chromium.org/1911023002/diff/250001/base/threading/sequenc... File base/threading/sequenced_task_runner_handle.cc (right): https://codereview.chromium.org/1911023002/diff/250001/base/threading/sequenc... base/threading/sequenced_task_runner_handle.cc:32: // Return the SequencedTaskRunner obtained from SequencedWorkerPool, if any. On 2016/05/02 14:36:54, gab wrote: > On 2016/04/29 22:36:34, danakj wrote: > > These can't combine right? Maybe DCHECK this is null in the return block > above? > > Good point, DCHECKs added. > > > > > Can SequencedWorkerPool use this new mechanism to bind itself to the thread? > > SequencedWorkerPool doesn't have a reference to the TaskRunner that posted the > task so it actually creates a new one on demand (ref. > SequencedWorkerPool::GetSequencedTaskRunnerForCurrentThread()). Therefore I > don't think we want to have it preemptively create one for every task just in > case. Ah, right :/ > And any other solution to keep a ref to the TaskRunner would be overly involved > IMO given it's soon being deprecated by TaskScheduler anyways :-).
The CQ bit was checked by gab@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fdoray@chromium.org, robliao@chromium.org Link to the patchset: https://codereview.chromium.org/1911023002/#ps270001 (title: "review:danakj#28")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1911023002/270001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1911023002/270001
Message was sent while issue was closed.
Description was changed from ========== TaskScheduler: Add TaskRunnerHandle support to TaskScheduler tasks. A follow-up CL will be required to support base::SequenceChecker. BUG=553459 ========== to ========== TaskScheduler: Add TaskRunnerHandle support to TaskScheduler tasks. A follow-up CL will be required to support base::SequenceChecker. BUG=553459 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:270001)
Message was sent while issue was closed.
Description was changed from ========== TaskScheduler: Add TaskRunnerHandle support to TaskScheduler tasks. A follow-up CL will be required to support base::SequenceChecker. BUG=553459 ========== to ========== TaskScheduler: Add TaskRunnerHandle support to TaskScheduler tasks. A follow-up CL will be required to support base::SequenceChecker. BUG=553459 Committed: https://crrev.com/a8e9e2c4ca588971ed8f3b9209a33ad3c563c586 Cr-Commit-Position: refs/heads/master@{#391051} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/a8e9e2c4ca588971ed8f3b9209a33ad3c563c586 Cr-Commit-Position: refs/heads/master@{#391051} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
