Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2572893002:
[Reland] Dont post delayed DoWork for disabled queues. (Closed)
|
Created:
4 years ago by alex clarke (OOO till 29th) Modified:
3 years, 10 months ago CC:
chromium-reviews, blink-reviews, scheduler-bugs_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDont post delayed DoWork for disabled queues. We could do this for
fences too but don't because that would break the throttling logic
which needs to know when the next delayed task is due to be run.
Contains a fix for the bug which previously was causing renderer hangs.
TaskQueue::GetNextScheduledWakeUp must return base::nullopt for
disabled queues because we no longer schedule wakeups for those. If we
don't do this then the TaskQueueThrottler will be fed misleading data
which causes it to misbehave.
BUG=671669, 688422, 693672
Committed: https://crrev.com/cab9842ac55e5b9dab766f11c6e412949d854483
Cr-Original-Original-Original-Commit-Position: refs/heads/master@{#438962}
Review-Url: https://codereview.chromium.org/2572893002
Cr-Original-Original-Commit-Position: refs/heads/master@{#447591}
Committed: https://chromium.googlesource.com/chromium/src/+/9a8a61516e750796f74869167bb919a05551d4dd
Review-Url: https://codereview.chromium.org/2572893002
Cr-Original-Commit-Position: refs/heads/master@{#450679}
Committed: https://chromium.googlesource.com/chromium/src/+/c4b071b9088980c941d3dea85478b10675a479e6
Review-Url: https://codereview.chromium.org/2572893002
Cr-Commit-Position: refs/heads/master@{#451715}
Committed: https://chromium.googlesource.com/chromium/src/+/2f16bd68bd7e191db66f96d2c761017902074d91
Patch Set 1 #Patch Set 2 : Fix perftest #
Total comments: 8
Patch Set 3 : Fix LazyNow check #Patch Set 4 : Fix nits and add a test #Patch Set 5 : Rebased #Patch Set 6 : Rebased #
Total comments: 2
Patch Set 7 : Keep track of which TimeDomain the delayed do works are associated with. #
Total comments: 8
Patch Set 8 : Address Sami's comments #Patch Set 9 : Fix Test #Patch Set 10 : Don't need LazyNow #Patch Set 11 : Fix renderer hangs #Patch Set 12 : TimeDomain::CancelDelayedWork shouldn't send a OnTimeDomainHasDelayedWork notification #
Total comments: 5
Patch Set 13 #Patch Set 14 : Rebase #Patch Set 15 : Fix bug with TaskQueueImpl::SetTimeDomain and disabled queue. #Patch Set 16 : Fix compile #Messages
Total messages: 128 (91 generated)
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Description was changed from ========== Dont post delayed DoWork for disabled queues. BUG=671669 ========== to ========== Dont post delayed DoWork for disabled queues. BUG=671669 ==========
alexclarke@chromium.org changed reviewers: + skyostil@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm, nice optimization! https://codereview.chromium.org/2572893002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc (right): https://codereview.chromium.org/2572893002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:221: LazyNow* lazy_now, Not something for this patch, but I'm feeling a little nervous about all the LazyNows we're passing around. It feels like it's too easy to accidentally keep a LazyNow around for too long (say, around task observers) and end up with time skew as a result. I wonder if we should add some DCHECKs or another mechanism to guard against that? https://codereview.chromium.org/2572893002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/virtual_time_domain.cc (right): https://codereview.chromium.org/2572893002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/virtual_time_domain.cc:44: void VirtualTimeDomain::CancelWakeupAt(base::TimeTicks run_time) {} // No-op because of the above. ? https://codereview.chromium.org/2572893002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/auto_advancing_virtual_time_domain.cc (right): https://codereview.chromium.org/2572893002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/auto_advancing_virtual_time_domain.cc:35: void AutoAdvancingVirtualTimeDomain::CancelWakeupAt(base::TimeTicks run_time) {} I guess in theory we could cancel the DoWork but it's probably not worth the complexity, right? Maybe add a short comment? https://codereview.chromium.org/2572893002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/throttled_time_domain.cc (right): https://codereview.chromium.org/2572893002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/throttled_time_domain.cc:26: void ThrottledTimeDomain::CancelWakeupAt(base::TimeTicks run_time) {} // No-op because of the above.
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by alexclarke@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...
alexclarke@chromium.org changed reviewers: + haraken@chromium.org
haraken@chromium.org: Please review changes in TestingPlatformSupport.cpp https://codereview.chromium.org/2572893002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc (right): https://codereview.chromium.org/2572893002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:221: LazyNow* lazy_now, On 2016/12/14 16:29:38, Sami wrote: > Not something for this patch, but I'm feeling a little nervous about all the > LazyNows we're passing around. It feels like it's too easy to accidentally keep > a LazyNow around for too long (say, around task observers) and end up with time > skew as a result. I wonder if we should add some DCHECKs or another mechanism to > guard against that? Agreed it would be easy for that to happen. I'll have to think if there's something we can do. https://codereview.chromium.org/2572893002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/virtual_time_domain.cc (right): https://codereview.chromium.org/2572893002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/virtual_time_domain.cc:44: void VirtualTimeDomain::CancelWakeupAt(base::TimeTicks run_time) {} On 2016/12/14 16:29:38, Sami wrote: > // No-op because of the above. > > ? Done. https://codereview.chromium.org/2572893002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/auto_advancing_virtual_time_domain.cc (right): https://codereview.chromium.org/2572893002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/auto_advancing_virtual_time_domain.cc:35: void AutoAdvancingVirtualTimeDomain::CancelWakeupAt(base::TimeTicks run_time) {} On 2016/12/14 16:29:38, Sami wrote: > I guess in theory we could cancel the DoWork but it's probably not worth the > complexity, right? Maybe add a short comment? We don't need to do anything, I added a comment. https://codereview.chromium.org/2572893002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/throttled_time_domain.cc (right): https://codereview.chromium.org/2572893002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/throttled_time_domain.cc:26: void ThrottledTimeDomain::CancelWakeupAt(base::TimeTicks run_time) {} On 2016/12/14 16:29:38, Sami wrote: > // No-op because of the above. Done.
Description was changed from ========== Dont post delayed DoWork for disabled queues. BUG=671669 ========== to ========== Dont post delayed DoWork for disabled queues. We could do this for fences too but don't because that would break the throttling logic which needs to know when the next delayed task is due to be run. BUG=671669 ==========
Description was changed from ========== Dont post delayed DoWork for disabled queues. We could do this for fences too but don't because that would break the throttling logic which needs to know when the next delayed task is due to be run. BUG=671669 ========== to ========== Dont post delayed DoWork for disabled queues. We could do this for fences too but don't because that would break the throttling logic which needs to know when the next delayed task is due to be run. BUG=671669 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM
The CQ bit was checked by alexclarke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/2572893002/#ps60001 (title: "Fix nits and add a test")
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
Failed to apply patch for
third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:
While running git apply --index -p1;
error: patch failed:
third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:236
error:
third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc: patch
does not apply
Patch:
third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc
Index: third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc
diff --git
a/third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc
b/third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc
index
252f086813978c0c5c6185eb774e18df87e44f1f..c9c273b8c4f829bb027fff0fe31f2e09bbf49823
100644
--- a/third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc
+++ b/third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc
@@ -218,10 +218,9 @@ void TaskQueueManager::MaybeScheduleImmediateWorkLocked(
void TaskQueueManager::MaybeScheduleDelayedWork(
const tracked_objects::Location& from_here,
- base::TimeTicks now,
- base::TimeDelta delay) {
+ LazyNow* lazy_now,
+ base::TimeTicks run_time) {
DCHECK(main_thread_checker_.CalledOnValidThread());
- DCHECK_GE(delay, base::TimeDelta());
{
base::AutoLock lock(any_thread_lock_);
@@ -236,19 +235,31 @@ void TaskQueueManager::MaybeScheduleDelayedWork(
}
// De-duplicate DoWork posts.
- base::TimeTicks run_time = now + delay;
if (next_delayed_do_work_ <= run_time && !next_delayed_do_work_.is_null())
return;
+ cancelable_delayed_do_work_closure_.Reset(delayed_do_work_closure_);
+
+ base::TimeDelta delay =
+ std::max(base::TimeDelta(), run_time - lazy_now->Now());
+ next_delayed_do_work_ = lazy_now->Now() + delay;
+
TRACE_EVENT1(tracing_category_, "MaybeScheduleDelayedWorkInternal",
"delay_ms", delay.InMillisecondsF());
- cancelable_delayed_do_work_closure_.Reset(delayed_do_work_closure_);
- next_delayed_do_work_ = run_time;
delegate_->PostDelayedTask(
from_here, cancelable_delayed_do_work_closure_.callback(), delay);
}
+void TaskQueueManager::CancelDelayedWork(base::TimeTicks run_time) {
+ DCHECK(main_thread_checker_.CalledOnValidThread());
+ if (next_delayed_do_work_ != run_time)
+ return;
+
+ cancelable_delayed_do_work_closure_.Cancel();
+ next_delayed_do_work_ = base::TimeTicks();
+}
+
void TaskQueueManager::DoWork(bool delayed) {
DCHECK(main_thread_checker_.CalledOnValidThread());
TRACE_EVENT1("tracing_category_", "TaskQueueManager::DoWork", "delayed",
The CQ bit was checked by alexclarke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/2572893002/#ps80001 (title: "Rebased")
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": 80001, "attempt_start_ts": 1481835228712150,
"parent_rev": "2948656d60f5c6bbf7d50cfb7a799a22a1308915", "commit_rev":
"ab610f727399268e06d4533fa5822fcc8e9c2bb8"}
Message was sent while issue was closed.
Description was changed from ========== Dont post delayed DoWork for disabled queues. We could do this for fences too but don't because that would break the throttling logic which needs to know when the next delayed task is due to be run. BUG=671669 ========== to ========== Dont post delayed DoWork for disabled queues. We could do this for fences too but don't because that would break the throttling logic which needs to know when the next delayed task is due to be run. BUG=671669 Review-Url: https://codereview.chromium.org/2572893002 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Dont post delayed DoWork for disabled queues. We could do this for fences too but don't because that would break the throttling logic which needs to know when the next delayed task is due to be run. BUG=671669 Review-Url: https://codereview.chromium.org/2572893002 ========== to ========== Dont post delayed DoWork for disabled queues. We could do this for fences too but don't because that would break the throttling logic which needs to know when the next delayed task is due to be run. BUG=671669 Committed: https://crrev.com/cab9842ac55e5b9dab766f11c6e412949d854483 Cr-Commit-Position: refs/heads/master@{#438962} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/cab9842ac55e5b9dab766f11c6e412949d854483 Cr-Commit-Position: refs/heads/master@{#438962}
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/2583333002/ by gab@chromium.org. The reason for reverting is: Need to revert this to be able to revert https://codereview.chromium.org/2546423002 which is the #1 culprit for http://crbug.com/674895..
Message was sent while issue was closed.
Description was changed from ========== Dont post delayed DoWork for disabled queues. We could do this for fences too but don't because that would break the throttling logic which needs to know when the next delayed task is due to be run. BUG=671669 Committed: https://crrev.com/cab9842ac55e5b9dab766f11c6e412949d854483 Cr-Commit-Position: refs/heads/master@{#438962} ========== to ========== Dont post delayed DoWork for disabled queues. We could do this for fences too but don't because that would break the throttling logic which needs to know when the next delayed task is due to be run. BUG=671669 Committed: https://crrev.com/cab9842ac55e5b9dab766f11c6e412949d854483 Cr-Commit-Position: refs/heads/master@{#438962} ==========
The CQ bit was checked by alexclarke@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...
Sami PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2572893002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.h (right): https://codereview.chromium.org/2572893002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.h:82: void CancelDelayedWork(base::TimeTicks run_time); What if one time domain accidentally cancels the wakeup requested by another one? Do we need some refcounting here?
PTAL https://codereview.chromium.org/2572893002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.h (right): https://codereview.chromium.org/2572893002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.h:82: void CancelDelayedWork(base::TimeTicks run_time); On 2017/01/27 12:36:25, Sami wrote: > What if one time domain accidentally cancels the wakeup requested by another > one? Do we need some refcounting here? As discussed offline I've added some code to track which time domain is associated with the wakeup.
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with a few comments. https://codereview.chromium.org/2572893002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc (right): https://codereview.chromium.org/2572893002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:236: DCHECK(main_thread_checker_.CalledOnValidThread()); DCHECK(!next_delayed_do_work_ || next_delayed_do_work_.time_domain() == requesting_time_domain); https://codereview.chromium.org/2572893002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:292: // |next_delayed_do_work_|. nit: rewrap https://codereview.chromium.org/2572893002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:295: ; Extra ';'? https://codereview.chromium.org/2572893002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.h (right): https://codereview.chromium.org/2572893002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.h:160: class NextTaskDelay { TaskQueueManagerTest is a friend -- could these two be private instead?
Thanks! https://codereview.chromium.org/2572893002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc (right): https://codereview.chromium.org/2572893002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:236: DCHECK(main_thread_checker_.CalledOnValidThread()); On 2017/02/01 07:30:05, Sami (slow -- travelling) wrote: > DCHECK(!next_delayed_do_work_ || next_delayed_do_work_.time_domain() == > requesting_time_domain); Done. https://codereview.chromium.org/2572893002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:292: // |next_delayed_do_work_|. On 2017/02/01 07:30:04, Sami (slow -- travelling) wrote: > nit: rewrap Done. https://codereview.chromium.org/2572893002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:295: ; On 2017/02/01 07:30:04, Sami (slow -- travelling) wrote: > Extra ';'? Done. https://codereview.chromium.org/2572893002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.h (right): https://codereview.chromium.org/2572893002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.h:160: class NextTaskDelay { On 2017/02/01 07:30:05, Sami (slow -- travelling) wrote: > TaskQueueManagerTest is a friend -- could these two be private instead? I found a way for NextDelayedDoWork to be private and NextTaskDelay to be protected.
The CQ bit was checked by alexclarke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/2572893002/#ps140001 (title: "Address ami's comments")
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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by alexclarke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/2572893002/#ps160001 (title: "Fix Test")
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": 160001, "attempt_start_ts": 1485971416663190,
"parent_rev": "12a5a0b5a474a24c6943aef3686d91199a2e2b58", "commit_rev":
"9a8a61516e750796f74869167bb919a05551d4dd"}
Message was sent while issue was closed.
Description was changed from ========== Dont post delayed DoWork for disabled queues. We could do this for fences too but don't because that would break the throttling logic which needs to know when the next delayed task is due to be run. BUG=671669 Committed: https://crrev.com/cab9842ac55e5b9dab766f11c6e412949d854483 Cr-Commit-Position: refs/heads/master@{#438962} ========== to ========== Dont post delayed DoWork for disabled queues. We could do this for fences too but don't because that would break the throttling logic which needs to know when the next delayed task is due to be run. BUG=671669 Committed: https://crrev.com/cab9842ac55e5b9dab766f11c6e412949d854483 Cr-Original-Commit-Position: refs/heads/master@{#438962} Review-Url: https://codereview.chromium.org/2572893002 Cr-Commit-Position: refs/heads/master@{#447591} Committed: https://chromium.googlesource.com/chromium/src/+/9a8a61516e750796f74869167bb9... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/9a8a61516e750796f74869167bb9...
Message was sent while issue was closed.
A revert of this CL (patchset #9 id:160001) has been created in https://codereview.chromium.org/2674903003/ by alexclarke@chromium.org. The reason for reverting is: Speculative revert. Might be causing renderer hangs..
Message was sent while issue was closed.
Description was changed from ========== Dont post delayed DoWork for disabled queues. We could do this for fences too but don't because that would break the throttling logic which needs to know when the next delayed task is due to be run. BUG=671669 Committed: https://crrev.com/cab9842ac55e5b9dab766f11c6e412949d854483 Cr-Original-Commit-Position: refs/heads/master@{#438962} Review-Url: https://codereview.chromium.org/2572893002 Cr-Commit-Position: refs/heads/master@{#447591} Committed: https://chromium.googlesource.com/chromium/src/+/9a8a61516e750796f74869167bb9... ========== to ========== Dont post delayed DoWork for disabled queues. We could do this for fences too but don't because that would break the throttling logic which needs to know when the next delayed task is due to be run. BUG=671669 Committed: https://crrev.com/cab9842ac55e5b9dab766f11c6e412949d854483 Cr-Original-Commit-Position: refs/heads/master@{#438962} Review-Url: https://codereview.chromium.org/2572893002 Cr-Commit-Position: refs/heads/master@{#447591} Committed: https://chromium.googlesource.com/chromium/src/+/9a8a61516e750796f74869167bb9... ==========
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Dont post delayed DoWork for disabled queues. We could do this for fences too but don't because that would break the throttling logic which needs to know when the next delayed task is due to be run. BUG=671669 Committed: https://crrev.com/cab9842ac55e5b9dab766f11c6e412949d854483 Cr-Original-Commit-Position: refs/heads/master@{#438962} Review-Url: https://codereview.chromium.org/2572893002 Cr-Commit-Position: refs/heads/master@{#447591} Committed: https://chromium.googlesource.com/chromium/src/+/9a8a61516e750796f74869167bb9... ========== to ========== Dont post delayed DoWork for disabled queues. We could do this for fences too but don't because that would break the throttling logic which needs to know when the next delayed task is due to be run. Contains a fix for the bug which previously was causing renderer hangs. TaskQueue::GetNextScheduledWakeUp must return base::nullopt for disabled queues, and we sometimes needed to call TimeDomain::OnTimeDomainHasDelayedWork in TimeDomain::CancelDelayedWork or the TaskQueueThrottler would misbehave due to being given bad input. BUG=671669 Committed: https://crrev.com/cab9842ac55e5b9dab766f11c6e412949d854483 Cr-Original-Commit-Position: refs/heads/master@{#438962} Review-Url: https://codereview.chromium.org/2572893002 Cr-Commit-Position: refs/heads/master@{#447591} Committed: https://chromium.googlesource.com/chromium/src/+/9a8a61516e750796f74869167bb9... ==========
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
I believe I've fixed the renderer hangs. PTAL.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by alexclarke@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
skyostil@chromium.org changed reviewers: + altimin@chromium.org
Thanks for the fix! Could you expand a bit in the patch description *why* GetNextScheduledWakeUp must return nullopt? Also, the second part of the description seems to contradict the title of the latest patch set and also could be a little more explicit about what the actual problem was. (Note sure if it's feasible but the fix could be landed separately since it seems to be mostly orthogonal to the actual change here.) https://codereview.chromium.org/2572893002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc (right): https://codereview.chromium.org/2572893002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc:410: if (main_thread_only().delayed_incoming_queue.empty() || !IsQueueEnabled()) Should we do this at call site to avoid making this method too magic? I'm wondering what other methods should be changed to check for disabled queues if we go down this road. https://codereview.chromium.org/2572893002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager_unittest.cc (right): https://codereview.chromium.org/2572893002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager_unittest.cc:1773: TEST_F(TaskQueueManagerTest, TimeDomainObserver_DelayedTaskMultpleQueues) { typo: multiple
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
Description was changed from ========== Dont post delayed DoWork for disabled queues. We could do this for fences too but don't because that would break the throttling logic which needs to know when the next delayed task is due to be run. Contains a fix for the bug which previously was causing renderer hangs. TaskQueue::GetNextScheduledWakeUp must return base::nullopt for disabled queues, and we sometimes needed to call TimeDomain::OnTimeDomainHasDelayedWork in TimeDomain::CancelDelayedWork or the TaskQueueThrottler would misbehave due to being given bad input. BUG=671669 Committed: https://crrev.com/cab9842ac55e5b9dab766f11c6e412949d854483 Cr-Original-Commit-Position: refs/heads/master@{#438962} Review-Url: https://codereview.chromium.org/2572893002 Cr-Commit-Position: refs/heads/master@{#447591} Committed: https://chromium.googlesource.com/chromium/src/+/9a8a61516e750796f74869167bb9... ========== to ========== Dont post delayed DoWork for disabled queues. We could do this for fences too but don't because that would break the throttling logic which needs to know when the next delayed task is due to be run. Contains a fix for the bug which previously was causing renderer hangs. TaskQueue::GetNextScheduledWakeUp must return base::nullopt for disabled queues because we no longer schedule wakeups for those. If we don't do this then the TaskQueueThrottler will be fed misleading data which causes it to misbehave. BUG=671669 Committed: https://crrev.com/cab9842ac55e5b9dab766f11c6e412949d854483 Cr-Original-Commit-Position: refs/heads/master@{#438962} Review-Url: https://codereview.chromium.org/2572893002 Cr-Commit-Position: refs/heads/master@{#447591} Committed: https://chromium.googlesource.com/chromium/src/+/9a8a61516e750796f74869167bb9... ==========
Is it a good idea to add comment to NextScheduledWakeUp method instead of a patch description. https://codereview.chromium.org/2572893002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc (right): https://codereview.chromium.org/2572893002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc:410: if (main_thread_only().delayed_incoming_queue.empty() || !IsQueueEnabled()) On 2017/02/10 12:49:43, Sami wrote: > Should we do this at call site to avoid making this method too magic? I'm > wondering what other methods should be changed to check for disabled queues if > we go down this road. I'm, for one, in favour of this approach: if the queue is disabled, we do not have a next scheduled wake up. This is also should be a part of this change because it is this patch which changes logic behind scheduling wakeups.
PTAL https://codereview.chromium.org/2572893002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc (right): https://codereview.chromium.org/2572893002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc:410: if (main_thread_only().delayed_incoming_queue.empty() || !IsQueueEnabled()) On 2017/02/10 12:49:43, Sami wrote: > Should we do this at call site to avoid making this method too magic? As discussed offline, this change is making the function do what it's supposed to do. I.e. tell us when the next schedule wake up for this queue is. After this patch disabled queues don't have any wake-ups scheduled. I'm > wondering what other methods should be changed to check for disabled queues if > we go down this road. No other methods need to change. https://codereview.chromium.org/2572893002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager_unittest.cc (right): https://codereview.chromium.org/2572893002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager_unittest.cc:1773: TEST_F(TaskQueueManagerTest, TimeDomainObserver_DelayedTaskMultpleQueues) { On 2017/02/10 12:49:43, Sami wrote: > typo: multiple Done.
The CQ bit was checked by alexclarke@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
lgtm.
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 alexclarke@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 alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
The try job for v8.mobile_infinite_scroll-turbo_tbmv2 passed, so hopefully it's not safe to re-land this patch without re-occurrence of https://bugs.chromium.org/p/chromium/issues/detail?id=688422
Description was changed from ========== Dont post delayed DoWork for disabled queues. We could do this for fences too but don't because that would break the throttling logic which needs to know when the next delayed task is due to be run. Contains a fix for the bug which previously was causing renderer hangs. TaskQueue::GetNextScheduledWakeUp must return base::nullopt for disabled queues because we no longer schedule wakeups for those. If we don't do this then the TaskQueueThrottler will be fed misleading data which causes it to misbehave. BUG=671669 Committed: https://crrev.com/cab9842ac55e5b9dab766f11c6e412949d854483 Cr-Original-Commit-Position: refs/heads/master@{#438962} Review-Url: https://codereview.chromium.org/2572893002 Cr-Commit-Position: refs/heads/master@{#447591} Committed: https://chromium.googlesource.com/chromium/src/+/9a8a61516e750796f74869167bb9... ========== to ========== Dont post delayed DoWork for disabled queues. We could do this for fences too but don't because that would break the throttling logic which needs to know when the next delayed task is due to be run. Contains a fix for the bug which previously was causing renderer hangs. TaskQueue::GetNextScheduledWakeUp must return base::nullopt for disabled queues because we no longer schedule wakeups for those. If we don't do this then the TaskQueueThrottler will be fed misleading data which causes it to misbehave. BUG=671669, 688422 Committed: https://crrev.com/cab9842ac55e5b9dab766f11c6e412949d854483 Cr-Original-Commit-Position: refs/heads/master@{#438962} Review-Url: https://codereview.chromium.org/2572893002 Cr-Commit-Position: refs/heads/master@{#447591} Committed: https://chromium.googlesource.com/chromium/src/+/9a8a61516e750796f74869167bb9... ==========
The CQ bit was checked by alexclarke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, skyostil@chromium.org, altimin@chromium.org Link to the patchset: https://codereview.chromium.org/2572893002/#ps260001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/02/15 11:20:08, alex clarke wrote: > The try job for v8.mobile_infinite_scroll-turbo_tbmv2 passed, so hopefully it's > not safe to re-land this patch without re-occurrence of > > https://bugs.chromium.org/p/chromium/issues/detail?id=688422 Erm I meant to say hopefully it's now safe to re-land this patch!
On 2017/02/15 11:20:08, alex clarke wrote: > The try job for v8.mobile_infinite_scroll-turbo_tbmv2 passed, so hopefully it's > not safe to re-land this patch without re-occurrence of > > https://bugs.chromium.org/p/chromium/issues/detail?id=688422 Erm I meant to say hopefully it's now safe to re-land this patch!
CQ is committing da patch.
Bot data: {"patchset_id": 260001, "attempt_start_ts": 1487157658822800,
"parent_rev": "c6892193c349af49244bc4cd88221b82677d88f2", "commit_rev":
"c4b071b9088980c941d3dea85478b10675a479e6"}
Message was sent while issue was closed.
Description was changed from ========== Dont post delayed DoWork for disabled queues. We could do this for fences too but don't because that would break the throttling logic which needs to know when the next delayed task is due to be run. Contains a fix for the bug which previously was causing renderer hangs. TaskQueue::GetNextScheduledWakeUp must return base::nullopt for disabled queues because we no longer schedule wakeups for those. If we don't do this then the TaskQueueThrottler will be fed misleading data which causes it to misbehave. BUG=671669, 688422 Committed: https://crrev.com/cab9842ac55e5b9dab766f11c6e412949d854483 Cr-Original-Commit-Position: refs/heads/master@{#438962} Review-Url: https://codereview.chromium.org/2572893002 Cr-Commit-Position: refs/heads/master@{#447591} Committed: https://chromium.googlesource.com/chromium/src/+/9a8a61516e750796f74869167bb9... ========== to ========== Dont post delayed DoWork for disabled queues. We could do this for fences too but don't because that would break the throttling logic which needs to know when the next delayed task is due to be run. Contains a fix for the bug which previously was causing renderer hangs. TaskQueue::GetNextScheduledWakeUp must return base::nullopt for disabled queues because we no longer schedule wakeups for those. If we don't do this then the TaskQueueThrottler will be fed misleading data which causes it to misbehave. BUG=671669, 688422 Committed: https://crrev.com/cab9842ac55e5b9dab766f11c6e412949d854483 Cr-Original-Original-Commit-Position: refs/heads/master@{#438962} Review-Url: https://codereview.chromium.org/2572893002 Cr-Original-Commit-Position: refs/heads/master@{#447591} Committed: https://chromium.googlesource.com/chromium/src/+/9a8a61516e750796f74869167bb9... Review-Url: https://codereview.chromium.org/2572893002 Cr-Commit-Position: refs/heads/master@{#450679} Committed: https://chromium.googlesource.com/chromium/src/+/c4b071b9088980c941d3dea85478... ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001) as https://chromium.googlesource.com/chromium/src/+/c4b071b9088980c941d3dea85478...
Message was sent while issue was closed.
Description was changed from ========== Dont post delayed DoWork for disabled queues. We could do this for fences too but don't because that would break the throttling logic which needs to know when the next delayed task is due to be run. Contains a fix for the bug which previously was causing renderer hangs. TaskQueue::GetNextScheduledWakeUp must return base::nullopt for disabled queues because we no longer schedule wakeups for those. If we don't do this then the TaskQueueThrottler will be fed misleading data which causes it to misbehave. BUG=671669, 688422 Committed: https://crrev.com/cab9842ac55e5b9dab766f11c6e412949d854483 Cr-Original-Original-Commit-Position: refs/heads/master@{#438962} Review-Url: https://codereview.chromium.org/2572893002 Cr-Original-Commit-Position: refs/heads/master@{#447591} Committed: https://chromium.googlesource.com/chromium/src/+/9a8a61516e750796f74869167bb9... Review-Url: https://codereview.chromium.org/2572893002 Cr-Commit-Position: refs/heads/master@{#450679} Committed: https://chromium.googlesource.com/chromium/src/+/c4b071b9088980c941d3dea85478... ========== to ========== Dont post delayed DoWork for disabled queues. We could do this for fences too but don't because that would break the throttling logic which needs to know when the next delayed task is due to be run. Contains a fix for the bug which previously was causing renderer hangs. TaskQueue::GetNextScheduledWakeUp must return base::nullopt for disabled queues because we no longer schedule wakeups for those. If we don't do this then the TaskQueueThrottler will be fed misleading data which causes it to misbehave. BUG=671669, 688422, 693672 Committed: https://crrev.com/cab9842ac55e5b9dab766f11c6e412949d854483 Cr-Original-Original-Commit-Position: refs/heads/master@{#438962} Review-Url: https://codereview.chromium.org/2572893002 Cr-Original-Commit-Position: refs/heads/master@{#447591} Committed: https://chromium.googlesource.com/chromium/src/+/9a8a61516e750796f74869167bb9... Review-Url: https://codereview.chromium.org/2572893002 Cr-Commit-Position: refs/heads/master@{#450679} Committed: https://chromium.googlesource.com/chromium/src/+/c4b071b9088980c941d3dea85478... ==========
Message was sent while issue was closed.
A revert of this CL (patchset #14 id:260001) has been created in https://codereview.chromium.org/2705703002/ by alexclarke@chromium.org. The reason for reverting is: Looks like there's still a bad interaction with the TaskQueueThrottler. https://bugs.chromium.org/p/chromium/issues/detail?id=693672.
The CQ bit was checked by alexclarke@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...
On 2017/02/17 21:42:56, alex clarke wrote: > A revert of this CL (patchset #14 id:260001) has been created in > https://codereview.chromium.org/2705703002/ by mailto:alexclarke@chromium.org. > > The reason for reverting is: Looks like there's still a bad interaction with the > TaskQueueThrottler. > > https://bugs.chromium.org/p/chromium/issues/detail?id=693672. lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
SetTimeDomain fix lgtm, thanks!
The CQ bit was checked by alexclarke@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 alexclarke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, skyostil@chromium.org, altimin@chromium.org Link to the patchset: https://codereview.chromium.org/2572893002/#ps300001 (title: "Fix compile")
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": 1487666916061590,
"parent_rev": "ddb3cdad576e8414efec789a7b0d31323bf237e9", "commit_rev":
"2f16bd68bd7e191db66f96d2c761017902074d91"}
Message was sent while issue was closed.
Description was changed from ========== Dont post delayed DoWork for disabled queues. We could do this for fences too but don't because that would break the throttling logic which needs to know when the next delayed task is due to be run. Contains a fix for the bug which previously was causing renderer hangs. TaskQueue::GetNextScheduledWakeUp must return base::nullopt for disabled queues because we no longer schedule wakeups for those. If we don't do this then the TaskQueueThrottler will be fed misleading data which causes it to misbehave. BUG=671669, 688422, 693672 Committed: https://crrev.com/cab9842ac55e5b9dab766f11c6e412949d854483 Cr-Original-Original-Commit-Position: refs/heads/master@{#438962} Review-Url: https://codereview.chromium.org/2572893002 Cr-Original-Commit-Position: refs/heads/master@{#447591} Committed: https://chromium.googlesource.com/chromium/src/+/9a8a61516e750796f74869167bb9... Review-Url: https://codereview.chromium.org/2572893002 Cr-Commit-Position: refs/heads/master@{#450679} Committed: https://chromium.googlesource.com/chromium/src/+/c4b071b9088980c941d3dea85478... ========== to ========== Dont post delayed DoWork for disabled queues. We could do this for fences too but don't because that would break the throttling logic which needs to know when the next delayed task is due to be run. Contains a fix for the bug which previously was causing renderer hangs. TaskQueue::GetNextScheduledWakeUp must return base::nullopt for disabled queues because we no longer schedule wakeups for those. If we don't do this then the TaskQueueThrottler will be fed misleading data which causes it to misbehave. BUG=671669, 688422, 693672 Committed: https://crrev.com/cab9842ac55e5b9dab766f11c6e412949d854483 Cr-Original-Original-Original-Commit-Position: refs/heads/master@{#438962} Review-Url: https://codereview.chromium.org/2572893002 Cr-Original-Original-Commit-Position: refs/heads/master@{#447591} Committed: https://chromium.googlesource.com/chromium/src/+/9a8a61516e750796f74869167bb9... Review-Url: https://codereview.chromium.org/2572893002 Cr-Original-Commit-Position: refs/heads/master@{#450679} Committed: https://chromium.googlesource.com/chromium/src/+/c4b071b9088980c941d3dea85478... Review-Url: https://codereview.chromium.org/2572893002 Cr-Commit-Position: refs/heads/master@{#451715} Committed: https://chromium.googlesource.com/chromium/src/+/2f16bd68bd7e191db66f96d2c761... ==========
Message was sent while issue was closed.
Committed patchset #16 (id:300001) as https://chromium.googlesource.com/chromium/src/+/2f16bd68bd7e191db66f96d2c761... |
