Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2698843006:
Introduce SchedulerSingleThreadTaskRunnerManager (Closed)
DescriptionIntroduce SchedulerSingleThreadTaskRunnerManager
This component provides one dedicated thread per
SingleThreadTaskRunner.
Reference Change:
https://codereview.chromium.org/2686593003/
BUG=684080, 694823, 697697
Review-Url: https://codereview.chromium.org/2698843006
Cr-Original-Commit-Position: refs/heads/master@{#453162}
Committed: https://chromium.googlesource.com/chromium/src/+/fc203f52e046a8d82e053525d764671f7128f3d4
Review-Url: https://codereview.chromium.org/2698843006
Cr-Commit-Position: refs/heads/master@{#456148}
Committed: https://chromium.googlesource.com/chromium/src/+/cee951ecb8aff83a6eb0ff734241813f735e8c45
Patch Set 1 #
Total comments: 26
Patch Set 2 : Delegate Refinement #Patch Set 3 : CR Feedback #
Total comments: 14
Patch Set 4 : CR Feedback #
Total comments: 8
Patch Set 5 : CR Feedback #
Total comments: 2
Patch Set 6 : CR Feedback #
Total comments: 2
Patch Set 7 : EXPECT_NE + Command + auto* #Patch Set 8 : Merge in https://codereview.chromium.org/2726073002/ #Messages
Total messages: 65 (45 generated)
robliao@chromium.org changed reviewers: + fdoray@chromium.org, gab@chromium.org
Here's the next one in the sequence!
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
https://codereview.chromium.org/2698843006/diff/1/base/task_scheduler/schedul... File base/task_scheduler/scheduler_single_thread_task_runner_manager.cc (right): https://codereview.chromium.org/2698843006/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:29: : public SingleThreadTaskRunner { Cleanup up of previous one will be in a follow-up CL? Mention this in CL desc and add TODOs in this CL https://codereview.chromium.org/2698843006/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:115: SchedulerLock sequence_lock_; Initially thought this wasn't necessary but then realized it is in the scenario where the sequence becomes empty but something new is posted and the sequence is re-enqueued before the thread calls GetWork() to realize it's empty and go to sleep. This is a side-effect of SchedulerWorker handling the non-single-threaded use case as well (i.e. emptying assigned sequence doesn't mean no more work for that worker). Add a comment to that effect? https://codereview.chromium.org/2698843006/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:190: worker->JoinForTesting(); Empty lines above and below this loop (otherwise it looks like you forgot to include one line in the {} below (i.e. as if it all belongs in the loop)) https://codereview.chromium.org/2698843006/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:193: DCHECK(workers_.empty()); << "New worker(s) unexpectedly registered during join." https://codereview.chromium.org/2698843006/diff/1/base/task_scheduler/schedul... File base/task_scheduler/scheduler_single_thread_task_runner_manager.h (right): https://codereview.chromium.org/2698843006/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.h:59: // Synchornizes access to |workers_| and |worker_id_|. "Synchronizes" https://codereview.chromium.org/2698843006/diff/1/base/task_scheduler/schedul... File base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc (right): https://codereview.chromium.org/2698843006/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc:20: namespace base { nit empty line above https://codereview.chromium.org/2698843006/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc:41: ASSERT_FALSE(delayed_task_manager_); It's not typical to ASSERT_FALSE on the fixture's ptrs at the top of SetUp() https://codereview.chromium.org/2698843006/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc:93: void ShouldNotRunCallback() { ShouldNotRun() -- i.e. I'd drop "Callback", it's only a Callback when it's bound and at that point it's implicit. https://codereview.chromium.org/2698843006/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc:212: TimeDelta::FromMilliseconds(250) + TestTimeouts::tiny_timeout()); Flip as TestTimeouts::tiny_timeout() + TimeDelta::FromMilliseconds(250) (i.e. 250ms *after* timeout to me sounds like "timeout + 250ms") https://codereview.chromium.org/2698843006/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc:225: scoped_refptr<SequencedTaskRunner> sequenced_task_runner( Perhaps arbitrary but I prefer = instead of () to init when not directly invoking a constructor (I don't find scoped_refptr's constructor counts). https://codereview.chromium.org/2698843006/diff/1/base/task_scheduler/task_sc... File base/task_scheduler/task_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/2698843006/diff/1/base/task_scheduler/task_sc... base/task_scheduler/task_scheduler_impl_unittest.cc:326: // returns false when called from a task that isn't part of the sequence. Note: "Note:" ? https://codereview.chromium.org/2698843006/diff/1/base/task_scheduler/task_sc... base/task_scheduler/task_scheduler_impl_unittest.cc:344: task_ran.Wait(); Also test the other way around? And drop TaskSchedulerSingleThreadTaskRunnerManagerTest.SequencedRunsTasksOnCurrentThread because it's redundant?
Thanks for the comments! https://codereview.chromium.org/2698843006/diff/1/base/task_scheduler/schedul... File base/task_scheduler/scheduler_single_thread_task_runner_manager.cc (right): https://codereview.chromium.org/2698843006/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:29: : public SingleThreadTaskRunner { On 2017/02/21 21:39:50, gab wrote: > Cleanup up of previous one will be in a follow-up CL? Mention this in CL desc > and add TODOs in this CL Added a TODO and a new bug instead of the CL description. https://codereview.chromium.org/2698843006/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:115: SchedulerLock sequence_lock_; On 2017/02/21 21:39:50, gab wrote: > Initially thought this wasn't necessary but then realized it is in the scenario > where the sequence becomes empty but something new is posted and the sequence is > re-enqueued before the thread calls GetWork() to realize it's empty and go to > sleep. > > This is a side-effect of SchedulerWorker handling the non-single-threaded use > case as well (i.e. emptying assigned sequence doesn't mean no more work for that > worker). > > Add a comment to that effect? Done. I emphasized that ReEnqueueSequence() in this case is called from on and off the worker thread, which is unlike other delegates. https://codereview.chromium.org/2698843006/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:190: worker->JoinForTesting(); On 2017/02/21 21:39:50, gab wrote: > Empty lines above and below this loop (otherwise it looks like you forgot to > include one line in the {} below (i.e. as if it all belongs in the loop)) My preference at this point is just to use brackets, but that's another debate :-) Added the lines. https://codereview.chromium.org/2698843006/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:193: DCHECK(workers_.empty()); On 2017/02/21 21:39:50, gab wrote: > << "New worker(s) unexpectedly registered during join." Done. https://codereview.chromium.org/2698843006/diff/1/base/task_scheduler/schedul... File base/task_scheduler/scheduler_single_thread_task_runner_manager.h (right): https://codereview.chromium.org/2698843006/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.h:59: // Synchornizes access to |workers_| and |worker_id_|. On 2017/02/21 21:39:50, gab wrote: > "Synchronizes" Done. https://codereview.chromium.org/2698843006/diff/1/base/task_scheduler/schedul... File base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc (right): https://codereview.chromium.org/2698843006/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc:20: namespace base { On 2017/02/21 21:39:50, gab wrote: > nit empty line above Done. https://codereview.chromium.org/2698843006/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc:41: ASSERT_FALSE(delayed_task_manager_); On 2017/02/21 21:39:50, gab wrote: > It's not typical to ASSERT_FALSE on the fixture's ptrs at the top of SetUp() Done. https://codereview.chromium.org/2698843006/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc:93: void ShouldNotRunCallback() { On 2017/02/21 21:39:50, gab wrote: > ShouldNotRun() -- i.e. I'd drop "Callback", it's only a Callback when it's bound > and at that point it's implicit. This was another holdover https://cs.chromium.org/chromium/src/base/task_scheduler/scheduler_worker_poo... Removed here and originating file updated at https://codereview.chromium.org/2710853002 https://codereview.chromium.org/2698843006/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc:212: TimeDelta::FromMilliseconds(250) + TestTimeouts::tiny_timeout()); On 2017/02/21 21:39:50, gab wrote: > Flip as > > TestTimeouts::tiny_timeout() + TimeDelta::FromMilliseconds(250) > > (i.e. 250ms *after* timeout to me sounds like "timeout + 250ms") The important part here is the 250ms. This should be read like 250ms + _epsilon. _eplison + 250ms doesn't flow as nicely. This is also consistent with our SchedulerWorkerPoolImpl tests. https://cs.chromium.org/chromium/src/base/task_scheduler/scheduler_worker_poo... https://codereview.chromium.org/2698843006/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc:225: scoped_refptr<SequencedTaskRunner> sequenced_task_runner( On 2017/02/21 21:39:50, gab wrote: > Perhaps arbitrary but I prefer = instead of () to init when not directly > invoking a constructor (I don't find scoped_refptr's constructor counts). Done one better and promoted to auto. Originating file updated at https://codereview.chromium.org/2710853002 https://codereview.chromium.org/2698843006/diff/1/base/task_scheduler/task_sc... File base/task_scheduler/task_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/2698843006/diff/1/base/task_scheduler/task_sc... base/task_scheduler/task_scheduler_impl_unittest.cc:326: // returns false when called from a task that isn't part of the sequence. Note: On 2017/02/21 21:39:50, gab wrote: > "Note:" ? Removed. (It's no longer needed) https://codereview.chromium.org/2698843006/diff/1/base/task_scheduler/task_sc... base/task_scheduler/task_scheduler_impl_unittest.cc:344: task_ran.Wait(); On 2017/02/21 21:39:50, gab wrote: > Also test the other way around? > > And drop > TaskSchedulerSingleThreadTaskRunnerManagerTest.SequencedRunsTasksOnCurrentThread > because it's redundant? Removed redundant test and added reverse.
lgtm https://codereview.chromium.org/2698843006/diff/80001/base/task_scheduler/sch... File base/task_scheduler/scheduler_single_thread_task_runner_manager.cc (right): https://codereview.chromium.org/2698843006/diff/80001/base/task_scheduler/sch... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:32: SchedulerWorkerDelegate(const std::string& thread_name) #include <string> https://codereview.chromium.org/2698843006/diff/80001/base/task_scheduler/sch... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:108: return sequence_->token() == SequenceToken::GetForCurrentThread(); Should we check the thread id instead? That way, RunsTaskOnCurrentThread() will return true if called from outside of TaskTracker::RunTask (could happen if we start running callbacks in response to COM messages). Checking the SequenceToken instead of the thread id was useful when tasks that were not posted to a SchedulerSingleThreadTaskRunner could run on the thread to which the SchedulerSingleThreadTaskRunner was assigned. https://codereview.chromium.org/2698843006/diff/80001/base/task_scheduler/sch... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:176: ~SchedulerSingleThreadTaskRunnerManager() = default; DCHECK(workers_.empty()) << "Deleting SchedulerSingleThreadTaskRunnerManager when there are still active SingleThreadTaskRunners."; If it's not empty, it means that a SingleThreadTaskRunner is still alive. When that SingleThreadTaskRunner is destroyed, it will call UnregisterSchedulerWorker() on |this| which has been deleted -> crash. https://codereview.chromium.org/2698843006/diff/80001/base/task_scheduler/sch... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:236: DCHECK(worker_iter != workers_.end()); DCHECK_NE? Maybe it doesn't work with iterators. https://codereview.chromium.org/2698843006/diff/80001/base/task_scheduler/sch... File base/task_scheduler/scheduler_single_thread_task_runner_manager.h (right): https://codereview.chromium.org/2698843006/diff/80001/base/task_scheduler/sch... base/task_scheduler/scheduler_single_thread_task_runner_manager.h:13: #include "base/synchronization/condition_variable.h" Not needed
https://codereview.chromium.org/2698843006/diff/1/base/task_scheduler/schedul... File base/task_scheduler/scheduler_single_thread_task_runner_manager.cc (right): https://codereview.chromium.org/2698843006/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:115: SchedulerLock sequence_lock_; On 2017/02/22 01:04:18, robliao wrote: > On 2017/02/21 21:39:50, gab wrote: > > Initially thought this wasn't necessary but then realized it is in the > scenario > > where the sequence becomes empty but something new is posted and the sequence > is > > re-enqueued before the thread calls GetWork() to realize it's empty and go to > > sleep. > > > > This is a side-effect of SchedulerWorker handling the non-single-threaded use > > case as well (i.e. emptying assigned sequence doesn't mean no more work for > that > > worker). > > > > Add a comment to that effect? > > Done. I emphasized that ReEnqueueSequence() in this case is called from on and > off the worker thread, which is unlike other delegates. I don't think this is unlike the other delegates. To the contrary I thought that unlike the other delegates, this one could do without a lock as ReEnqueueSequence() is never racing with itself (yes it can be called from different threads but only in sequence with the Sequence toggling between empty and non-empty states behind Sequence::lock_). The reason you need the lock is because GetWork() will be called one more time after the sequence became empty (and wasn't re-enqueued) and that GetWork() call can race with a ReEnqueueSequence() call caused by new work posted from another thread. This wouldn't be an issue if SchedulerWorker went to sleep immediately after emptying its Sequence but it doesn't because it doesn't assume GetWork() will return null after it emptied the current sequence (which would be a fine assumption in the single-threaded use case). https://codereview.chromium.org/2698843006/diff/80001/base/task_scheduler/sch... File base/task_scheduler/scheduler_single_thread_task_runner_manager.cc (right): https://codereview.chromium.org/2698843006/diff/80001/base/task_scheduler/sch... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:68: // ReEnqueueSequence() is called on both on the worker thread for reenqueuing s/on both on/both on/ ?
https://codereview.chromium.org/2698843006/diff/80001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_pool_impl.cc (right): https://codereview.chromium.org/2698843006/diff/80001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl.cc:144: // TODO(http://crbug.com/694823): Remove this and supporting framework. Refer to this bug in CL desc.
Description was changed from ========== Introduce SchedulerSingleThreadTaskRunnerManager This component provides one dedicated thread per SingleThreadTaskRunner. Reference Change: https://codereview.chromium.org/2686593003/ BUG=684080 ========== to ========== Introduce SchedulerSingleThreadTaskRunnerManager This component provides one dedicated thread per SingleThreadTaskRunner. Reference Change: https://codereview.chromium.org/2686593003/ BUG=684080, 694823 ==========
Patchset #4 (id:100001) has been deleted
Patchset #4 (id:120001) has been deleted
Patchset #4 (id:140001) has been deleted
Thanks for the comments! https://codereview.chromium.org/2698843006/diff/1/base/task_scheduler/schedul... File base/task_scheduler/scheduler_single_thread_task_runner_manager.cc (right): https://codereview.chromium.org/2698843006/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:115: SchedulerLock sequence_lock_; On 2017/02/22 18:18:21, gab wrote: > On 2017/02/22 01:04:18, robliao wrote: > > On 2017/02/21 21:39:50, gab wrote: > > > Initially thought this wasn't necessary but then realized it is in the > > scenario > > > where the sequence becomes empty but something new is posted and the > sequence > > is > > > re-enqueued before the thread calls GetWork() to realize it's empty and go > to > > > sleep. > > > > > > This is a side-effect of SchedulerWorker handling the non-single-threaded > use > > > case as well (i.e. emptying assigned sequence doesn't mean no more work for > > that > > > worker). > > > > > > Add a comment to that effect? > > > > Done. I emphasized that ReEnqueueSequence() in this case is called from on and > > off the worker thread, which is unlike other delegates. > > I don't think this is unlike the other delegates. To the contrary I thought that > unlike the other delegates, this one could do without a lock as > ReEnqueueSequence() is never racing with itself (yes it can be called from > different threads but only in sequence with the Sequence toggling between empty > and non-empty states behind Sequence::lock_). > > The reason you need the lock is because GetWork() will be called one more time > after the sequence became empty (and wasn't re-enqueued) and that GetWork() call > can race with a ReEnqueueSequence() call caused by new work posted from another > thread. This wouldn't be an issue if SchedulerWorker went to sleep immediately > after emptying its Sequence but it doesn't because it doesn't assume GetWork() > will return null after it emptied the current sequence (which would be a fine > assumption in the single-threaded use case). We specify in the delegate documentation that all of those methods are generally called on one thread: https://cs.chromium.org/chromium/src/base/task_scheduler/scheduler_worker.h?r... Violating this contract necessitates the use of locks. https://codereview.chromium.org/2698843006/diff/80001/base/task_scheduler/sch... File base/task_scheduler/scheduler_single_thread_task_runner_manager.cc (right): https://codereview.chromium.org/2698843006/diff/80001/base/task_scheduler/sch... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:32: SchedulerWorkerDelegate(const std::string& thread_name) On 2017/02/22 17:40:40, fdoray wrote: > #include <string> Done. https://codereview.chromium.org/2698843006/diff/80001/base/task_scheduler/sch... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:68: // ReEnqueueSequence() is called on both on the worker thread for reenqueuing On 2017/02/22 18:18:21, gab wrote: > s/on both on/both on/ ? ... on both the worker thread ... Done https://codereview.chromium.org/2698843006/diff/80001/base/task_scheduler/sch... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:108: return sequence_->token() == SequenceToken::GetForCurrentThread(); On 2017/02/22 17:40:40, fdoray wrote: > Should we check the thread id instead? That way, RunsTaskOnCurrentThread() will > return true if called from outside of TaskTracker::RunTask (could happen if we > start running callbacks in response to COM messages). > > Checking the SequenceToken instead of the thread id was useful when tasks that > were not posted to a SchedulerSingleThreadTaskRunner could run on the thread to > which the SchedulerSingleThreadTaskRunner was assigned. Hrm... now that's tricky as SchedulerWorker doesn't expose the notion of a real thread and rightfully so as the thread isn't bound to the SchedulerWorker's lifetime, but it would be useful for thread checking. Added a way to figure this out via the delegate. https://codereview.chromium.org/2698843006/diff/80001/base/task_scheduler/sch... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:176: ~SchedulerSingleThreadTaskRunnerManager() = default; On 2017/02/22 17:40:40, fdoray wrote: > DCHECK(workers_.empty()) << "Deleting SchedulerSingleThreadTaskRunnerManager > when there are still active SingleThreadTaskRunners."; > > If it's not empty, it means that a SingleThreadTaskRunner is still alive. When > that SingleThreadTaskRunner is destroyed, it will call > UnregisterSchedulerWorker() on |this| which has been deleted -> crash. Done. https://codereview.chromium.org/2698843006/diff/80001/base/task_scheduler/sch... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:236: DCHECK(worker_iter != workers_.end()); On 2017/02/22 17:40:40, fdoray wrote: > DCHECK_NE? Maybe it doesn't work with iterators. That's right. The template for value types like this isn't quite there. (It assumes this is nullptr'able). https://codereview.chromium.org/2698843006/diff/80001/base/task_scheduler/sch... File base/task_scheduler/scheduler_single_thread_task_runner_manager.h (right): https://codereview.chromium.org/2698843006/diff/80001/base/task_scheduler/sch... base/task_scheduler/scheduler_single_thread_task_runner_manager.h:13: #include "base/synchronization/condition_variable.h" On 2017/02/22 17:40:40, fdoray wrote: > Not needed Removed. https://codereview.chromium.org/2698843006/diff/80001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_pool_impl.cc (right): https://codereview.chromium.org/2698843006/diff/80001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl.cc:144: // TODO(http://crbug.com/694823): Remove this and supporting framework. On 2017/02/22 18:36:57, gab wrote: > Refer to this bug in CL desc. Done.
https://codereview.chromium.org/2698843006/diff/160001/base/task_scheduler/sc... File base/task_scheduler/scheduler_single_thread_task_runner_manager.cc (right): https://codereview.chromium.org/2698843006/diff/160001/base/task_scheduler/sc... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:66: thread_ref_ = PlatformThreadRef(); Actually, shouldn't we NOTREACHED() here? It doesn't make sense to detach a single-threaded worker as it then loses its only property : thread-affinity. https://codereview.chromium.org/2698843006/diff/160001/base/task_scheduler/sc... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:90: SchedulerLock thread_ref_lock_; Meh, I don't like locking for this. But, also, I didn't really mind checking SequenceToken directly... checking beyond that is mostly a check of our on impl (we can verify that in unit tests -- we already almost have that test but could expand the id check to a task running on that TR) for external users it should be equivalent. ************** Thought if we really wanted to a thread check: an event + thread_ref_. Then you can: return event.IsSignaled() && thread_ref_ == Current(); since RunsTasksOnCurrentThread() is only supposed to return true from its own thread, the event must be signaled or we haven't hit OnMainEntry() and the call can't possibly come from this thread. https://codereview.chromium.org/2698843006/diff/160001/base/task_scheduler/sc... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:198: "SingleThreadTaskRunners may crash."; "may crash" is too loose IMO, suggest: "SchedulerSingleThreadTaskRunners must outlive SchedulerSingleThreadTaskRunnerManager." (and this is then easy to verify for the reader per the SSTTRs taking a raw SSTTRM ptr)
Patchset #5 (id:180001) has been deleted
https://codereview.chromium.org/2698843006/diff/160001/base/task_scheduler/sc... File base/task_scheduler/scheduler_single_thread_task_runner_manager.cc (right): https://codereview.chromium.org/2698843006/diff/160001/base/task_scheduler/sc... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:66: thread_ref_ = PlatformThreadRef(); On 2017/02/23 19:48:10, gab wrote: > Actually, shouldn't we NOTREACHED() here? It doesn't make sense to detach a > single-threaded worker as it then loses its only property : thread-affinity. Indeed. You found a vestige of the std::unique_ptr based SchedulerWorker. Removed and asserted. https://codereview.chromium.org/2698843006/diff/160001/base/task_scheduler/sc... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:90: SchedulerLock thread_ref_lock_; On 2017/02/23 19:48:10, gab wrote: > Meh, I don't like locking for this. > > > But, also, I didn't really mind checking SequenceToken directly... checking > beyond that is mostly a check of our on impl (we can verify that in unit tests > -- we already almost have that test but could expand the id check to a task > running on that TR) for external users it should be equivalent. > > > ************** > > Thought if we really wanted to a thread check: an event + thread_ref_. > > Then you can: > > return event.IsSignaled() && thread_ref_ == Current(); > > since RunsTasksOnCurrentThread() is only supposed to return true from its own > thread, the event must be signaled or we haven't hit OnMainEntry() and the call > can't possibly come from this thread. Done via AtomicFlag instead of WaitableEvent. (WaitableEvents should not be used as atomic booleans as they consume real kernel resources). https://codereview.chromium.org/2698843006/diff/160001/base/task_scheduler/sc... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:198: "SingleThreadTaskRunners may crash."; On 2017/02/23 19:48:10, gab wrote: > "may crash" is too loose IMO, suggest: > > "SchedulerSingleThreadTaskRunners must outlive > SchedulerSingleThreadTaskRunnerManager." > > (and this is then easy to verify for the reader per the SSTTRs taking a raw > SSTTRM ptr) How about SchedulerSingleThreadTaskRunners must outlive SchedulerSingleThreadTaskRunnerManager otherwise the outstanding SchedulerSingleThreadTaskRunner will trigger crashes.
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...
https://codereview.chromium.org/2698843006/diff/160001/base/task_scheduler/sc... File base/task_scheduler/scheduler_single_thread_task_runner_manager.cc (right): https://codereview.chromium.org/2698843006/diff/160001/base/task_scheduler/sc... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:198: "SingleThreadTaskRunners may crash."; On 2017/02/23 21:26:06, robliao wrote: > On 2017/02/23 19:48:10, gab wrote: > > "may crash" is too loose IMO, suggest: > > > > "SchedulerSingleThreadTaskRunners must outlive > > SchedulerSingleThreadTaskRunnerManager." > > > > (and this is then easy to verify for the reader per the SSTTRs taking a raw > > SSTTRM ptr) > > How about > SchedulerSingleThreadTaskRunners must outlive > SchedulerSingleThreadTaskRunnerManager otherwise the outstanding > SchedulerSingleThreadTaskRunner will trigger crashes. I think "otherwise the outstanding SchedulerSingleThreadTaskRunner will trigger crashes" is implicit and would prefer not to have it. e.g. if every DCHECK(foo) had a << "will crash otherwise" string it would be reading overhead: it's implicit and easy to verify. https://codereview.chromium.org/2698843006/diff/200001/base/task_scheduler/sc... File base/task_scheduler/scheduler_single_thread_task_runner_manager.cc (right): https://codereview.chromium.org/2698843006/diff/200001/base/task_scheduler/sc... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:33: class AtomicThreadRefChecker { Don't think this warrants a separate class.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2698843006/diff/160001/base/task_scheduler/sc... File base/task_scheduler/scheduler_single_thread_task_runner_manager.cc (right): https://codereview.chromium.org/2698843006/diff/160001/base/task_scheduler/sc... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:198: "SingleThreadTaskRunners may crash."; On 2017/02/23 21:52:10, gab wrote: > On 2017/02/23 21:26:06, robliao wrote: > > On 2017/02/23 19:48:10, gab wrote: > > > "may crash" is too loose IMO, suggest: > > > > > > "SchedulerSingleThreadTaskRunners must outlive > > > SchedulerSingleThreadTaskRunnerManager." > > > > > > (and this is then easy to verify for the reader per the SSTTRs taking a raw > > > SSTTRM ptr) > > > > How about > > SchedulerSingleThreadTaskRunners must outlive > > SchedulerSingleThreadTaskRunnerManager otherwise the outstanding > > SchedulerSingleThreadTaskRunner will trigger crashes. > > I think "otherwise the outstanding SchedulerSingleThreadTaskRunner will trigger > crashes" is implicit and would prefer not to have it. > > e.g. if every DCHECK(foo) had a << "will crash otherwise" string it would be > reading overhead: it's implicit and easy to verify. Done. https://codereview.chromium.org/2698843006/diff/200001/base/task_scheduler/sc... File base/task_scheduler/scheduler_single_thread_task_runner_manager.cc (right): https://codereview.chromium.org/2698843006/diff/200001/base/task_scheduler/sc... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:33: class AtomicThreadRefChecker { On 2017/02/23 21:52:10, gab wrote: > Don't think this warrants a separate class. The joy of lock-free algorithms is that they're easy to get wrong. This class guards against future changes that use thread_ref_ without checking the atomic flag.
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...
lgtm w/ nit https://codereview.chromium.org/2698843006/diff/220001/base/task_scheduler/sc... File base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc (right): https://codereview.chromium.org/2698843006/diff/220001/base/task_scheduler/sc... base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc:119: EXPECT_FALSE(thread_ref_1 == thread_ref_2); EXPECT_NE?
https://codereview.chromium.org/2698843006/diff/220001/base/task_scheduler/sc... File base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc (right): https://codereview.chromium.org/2698843006/diff/220001/base/task_scheduler/sc... base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc:119: EXPECT_FALSE(thread_ref_1 == thread_ref_2); On 2017/02/24 18:44:17, gab wrote: > EXPECT_NE? Yeah, that will depend on my PlatformThreadRef change. I'll change this when that goes in.
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
The patchset sent to the CQ was uploaded after l-g-t-m from fdoray@chromium.org Link to the patchset: https://codereview.chromium.org/2698843006/#ps220001 (title: "CR Feedback")
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 robliao@chromium.org
Patchset #7 (id:240001) 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...
Patchset #7 (id:260001) 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: This issue passed the CQ dry run.
The CQ bit was checked by robliao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org, fdoray@chromium.org Link to the patchset: https://codereview.chromium.org/2698843006/#ps280001 (title: "EXPECT_NE + Command + auto*")
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": 280001, "attempt_start_ts": 1488177819496060,
"parent_rev": "c9eec829dfba011be14f3bb508129653fbdee3d6", "commit_rev":
"fc203f52e046a8d82e053525d764671f7128f3d4"}
Message was sent while issue was closed.
Description was changed from ========== Introduce SchedulerSingleThreadTaskRunnerManager This component provides one dedicated thread per SingleThreadTaskRunner. Reference Change: https://codereview.chromium.org/2686593003/ BUG=684080, 694823 ========== to ========== Introduce SchedulerSingleThreadTaskRunnerManager This component provides one dedicated thread per SingleThreadTaskRunner. Reference Change: https://codereview.chromium.org/2686593003/ BUG=684080, 694823 Review-Url: https://codereview.chromium.org/2698843006 Cr-Commit-Position: refs/heads/master@{#453162} Committed: https://chromium.googlesource.com/chromium/src/+/fc203f52e046a8d82e053525d764... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:280001) as https://chromium.googlesource.com/chromium/src/+/fc203f52e046a8d82e053525d764...
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:280001) has been created in https://codereview.chromium.org/2722113006/ by twellington@chromium.org. The reason for reverting is: TaskSchedulerSingleThreadTaskRunnerManagerTest.PrioritySetCorrectly is failing on numerous bots. See crbug.com/697697.
Message was sent while issue was closed.
Description was changed from ========== Introduce SchedulerSingleThreadTaskRunnerManager This component provides one dedicated thread per SingleThreadTaskRunner. Reference Change: https://codereview.chromium.org/2686593003/ BUG=684080, 694823 Review-Url: https://codereview.chromium.org/2698843006 Cr-Commit-Position: refs/heads/master@{#453162} Committed: https://chromium.googlesource.com/chromium/src/+/fc203f52e046a8d82e053525d764... ========== to ========== Introduce SchedulerSingleThreadTaskRunnerManager This component provides one dedicated thread per SingleThreadTaskRunner. Reference Change: https://codereview.chromium.org/2686593003/ BUG=684080, 694823 Review-Url: https://codereview.chromium.org/2698843006 Cr-Commit-Position: refs/heads/master@{#453162} Committed: https://chromium.googlesource.com/chromium/src/+/fc203f52e046a8d82e053525d764... ==========
On 2017/03/02 18:44:03, Theresa wrote: > A revert of this CL (patchset #7 id:280001) has been created in > https://codereview.chromium.org/2722113006/ by mailto:twellington@chromium.org. > > The reason for reverting is: > TaskSchedulerSingleThreadTaskRunnerManagerTest.PrioritySetCorrectly is failing > on numerous bots. See crbug.com/697697. Reopening for the merge.
Description was changed from ========== Introduce SchedulerSingleThreadTaskRunnerManager This component provides one dedicated thread per SingleThreadTaskRunner. Reference Change: https://codereview.chromium.org/2686593003/ BUG=684080, 694823 Review-Url: https://codereview.chromium.org/2698843006 Cr-Commit-Position: refs/heads/master@{#453162} Committed: https://chromium.googlesource.com/chromium/src/+/fc203f52e046a8d82e053525d764... ========== to ========== Introduce SchedulerSingleThreadTaskRunnerManager This component provides one dedicated thread per SingleThreadTaskRunner. Reference Change: https://codereview.chromium.org/2686593003/ BUG=684080, 694823, 697697 Review-Url: https://codereview.chromium.org/2698843006 Cr-Commit-Position: refs/heads/master@{#453162} Committed: https://chromium.googlesource.com/chromium/src/+/fc203f52e046a8d82e053525d764... ==========
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 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
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org, fdoray@chromium.org Link to the patchset: https://codereview.chromium.org/2698843006/#ps300001 (title: "Merge in https://codereview.chromium.org/2726073002/")
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": 300001, "attempt_start_ts": 1489176994000880,
"parent_rev": "67435cd8a4ae34c8005f848cb0a2e6b1ebabbafd", "commit_rev":
"cee951ecb8aff83a6eb0ff734241813f735e8c45"}
Message was sent while issue was closed.
Description was changed from ========== Introduce SchedulerSingleThreadTaskRunnerManager This component provides one dedicated thread per SingleThreadTaskRunner. Reference Change: https://codereview.chromium.org/2686593003/ BUG=684080, 694823, 697697 Review-Url: https://codereview.chromium.org/2698843006 Cr-Commit-Position: refs/heads/master@{#453162} Committed: https://chromium.googlesource.com/chromium/src/+/fc203f52e046a8d82e053525d764... ========== to ========== Introduce SchedulerSingleThreadTaskRunnerManager This component provides one dedicated thread per SingleThreadTaskRunner. Reference Change: https://codereview.chromium.org/2686593003/ BUG=684080, 694823, 697697 Review-Url: https://codereview.chromium.org/2698843006 Cr-Original-Commit-Position: refs/heads/master@{#453162} Committed: https://chromium.googlesource.com/chromium/src/+/fc203f52e046a8d82e053525d764... Review-Url: https://codereview.chromium.org/2698843006 Cr-Commit-Position: refs/heads/master@{#456148} Committed: https://chromium.googlesource.com/chromium/src/+/cee951ecb8aff83a6eb0ff734241... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:300001) as https://chromium.googlesource.com/chromium/src/+/cee951ecb8aff83a6eb0ff734241... |
