Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2362253002:
TaskScheduler: Add FlushForTesting(). (Closed)
DescriptionTaskScheduler: Add FlushForTesting().
Many browser tests depend on being able to wait until there are no
pending undelayed tasks in the blocking pool (via
content::RunAllBlockingPoolTasksUntilIdle). This CL brings this
functionality to TaskScheduler.
BUG=622400, 553459
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_tsan_rel_ng
Committed: https://crrev.com/fe309b0183048023246ccbefaf0cf4df0f05e29b
Cr-Commit-Position: refs/heads/master@{#420901}
Patch Set 1 #
Total comments: 8
Patch Set 2 : CR gab #3 #
Total comments: 8
Patch Set 3 : CR robliao #10 #Patch Set 4 : self-review #
Total comments: 8
Patch Set 5 : (ignore this) #Patch Set 6 : CR danakj #12 #Patch Set 7 : CR robliao #13 #Patch Set 8 : fix build error #
Messages
Total messages: 31 (12 generated)
fdoray@chromium.org changed reviewers: + gab@chromium.org, robliao@chromium.org
PTAL
lgtm % comments https://codereview.chromium.org/2362253002/diff/1/base/task_scheduler/task_tr... File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/2362253002/diff/1/base/task_scheduler/task_tr... base/task_scheduler/task_tracker.cc:161: subtle::NoBarrier_AtomicIncrement(&num_pending_undelayed_tasks_, 1); Suggest adding CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_tsan_rel_ng to your CL desc to test TSAN in CQ instead of post-commit. https://codereview.chromium.org/2362253002/diff/1/base/task_scheduler/task_tr... base/task_scheduler/task_tracker.cc:216: return can_run_task; Does changing the return value of RunTask belong in this CL or is this done for another reason? https://codereview.chromium.org/2362253002/diff/1/base/task_scheduler/task_tr... File base/task_scheduler/task_tracker_unittest.cc (right): https://codereview.chromium.org/2362253002/diff/1/base/task_scheduler/task_tr... base/task_scheduler/task_tracker_unittest.cc:494: TimeDelta::FromSeconds(10)); Make this minutes so that the test would timeout if the flush waits for it (10s is still within the range where the test wouldn't timeout)
Description was changed from ========== TaskScheduler: Add FlushForTesting(). Many browser tests depend on being able to wait until there are no pending undelayed tasks in the blocking pool (via content::RunAllBlockingPoolTasksUntilIdle). This CL brings this functionality to TaskScheduler. BUG=622400, 553459 ========== to ========== TaskScheduler: Add FlushForTesting(). Many browser tests depend on being able to wait until there are no pending undelayed tasks in the blocking pool (via content::RunAllBlockingPoolTasksUntilIdle). This CL brings this functionality to TaskScheduler. BUG=622400, 553459 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_tsan_rel_ng ==========
robliao@: PTAL Can I TBR this to danakj@ or should I get it reviewed by her? https://codereview.chromium.org/2362253002/diff/1/base/task_scheduler/task_tr... File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/2362253002/diff/1/base/task_scheduler/task_tr... base/task_scheduler/task_tracker.cc:161: subtle::NoBarrier_AtomicIncrement(&num_pending_undelayed_tasks_, 1); On 2016/09/23 15:39:18, gab wrote: > Suggest adding > > CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_tsan_rel_ng > > to your CL desc to test TSAN in CQ instead of post-commit. Done. https://codereview.chromium.org/2362253002/diff/1/base/task_scheduler/task_tr... base/task_scheduler/task_tracker.cc:216: return can_run_task; On 2016/09/23 15:39:18, gab wrote: > Does changing the return value of RunTask belong in this CL or is this done for > another reason? I changed this to have a single exit point in RunTask(). Having a single exit point allows me to have a single call to DecrementNumPendingUndelayedTasks(). The behavior of RunTask() doesn't change. https://codereview.chromium.org/2362253002/diff/1/base/task_scheduler/task_tr... File base/task_scheduler/task_tracker_unittest.cc (right): https://codereview.chromium.org/2362253002/diff/1/base/task_scheduler/task_tr... base/task_scheduler/task_tracker_unittest.cc:494: TimeDelta::FromSeconds(10)); On 2016/09/23 15:39:18, gab wrote: > Make this minutes so that the test would timeout if the flush waits for it (10s > is still within the range where the test wouldn't timeout) Ok. But since I'm just testing TaskTracker (not the whole TaskScheduler), it is already guaranteed that this test will timeout if the flush waits for the delayed task (there isn't another thread that can call RunTask).
lgtm++ Let's ask Dana just so she's aware of the addition of one more FlushForTesting() callsite (i.e. because of https://codereview.chromium.org/2337253003#msg12). https://codereview.chromium.org/2362253002/diff/1/base/task_scheduler/task_tr... File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/2362253002/diff/1/base/task_scheduler/task_tr... base/task_scheduler/task_tracker.cc:216: return can_run_task; On 2016/09/23 15:56:50, fdoray wrote: > On 2016/09/23 15:39:18, gab wrote: > > Does changing the return value of RunTask belong in this CL or is this done > for > > another reason? > > I changed this to have a single exit point in RunTask(). Having a single exit > point allows me to have a single call to DecrementNumPendingUndelayedTasks(). > > The behavior of RunTask() doesn't change. Ah makes sense, had missed the removal of the return false; https://codereview.chromium.org/2362253002/diff/1/base/task_scheduler/task_tr... File base/task_scheduler/task_tracker_unittest.cc (right): https://codereview.chromium.org/2362253002/diff/1/base/task_scheduler/task_tr... base/task_scheduler/task_tracker_unittest.cc:494: TimeDelta::FromSeconds(10)); On 2016/09/23 15:56:50, fdoray wrote: > On 2016/09/23 15:39:18, gab wrote: > > Make this minutes so that the test would timeout if the flush waits for it > (10s > > is still within the range where the test wouldn't timeout) > > Ok. But since I'm just testing TaskTracker (not the whole TaskScheduler), it is > already guaranteed that this test will timeout if the flush waits for the > delayed task (there isn't another thread that can call RunTask). Sure, but that makes it clear. Increasing readability for free is always a yes to me :-)
fdoray@chromium.org changed reviewers: + danakj@chromium.org
danakj@: PTAL
On 2016/09/23 17:11:20, fdoray wrote: > danakj@: PTAL @danakj: mostly want you to be aware of TaskScheduler::FlushForTesting(), feel free to defer the impl's review to us, thanks!
lgtm % comments. https://codereview.chromium.org/2362253002/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/2362253002/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_tracker.cc:232: void TaskTracker::ShutdownInternal() { Instead of ShutdownInternal, maybe PerformShutdown? https://codereview.chromium.org/2362253002/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_tracker.h (right): https://codereview.chromium.org/2362253002/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_tracker.h:112: mutable SchedulerLock flush_for_testing_lock_; CV's are generally thread safe, so it's very suspicious to see a lock synchronizing a CV. This is also partially synchronizing num_pending_undelayed_tasks_ on the zero check, so that scheme should be mentioned here. https://codereview.chromium.org/2362253002/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_tracker_unittest.cc (right): https://codereview.chromium.org/2362253002/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_tracker_unittest.cc:491: TEST_P(TaskSchedulerTaskTrackerTest, FlushForTestingDelayedTask) { Should we test what happens if a delayed task fires during flush for testing?
robliao@: Done. danakj@: PTAL https://codereview.chromium.org/2362253002/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/2362253002/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_tracker.cc:232: void TaskTracker::ShutdownInternal() { On 2016/09/23 17:24:30, robliao wrote: > Instead of ShutdownInternal, maybe PerformShutdown? Done. https://codereview.chromium.org/2362253002/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_tracker.h (right): https://codereview.chromium.org/2362253002/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_tracker.h:112: mutable SchedulerLock flush_for_testing_lock_; On 2016/09/23 17:24:30, robliao wrote: > CV's are generally thread safe, so it's very suspicious to see a lock > synchronizing a CV. > > This is also partially synchronizing num_pending_undelayed_tasks_ on the zero > check, so that scheme should be mentioned here. Every condition variable is associated with a lock https://cs.chromium.org/chromium/src/base/synchronization/condition_variable.... The lock must be held when ConditionVariable::Wait is called. Also, the lock must be acquired at some point between when the condition changes and when the ConditionVariable is signaled (can also be acquired before the condition changes). Otherwise, this can happen: WITHOUT LOCK ACQUISITION: Thread 1: AutoLock auto_lock(lock); while (!condition) { // condition is false Thread 2: condition = true; cv->Signal(); Thread 1: // Missed the signal. cv->Wait(); // Will wait forever. WITHOUT LOCK ACQUISITION: Thread 1: AutoLock auto_lock(lock); while (!condition) { // condition is false Thread 2: condition = true; AutoLock auto_lock(lock); // Blocks since thread 1 holds the lock. Thread 1: cv->Wait(); // Releases the lock and blocks. Thread 2: cv->Signal(); // Lock is released. Thread 1: cv->Wait(); // Acquires the lock and Wait() returns. while (!condition) { // condition is now true! Since this is true for any ConditionVariable, I'm not sure what you would me to add in the comment here? https://codereview.chromium.org/2362253002/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_tracker_unittest.cc (right): https://codereview.chromium.org/2362253002/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_tracker_unittest.cc:491: TEST_P(TaskSchedulerTaskTrackerTest, FlushForTestingDelayedTask) { On 2016/09/23 17:24:30, robliao wrote: > Should we test what happens if a delayed task fires during flush for testing? Done.
I didn't super look at the implementation details but LGTM % robliao/gab https://codereview.chromium.org/2362253002/diff/60001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_unittest.cc (right): https://codereview.chromium.org/2362253002/diff/60001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_unittest.cc:361: EXPECT_TRUE(task_tracker_); This is a case where I'd suggest ASSERT_TRUE because we're going to crash if this fails. https://codereview.chromium.org/2362253002/diff/60001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler_impl.cc (right): https://codereview.chromium.org/2362253002/diff/60001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl.cc:61: task_tracker_.FlushForTesting(); This is production code calling a ForTesting method. You need to not use ForTesting in the name of the task tracker's method or you're leaving a presubmit bomb for everyone else who touches this file. https://codereview.chromium.org/2362253002/diff/60001/base/task_scheduler/tas... File base/task_scheduler/task_tracker.h (right): https://codereview.chromium.org/2362253002/diff/60001/base/task_scheduler/tas... base/task_scheduler/task_tracker.h:109: subtle::Atomic32 num_pending_undelayed_tasks_ = 0; I think it'd be nice to document that you intend this to be used without barriers and explain why it is safe here. https://codereview.chromium.org/2362253002/diff/60001/base/task_scheduler/tas... File base/task_scheduler/task_tracker_unittest.cc (right): https://codereview.chromium.org/2362253002/diff/60001/base/task_scheduler/tas... base/task_scheduler/task_tracker_unittest.cc:49: // Returns true once the async call to Shutdown() has returned. Comment needs updating
https://codereview.chromium.org/2362253002/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_tracker.h (right): https://codereview.chromium.org/2362253002/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_tracker.h:112: mutable SchedulerLock flush_for_testing_lock_; On 2016/09/23 20:35:33, fdoray wrote: > On 2016/09/23 17:24:30, robliao wrote: > > CV's are generally thread safe, so it's very suspicious to see a lock > > synchronizing a CV. > > > > This is also partially synchronizing num_pending_undelayed_tasks_ on the zero > > check, so that scheme should be mentioned here. > > Every condition variable is associated with a lock > https://cs.chromium.org/chromium/src/base/synchronization/condition_variable.... > The lock must be held when ConditionVariable::Wait is called. Also, the lock > must be acquired at some point between when the condition changes and when the > ConditionVariable is signaled (can also be acquired before the condition > changes). Otherwise, this can happen: > > WITHOUT LOCK ACQUISITION: > > Thread 1: > AutoLock auto_lock(lock); > while (!condition) { // condition is false > > Thread 2: > condition = true; > cv->Signal(); > > Thread 1: > // Missed the signal. > cv->Wait(); // Will wait forever. > > WITHOUT LOCK ACQUISITION: > > Thread 1: > AutoLock auto_lock(lock); > while (!condition) { // condition is false > > Thread 2: > condition = true; > AutoLock auto_lock(lock); // Blocks since thread 1 holds the lock. > > Thread 1: > cv->Wait(); // Releases the lock and blocks. > > Thread 2: > cv->Signal(); > // Lock is released. > > Thread 1: > cv->Wait(); // Acquires the lock and Wait() returns. > while (!condition) { // condition is now true! > > > Since this is true for any ConditionVariable, I'm not sure what you would me to > add in the comment here? Agreed, as you state, the CV+Lock is only necessary because you need to look at other state (in this case num_pending_undelayed_tasks_). If no other state was necessary, you'd use a WaitableEvent instead. flush_for_testing_lock_ here is really for the benefit of num_pending_undelayed_tasks_ and not for flush_for_testing_cv_. So instead I'd update the comment to say that this lock is associated with |flush_for_testing_cv| and partially-synchronizes access to num_pending_undelayed_tasks_. Full synchronization isn't needed because it's atomic, but we need synchronization to coordinate waking and sleeping at the right time.
https://codereview.chromium.org/2362253002/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_tracker.h (right): https://codereview.chromium.org/2362253002/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_tracker.h:112: mutable SchedulerLock flush_for_testing_lock_; On 2016/09/23 21:05:46, robliao wrote: > On 2016/09/23 20:35:33, fdoray wrote: > > On 2016/09/23 17:24:30, robliao wrote: > > > CV's are generally thread safe, so it's very suspicious to see a lock > > > synchronizing a CV. > > > > > > This is also partially synchronizing num_pending_undelayed_tasks_ on the > zero > > > check, so that scheme should be mentioned here. > > > > Every condition variable is associated with a lock > > > https://cs.chromium.org/chromium/src/base/synchronization/condition_variable.... > > The lock must be held when ConditionVariable::Wait is called. Also, the lock > > must be acquired at some point between when the condition changes and when the > > ConditionVariable is signaled (can also be acquired before the condition > > changes). Otherwise, this can happen: > > > > WITHOUT LOCK ACQUISITION: > > > > Thread 1: > > AutoLock auto_lock(lock); > > while (!condition) { // condition is false > > > > Thread 2: > > condition = true; > > cv->Signal(); > > > > Thread 1: > > // Missed the signal. > > cv->Wait(); // Will wait forever. > > > > WITHOUT LOCK ACQUISITION: > > > > Thread 1: > > AutoLock auto_lock(lock); > > while (!condition) { // condition is false > > > > Thread 2: > > condition = true; > > AutoLock auto_lock(lock); // Blocks since thread 1 holds the lock. > > > > Thread 1: > > cv->Wait(); // Releases the lock and blocks. > > > > Thread 2: > > cv->Signal(); > > // Lock is released. > > > > Thread 1: > > cv->Wait(); // Acquires the lock and Wait() returns. > > while (!condition) { // condition is now true! > > > > > > Since this is true for any ConditionVariable, I'm not sure what you would me > to > > add in the comment here? > > Agreed, as you state, the CV+Lock is only necessary because you need to look at > other state (in this case num_pending_undelayed_tasks_). > > If no other state was necessary, you'd use a WaitableEvent instead. > > flush_for_testing_lock_ here is really for the benefit of > num_pending_undelayed_tasks_ and not for flush_for_testing_cv_. > > So instead I'd update the comment to say that this lock is associated with > |flush_for_testing_cv| and partially-synchronizes access to > num_pending_undelayed_tasks_. Full synchronization isn't needed because it's > atomic, but we need synchronization to coordinate waking and sleeping at the > right time. Done. https://codereview.chromium.org/2362253002/diff/60001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_unittest.cc (right): https://codereview.chromium.org/2362253002/diff/60001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_unittest.cc:361: EXPECT_TRUE(task_tracker_); On 2016/09/23 20:51:41, danakj wrote: > This is a case where I'd suggest ASSERT_TRUE because we're going to crash if > this fails. Sadly, we can't use ASSERT_TRUE in a constructor. error C2534: 'base::internal::`anonymous-namespace'::`anonymous-namespace'::ControllableDetachDelegate': constructor cannot return a value note: see declaration of 'base::internal::`anonymous-namespace'::`anonymous-namespace'::ControllableDetachDelegate' https://codereview.chromium.org/2362253002/diff/60001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler_impl.cc (right): https://codereview.chromium.org/2362253002/diff/60001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl.cc:61: task_tracker_.FlushForTesting(); On 2016/09/23 20:51:41, danakj wrote: > This is production code calling a ForTesting method. You need to not use > ForTesting in the name of the task tracker's method or you're leaving a > presubmit bomb for everyone else who touches this file. Done. https://codereview.chromium.org/2362253002/diff/60001/base/task_scheduler/tas... File base/task_scheduler/task_tracker.h (right): https://codereview.chromium.org/2362253002/diff/60001/base/task_scheduler/tas... base/task_scheduler/task_tracker.h:109: subtle::Atomic32 num_pending_undelayed_tasks_ = 0; On 2016/09/23 20:51:41, danakj wrote: > I think it'd be nice to document that you intend this to be used without > barriers and explain why it is safe here. Done. https://codereview.chromium.org/2362253002/diff/60001/base/task_scheduler/tas... File base/task_scheduler/task_tracker_unittest.cc (right): https://codereview.chromium.org/2362253002/diff/60001/base/task_scheduler/tas... base/task_scheduler/task_tracker_unittest.cc:49: // Returns true once the async call to Shutdown() has returned. On 2016/09/23 20:51:41, danakj wrote: > Comment needs updating Done.
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robliao@chromium.org, gab@chromium.org, danakj@chromium.org Link to the patchset: https://codereview.chromium.org/2362253002/#ps120001 (title: "CR robliao #13")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robliao@chromium.org, danakj@chromium.org, gab@chromium.org Link to the patchset: https://codereview.chromium.org/2362253002/#ps140001 (title: "fix build error")
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
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by fdoray@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== TaskScheduler: Add FlushForTesting(). Many browser tests depend on being able to wait until there are no pending undelayed tasks in the blocking pool (via content::RunAllBlockingPoolTasksUntilIdle). This CL brings this functionality to TaskScheduler. BUG=622400, 553459 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_tsan_rel_ng ========== to ========== TaskScheduler: Add FlushForTesting(). Many browser tests depend on being able to wait until there are no pending undelayed tasks in the blocking pool (via content::RunAllBlockingPoolTasksUntilIdle). This CL brings this functionality to TaskScheduler. BUG=622400, 553459 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_tsan_rel_ng ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== TaskScheduler: Add FlushForTesting(). Many browser tests depend on being able to wait until there are no pending undelayed tasks in the blocking pool (via content::RunAllBlockingPoolTasksUntilIdle). This CL brings this functionality to TaskScheduler. BUG=622400, 553459 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_tsan_rel_ng ========== to ========== TaskScheduler: Add FlushForTesting(). Many browser tests depend on being able to wait until there are no pending undelayed tasks in the blocking pool (via content::RunAllBlockingPoolTasksUntilIdle). This CL brings this functionality to TaskScheduler. BUG=622400, 553459 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_tsan_rel_ng Committed: https://crrev.com/fe309b0183048023246ccbefaf0cf4df0f05e29b Cr-Commit-Position: refs/heads/master@{#420901} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/fe309b0183048023246ccbefaf0cf4df0f05e29b Cr-Commit-Position: refs/heads/master@{#420901} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
