Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2754673002:
[scheduler] Split suspendable tq from unthrottled tq. (Closed)
|
Created:
3 years, 9 months ago by altimin Modified:
3 years, 8 months ago CC:
chromium-reviews, sof, eae+blinkwatch, blink-reviews-dom_chromium.org, dglazkov+blink, blink-reviews, kinuko+watch, scheduler-bugs_chromium.org, rwlbuis, nhiroki, tzik Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[scheduler] Add suspendable task queue.
Add suspendable task queue to avoid running (and cancelling) callbacks
when execution context is suspended.
R=skyostil@chromium.org,alexclarke@chromium.org,haraken@chromium.org
CC=nhiroki@chromium.org,tzik@chromium.org
BUG=704939, 702160
Review-Url: https://codereview.chromium.org/2754673002
Cr-Original-Commit-Position: refs/heads/master@{#459787}
Committed: https://chromium.googlesource.com/chromium/src/+/12e16766adec5bb1e5cdd73d3cd8f5016b59ce18
Review-Url: https://codereview.chromium.org/2754673002
Cr-Commit-Position: refs/heads/master@{#462208}
Committed: https://chromium.googlesource.com/chromium/src/+/cb434223a4b462abf2b9f9123646137250b3c2d9
Patch Set 1 #
Total comments: 5
Patch Set 2 : Add test #Patch Set 3 : Unthrottled means unthrottled #
Total comments: 1
Patch Set 4 : Address nit #Patch Set 5 : For the moment make only database access tasks suspendable #
Messages
Total messages: 39 (23 generated)
PTAL
Description was changed from ========== [scheduler] Add suspendable task queue. Add suspendable task queue to avoid running (and cancelling) callbacks when execution context is suspended. R=skyostil@chromium.org,alexclarke@chromium.org,haraken@chromium.org CC=nhiroki@chromium.org,tzik@chromium.org BUG=700792 ========== to ========== [scheduler] Add suspendable task queue. Add suspendable task queue to avoid running (and cancelling) callbacks when execution context is suspended. R=skyostil@chromium.org,alexclarke@chromium.org,haraken@chromium.org CC=nhiroki@chromium.org,tzik@chromium.org BUG=700792 ==========
The CQ bit was checked by altimin@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. https://codereview.chromium.org/2754673002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp (right): https://codereview.chromium.org/2754673002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp:50: case TaskType::Unthrottled: Can you leave a TODO here for making Unthrottled actually use the unthrottled task runner? https://codereview.chromium.org/2754673002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/WebFrameScheduler.h (right): https://codereview.chromium.org/2754673002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/WebFrameScheduler.h:57: // would become suspendable in the nearest future and a new unsuspended s/unsuspended/unsuspendable/
Mind adding a few lines to https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/sched...
The CQ bit was checked by altimin@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...
Changed test per skyostil@'s suggestion. https://codereview.chromium.org/2754673002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp (right): https://codereview.chromium.org/2754673002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp:50: case TaskType::Unthrottled: On 2017/03/15 15:01:50, Sami wrote: > Can you leave a TODO here for making Unthrottled actually use the unthrottled > task runner? I think that we're fine here. We have a todo to rename suspenable to unthrottled and add unsuspened. :) https://codereview.chromium.org/2754673002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/WebFrameScheduler.h (right): https://codereview.chromium.org/2754673002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/WebFrameScheduler.h:57: // would become suspendable in the nearest future and a new unsuspended On 2017/03/15 15:01:50, Sami wrote: > s/unsuspended/unsuspendable/ I wanted to match unthrottled tq (or should we rename it to unthrottleable?)
Media tasks are posted to the unthrottled task runner. Is it okay to suspend those tasks (e.g., audio, video)?
On 2017/03/15 15:12:17, haraken wrote: > Media tasks are posted to the unthrottled task runner. > > Is it okay to suspend those tasks (e.g., audio, video)? I think that the answer is yes. We didn't encounter any specific audio-related problems when these tasks were mapped to timer task runner, did we?
> I think that the answer is yes. We didn't encounter any specific audio-related > problems when these tasks were mapped to timer task runner, did we? Right, all of these tasks used to be suspendable before M57. https://codereview.chromium.org/2754673002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/WebFrameScheduler.h (right): https://codereview.chromium.org/2754673002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/WebFrameScheduler.h:57: // would become suspendable in the nearest future and a new unsuspended On 2017/03/15 15:10:15, altimin wrote: > On 2017/03/15 15:01:50, Sami wrote: > > s/unsuspended/unsuspendable/ > > I wanted to match unthrottled tq (or should we rename it to unthrottleable?) Ah, right, let's leave as-is.
On 2017/03/15 15:18:49, Sami wrote: > > I think that the answer is yes. We didn't encounter any specific audio-related > > problems when these tasks were mapped to timer task runner, did we? > > Right, all of these tasks used to be suspendable before M57. TaskType::Unthrottled was using an unthrottled task runner, I guess. Media tasks are mapped to TaskType::Unthrottled.
haraken@: this should be fine now. PTAL.
LGTM https://codereview.chromium.org/2754673002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/WebFrameScheduler.h (right): https://codereview.chromium.org/2754673002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/WebFrameScheduler.h:56: // TODO(altimin): This is transitional task runner. Unthrottled task runner a transitional task runner
The CQ bit was checked by altimin@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 altimin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skyostil@chromium.org, haraken@chromium.org Link to the patchset: https://codereview.chromium.org/2754673002/#ps60001 (title: "Address nit")
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": 60001, "attempt_start_ts": 1490629240363730,
"parent_rev": "063b70a51b3119b222a9d1997c65481dd0e79121", "commit_rev":
"12e16766adec5bb1e5cdd73d3cd8f5016b59ce18"}
Message was sent while issue was closed.
Description was changed from ========== [scheduler] Add suspendable task queue. Add suspendable task queue to avoid running (and cancelling) callbacks when execution context is suspended. R=skyostil@chromium.org,alexclarke@chromium.org,haraken@chromium.org CC=nhiroki@chromium.org,tzik@chromium.org BUG=700792 ========== to ========== [scheduler] Add suspendable task queue. Add suspendable task queue to avoid running (and cancelling) callbacks when execution context is suspended. R=skyostil@chromium.org,alexclarke@chromium.org,haraken@chromium.org CC=nhiroki@chromium.org,tzik@chromium.org BUG=700792 Review-Url: https://codereview.chromium.org/2754673002 Cr-Commit-Position: refs/heads/master@{#459787} Committed: https://chromium.googlesource.com/chromium/src/+/12e16766adec5bb1e5cdd73d3cd8... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/12e16766adec5bb1e5cdd73d3cd8...
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/2780233002/ by rnephew@chromium.org. The reason for reverting is: Causing failures in perf tests. Reverting to confirm it is the culprit. crbug.com/706108 crbug.com/706104.
Message was sent while issue was closed.
Description was changed from ========== [scheduler] Add suspendable task queue. Add suspendable task queue to avoid running (and cancelling) callbacks when execution context is suspended. R=skyostil@chromium.org,alexclarke@chromium.org,haraken@chromium.org CC=nhiroki@chromium.org,tzik@chromium.org BUG=700792 Review-Url: https://codereview.chromium.org/2754673002 Cr-Commit-Position: refs/heads/master@{#459787} Committed: https://chromium.googlesource.com/chromium/src/+/12e16766adec5bb1e5cdd73d3cd8... ========== to ========== [scheduler] Add suspendable task queue. Add suspendable task queue to avoid running (and cancelling) callbacks when execution context is suspended. R=skyostil@chromium.org,alexclarke@chromium.org,haraken@chromium.org CC=nhiroki@chromium.org,tzik@chromium.org BUG=700792 Review-Url: https://codereview.chromium.org/2754673002 Cr-Commit-Position: refs/heads/master@{#459787} Committed: https://chromium.googlesource.com/chromium/src/+/12e16766adec5bb1e5cdd73d3cd8... ==========
Description was changed from ========== [scheduler] Add suspendable task queue. Add suspendable task queue to avoid running (and cancelling) callbacks when execution context is suspended. R=skyostil@chromium.org,alexclarke@chromium.org,haraken@chromium.org CC=nhiroki@chromium.org,tzik@chromium.org BUG=700792 Review-Url: https://codereview.chromium.org/2754673002 Cr-Commit-Position: refs/heads/master@{#459787} Committed: https://chromium.googlesource.com/chromium/src/+/12e16766adec5bb1e5cdd73d3cd8... ========== to ========== [scheduler] Add suspendable task queue. Add suspendable task queue to avoid running (and cancelling) callbacks when execution context is suspended. R=skyostil@chromium.org,alexclarke@chromium.org,haraken@chromium.org CC=nhiroki@chromium.org,tzik@chromium.org BUG=704939, 702160 Review-Url: https://codereview.chromium.org/2754673002 Cr-Commit-Position: refs/heads/master@{#459787} Committed: https://chromium.googlesource.com/chromium/src/+/12e16766adec5bb1e5cdd73d3cd8... ==========
The CQ bit was checked by altimin@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...
FYI: I'm landing this patch with only database access tasks moved to suspendable task queue. There is a problem with other tasks being suspendable. I'll investigate the problem and address it in a separate patch.
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 altimin@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/2754673002/#ps80001 (title: "For the moment make only database access tasks suspendable")
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": 1491424284328780,
"parent_rev": "ebef20203ecdd7fb17431298e175bc15546dc3ee", "commit_rev":
"cb434223a4b462abf2b9f9123646137250b3c2d9"}
Message was sent while issue was closed.
Description was changed from ========== [scheduler] Add suspendable task queue. Add suspendable task queue to avoid running (and cancelling) callbacks when execution context is suspended. R=skyostil@chromium.org,alexclarke@chromium.org,haraken@chromium.org CC=nhiroki@chromium.org,tzik@chromium.org BUG=704939, 702160 Review-Url: https://codereview.chromium.org/2754673002 Cr-Commit-Position: refs/heads/master@{#459787} Committed: https://chromium.googlesource.com/chromium/src/+/12e16766adec5bb1e5cdd73d3cd8... ========== to ========== [scheduler] Add suspendable task queue. Add suspendable task queue to avoid running (and cancelling) callbacks when execution context is suspended. R=skyostil@chromium.org,alexclarke@chromium.org,haraken@chromium.org CC=nhiroki@chromium.org,tzik@chromium.org BUG=704939, 702160 Review-Url: https://codereview.chromium.org/2754673002 Cr-Original-Commit-Position: refs/heads/master@{#459787} Committed: https://chromium.googlesource.com/chromium/src/+/12e16766adec5bb1e5cdd73d3cd8... Review-Url: https://codereview.chromium.org/2754673002 Cr-Commit-Position: refs/heads/master@{#462208} Committed: https://chromium.googlesource.com/chromium/src/+/cb434223a4b462abf2b9f9123646... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/cb434223a4b462abf2b9f9123646... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
