Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2808273003:
[scheduler] Move some task types to suspendable task runner. (Closed)
Description[scheduler] Move some task types to suspendable task runner.
Move several safe-looking task types to suspendable task runner.
More task types will be moved to suspendable task runner, but
we'll start with this set and will monitor any problems
(there were some perfbot failures when all task queues were
moved to suspendable task runner).
R=haraken@chromium.org
CC=skyostil@chromium.org
BUG=702160
Review-Url: https://codereview.chromium.org/2808273003
Cr-Original-Commit-Position: refs/heads/master@{#464025}
Committed: https://chromium.googlesource.com/chromium/src/+/78a049660074003b8e1e602b0e7783ca7f1f4ee0
Review-Url: https://codereview.chromium.org/2808273003
Cr-Commit-Position: refs/heads/master@{#467633}
Committed: https://chromium.googlesource.com/chromium/src/+/c18764a33a595baab6ba35a1447e51c7c74b6e30
Patch Set 1 #
Total comments: 1
Messages
Total messages: 28 (14 generated)
The CQ bit was checked by altimin@chromium.org to run a CQ dry run
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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I'm basically fine with this CL but would you help me understand why you want to move tasks to the suspendable task runners? The suspend/resume is not a common event. The suspend/resume happens only when users set a breakpoint in DevTools, open a print page etc. I'm not sure why you need to try hard to stop the tasks in such a rare event. Note that the suspend/resume is about SuspendableObject::suspend/resume. It's totally a different thing from the tab suspension.
On 2017/04/12 04:30:23, haraken wrote: > I'm basically fine with this CL but would you help me understand why you want to > move tasks to the suspendable task runners? > > The suspend/resume is not a common event. The suspend/resume happens only when > users set a breakpoint in DevTools, open a print page etc. I'm not sure why you > need to try hard to stop the tasks in such a rare event. > > Note that the suspend/resume is about SuspendableObject::suspend/resume. It's > totally a different thing from the tab suspension. We had a problem with print dialog sliently cancelling websql callbacks (crbug.com/700792) and we decided to make almost all tasks suspendable (unless we have a very good reason not to) to prevent similar problems in the future. Also I was hoping to be able to delay suspended tasks for a short periods of time (crbug.com/704939) to fix scrolling latency regression. And in the long term I was hoping to unify both types of suspension.
On 2017/04/12 10:36:33, altimin wrote: > On 2017/04/12 04:30:23, haraken wrote: > > I'm basically fine with this CL but would you help me understand why you want > to > > move tasks to the suspendable task runners? > > > > The suspend/resume is not a common event. The suspend/resume happens only when > > users set a breakpoint in DevTools, open a print page etc. I'm not sure why > you > > need to try hard to stop the tasks in such a rare event. > > > > Note that the suspend/resume is about SuspendableObject::suspend/resume. It's > > totally a different thing from the tab suspension. > > We had a problem with print dialog sliently cancelling websql callbacks > (crbug.com/700792) and we decided to make almost all tasks suspendable (unless > we have a very good reason not to) to prevent similar problems in the future. > > Also I was hoping to be able to delay suspended tasks for a short periods of > time (crbug.com/704939) to fix scrolling latency regression. > > And in the long term I was hoping to unify both types of suspension. Makes sense. I understand the websql problem but it's not clear to me which is safer -- moving most tasks to the suspendable task runner or moving most tasks to the non-suspendable task runner. (Intuitively I thought that it would be safer to move most tasks to the non-suspendable task runner because running tasks would be safer than suspending tasks. We won't need to worry about the performance impact because the suspension is a rare event.) LGTM. I believe you will have a better sense.
The CQ bit was checked by altimin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1492009039771160, "parent_rev":
"5bc8d44f3ab226674d0dd2c52ef631d6d52dc464", "commit_rev":
"78a049660074003b8e1e602b0e7783ca7f1f4ee0"}
Message was sent while issue was closed.
Description was changed from ========== [scheduler] Move some task types to suspendable task runner. Move several safe-looking task types to suspendable task runner. More task types will be moved to suspendable task runner, but we'll start with this set and will monitor any problems (there were some perfbot failures when all task queues were moved to suspendable task runner). R=haraken@chromium.org CC=skyostil@chromium.org BUG=702160 ========== to ========== [scheduler] Move some task types to suspendable task runner. Move several safe-looking task types to suspendable task runner. More task types will be moved to suspendable task runner, but we'll start with this set and will monitor any problems (there were some perfbot failures when all task queues were moved to suspendable task runner). R=haraken@chromium.org CC=skyostil@chromium.org BUG=702160 Review-Url: https://codereview.chromium.org/2808273003 Cr-Commit-Position: refs/heads/master@{#464025} Committed: https://chromium.googlesource.com/chromium/src/+/78a049660074003b8e1e602b0e77... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/78a049660074003b8e1e602b0e77...
Message was sent while issue was closed.
skyostil@chromium.org changed reviewers: + skyostil@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2808273003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp (right): https://codereview.chromium.org/2808273003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp:28: // TODO(nhiroki): Throttle them again after we're convinced that it's safe Does this comment need updating by the way?
Message was sent while issue was closed.
On 2017/04/12 14:18:32, haraken wrote: > On 2017/04/12 10:36:33, altimin wrote: > > On 2017/04/12 04:30:23, haraken wrote: > > > I'm basically fine with this CL but would you help me understand why you > want > > to > > > move tasks to the suspendable task runners? > > > > > > The suspend/resume is not a common event. The suspend/resume happens only > when > > > users set a breakpoint in DevTools, open a print page etc. I'm not sure why > > you > > > need to try hard to stop the tasks in such a rare event. > > > > > > Note that the suspend/resume is about SuspendableObject::suspend/resume. > It's > > > totally a different thing from the tab suspension. > > > > We had a problem with print dialog sliently cancelling websql callbacks > > (crbug.com/700792) and we decided to make almost all tasks suspendable (unless > > we have a very good reason not to) to prevent similar problems in the future. > > > > Also I was hoping to be able to delay suspended tasks for a short periods of > > time (crbug.com/704939) to fix scrolling latency regression. > > > > And in the long term I was hoping to unify both types of suspension. > > Makes sense. I understand the websql problem but it's not clear to me which is > safer -- moving most tasks to the suspendable task runner or moving most tasks > to the non-suspendable task runner. (Intuitively I thought that it would be > safer to move most tasks to the non-suspendable task runner because running > tasks would be safer than suspending tasks. We won't need to worry about the > performance impact because the suspension is a rare event.) > > LGTM. I believe you will have a better sense. I thought about this a bit more and begin to think that it will be more correct to move more tasks to the suspended task runners. From the perspective of correctness, almost everything should stop when the document is suspended. i.e., I think this CL is going a right way (although I'm not sure how safe it will be :-).
Message was sent while issue was closed.
A revert of this CL (patchset #1 id:1) has been created in https://codereview.chromium.org/2829643002/ by hablich@chromium.org. The reason for reverting is: Reverting as suggested in BUG=711196.
Message was sent while issue was closed.
Description was changed from ========== [scheduler] Move some task types to suspendable task runner. Move several safe-looking task types to suspendable task runner. More task types will be moved to suspendable task runner, but we'll start with this set and will monitor any problems (there were some perfbot failures when all task queues were moved to suspendable task runner). R=haraken@chromium.org CC=skyostil@chromium.org BUG=702160 Review-Url: https://codereview.chromium.org/2808273003 Cr-Commit-Position: refs/heads/master@{#464025} Committed: https://chromium.googlesource.com/chromium/src/+/78a049660074003b8e1e602b0e77... ========== to ========== [scheduler] Move some task types to suspendable task runner. Move several safe-looking task types to suspendable task runner. More task types will be moved to suspendable task runner, but we'll start with this set and will monitor any problems (there were some perfbot failures when all task queues were moved to suspendable task runner). R=haraken@chromium.org CC=skyostil@chromium.org BUG=702160 Review-Url: https://codereview.chromium.org/2808273003 Cr-Commit-Position: refs/heads/master@{#464025} Committed: https://chromium.googlesource.com/chromium/src/+/78a049660074003b8e1e602b0e77... ==========
The CQ bit was checked by altimin@chromium.org
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 altimin@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1493285782942160, "parent_rev":
"b4046080f31d65ed2618ecf1a2538ac322e9848a", "commit_rev":
"c18764a33a595baab6ba35a1447e51c7c74b6e30"}
Message was sent while issue was closed.
Description was changed from ========== [scheduler] Move some task types to suspendable task runner. Move several safe-looking task types to suspendable task runner. More task types will be moved to suspendable task runner, but we'll start with this set and will monitor any problems (there were some perfbot failures when all task queues were moved to suspendable task runner). R=haraken@chromium.org CC=skyostil@chromium.org BUG=702160 Review-Url: https://codereview.chromium.org/2808273003 Cr-Commit-Position: refs/heads/master@{#464025} Committed: https://chromium.googlesource.com/chromium/src/+/78a049660074003b8e1e602b0e77... ========== to ========== [scheduler] Move some task types to suspendable task runner. Move several safe-looking task types to suspendable task runner. More task types will be moved to suspendable task runner, but we'll start with this set and will monitor any problems (there were some perfbot failures when all task queues were moved to suspendable task runner). R=haraken@chromium.org CC=skyostil@chromium.org BUG=702160 Review-Url: https://codereview.chromium.org/2808273003 Cr-Original-Commit-Position: refs/heads/master@{#464025} Committed: https://chromium.googlesource.com/chromium/src/+/78a049660074003b8e1e602b0e77... Review-Url: https://codereview.chromium.org/2808273003 Cr-Commit-Position: refs/heads/master@{#467633} Committed: https://chromium.googlesource.com/chromium/src/+/c18764a33a595baab6ba35a1447e... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/c18764a33a595baab6ba35a1447e...
Message was sent while issue was closed.
A revert of this CL (patchset #1 id:1) has been created in https://codereview.chromium.org/2850143003/ by rnephew@chromium.org. The reason for reverting is: Causing telemetry test failures: https://bugs.chromium.org/p/chromium/issues/detail?id=717078&can=2&start=0&nu.... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
