Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2669883003:
Scheduler: Enqueue non-JS-timer tasks into the unthrottled task runner (Closed)
DescriptionScheduler: Enqueue non-JS-timer tasks into the unthrottled task runner
We found throttling some of non-JS-timer tasks may break existing web pages,
so this CL tentatively makes them unthrottled.
In addition, this CL switches a task type for PostMessageTimer from
TaskType::Timer to TaskType::PostedMessage because the timer is used only for
performing postMessage() and actually not used as a JS timer.
BUG=684947
Review-Url: https://codereview.chromium.org/2669883003
Cr-Commit-Position: refs/heads/master@{#448173}
Committed: https://chromium.googlesource.com/chromium/src/+/04055dd2786e27cde8d210b68e7a726fab6d0c42
Patch Set 1 #
Total comments: 5
Patch Set 2 : unthrottle more #Patch Set 3 : rebase #
Total comments: 2
Patch Set 4 : unthrottle TaskType::UnspecedTimer #
Messages
Total messages: 44 (27 generated)
Description was changed from ========== Scheduller: Enqueue PostedMessage tasks into the unthrottled task runner BUG=684947 ========== to ========== Scheduller: Enqueue PostedMessage tasks into the unthrottled task runner We found throttling PostedMessage tasks may break existing web pages, so this CL tentatively makes them unthrottled. In addition, this CL switches a task type for PostMessageTimer from TaskType::Timer to TaskType::PostedMessage because the timer is used only to post a task to the same thread with 0 delay to perform postMessage(). BUG=684947 ==========
Description was changed from ========== Scheduller: Enqueue PostedMessage tasks into the unthrottled task runner We found throttling PostedMessage tasks may break existing web pages, so this CL tentatively makes them unthrottled. In addition, this CL switches a task type for PostMessageTimer from TaskType::Timer to TaskType::PostedMessage because the timer is used only to post a task to the same thread with 0 delay to perform postMessage(). BUG=684947 ========== to ========== Scheduler: Enqueue PostedMessage tasks into the unthrottled task runner We found throttling PostedMessage tasks may break existing web pages, so this CL tentatively makes them unthrottled. In addition, this CL switches a task type for PostMessageTimer from TaskType::Timer to TaskType::PostedMessage because the timer is used only for performing postMessage() and actually not used as a typical timer. BUG=684947 ==========
nhiroki@chromium.org changed reviewers: + altimin@chromium.org, haraken@chromium.org, skyostil@chromium.org, tzik@chromium.org
Hi, could you review this? Thanks. https://codereview.chromium.org/2669883003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp (right): https://codereview.chromium.org/2669883003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp:48: case TaskType::PostedMessage: As my comment on the issue[1], I'm still not sure why we move only PostedMessage to the unthrottled task runner for a short-term fix. Why don't we move other task types (other than Timer/UnspecedTimer) to the unthrottled task runner until we have a mechanism to opt-out throttling from web pages or something else? [1] https://bugs.chromium.org/p/chromium/issues/detail?id=684947#c18
The CQ bit was checked by nhiroki@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...
I want to hear thoughts of Sami and Altimin, but I'd prefer moving all task types to the unthrottled task runner (except JS timers) until Altimin split the timer task runner into the one for JS timers and the other for other timers.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2669883003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp (right): https://codereview.chromium.org/2669883003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp:48: case TaskType::PostedMessage: On 2017/02/02 05:46:38, nhiroki (slow) wrote: > As my comment on the issue[1], I'm still not sure why we move only PostedMessage > to the unthrottled task runner for a short-term fix. Why don't we move other > task types (other than Timer/UnspecedTimer) to the unthrottled task runner until > we have a mechanism to opt-out throttling from web pages or something else? > > [1] https://bugs.chromium.org/p/chromium/issues/detail?id=684947#c18 +1, I'm particularly interested in unthrottling WebSocket messages.
https://codereview.chromium.org/2669883003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp (right): https://codereview.chromium.org/2669883003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp:48: case TaskType::PostedMessage: On 2017/02/02 18:41:25, altimin wrote: > On 2017/02/02 05:46:38, nhiroki (slow) wrote: > > As my comment on the issue[1], I'm still not sure why we move only > PostedMessage > > to the unthrottled task runner for a short-term fix. Why don't we move other > > task types (other than Timer/UnspecedTimer) to the unthrottled task runner > until > > we have a mechanism to opt-out throttling from web pages or something else? > > > > [1] https://bugs.chromium.org/p/chromium/issues/detail?id=684947#c18 > > +1, I'm particularly interested in unthrottling WebSocket messages. Yeah, this seems like a reasonable step for now. If these other tasks are a problem for CPU usage, we'll eventually find out once we improve our CPU profiling and other metrics.
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by nhiroki@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 on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
Description was changed from ========== Scheduler: Enqueue PostedMessage tasks into the unthrottled task runner We found throttling PostedMessage tasks may break existing web pages, so this CL tentatively makes them unthrottled. In addition, this CL switches a task type for PostMessageTimer from TaskType::Timer to TaskType::PostedMessage because the timer is used only for performing postMessage() and actually not used as a typical timer. BUG=684947 ========== to ========== Scheduler: Enqueue non-JS-timer tasks into the unthrottled task runner We found throttling some of non-JS-timer tasks may break existing web pages, so this CL tentatively makes them unthrottled. In addition, this CL switches a task type for PostMessageTimer from TaskType::Timer to TaskType::PostedMessage because the timer is used only for performing postMessage() and actually not used as a JS timer. BUG=684947 ==========
The CQ bit was checked by nhiroki@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...
Thank you for your comments! I moved more tasks to the unthrottled task runner. PTAL. https://codereview.chromium.org/2669883003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp (right): https://codereview.chromium.org/2669883003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp:48: case TaskType::PostedMessage: On 2017/02/02 18:41:25, altimin wrote: > On 2017/02/02 05:46:38, nhiroki (slow) wrote: > > As my comment on the issue[1], I'm still not sure why we move only > PostedMessage > > to the unthrottled task runner for a short-term fix. Why don't we move other > > task types (other than Timer/UnspecedTimer) to the unthrottled task runner > until > > we have a mechanism to opt-out throttling from web pages or something else? > > > > [1] https://bugs.chromium.org/p/chromium/issues/detail?id=684947#c18 > > +1, I'm particularly interested in unthrottling WebSocket messages. Acknowledged. https://codereview.chromium.org/2669883003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp:48: case TaskType::PostedMessage: On 2017/02/02 23:37:10, Sami (slow -- travelling) wrote: > On 2017/02/02 18:41:25, altimin wrote: > > On 2017/02/02 05:46:38, nhiroki (slow) wrote: > > > As my comment on the issue[1], I'm still not sure why we move only > > PostedMessage > > > to the unthrottled task runner for a short-term fix. Why don't we move other > > > task types (other than Timer/UnspecedTimer) to the unthrottled task runner > > until > > > we have a mechanism to opt-out throttling from web pages or something else? > > > > > > [1] https://bugs.chromium.org/p/chromium/issues/detail?id=684947#c18 > > > > +1, I'm particularly interested in unthrottling WebSocket messages. > > Yeah, this seems like a reasonable step for now. If these other tasks are a > problem for CPU usage, we'll eventually find out once we improve our CPU > profiling and other metrics. Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2669883003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp (right): https://codereview.chromium.org/2669883003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp:20: case TaskType::UnspecedTimer: I would move UnspecedTimer to the unthrottled task runner as well.
lgtm % Kentaro's comment.
lgtm
Thank you. https://codereview.chromium.org/2669883003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp (right): https://codereview.chromium.org/2669883003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp:20: case TaskType::UnspecedTimer: On 2017/02/03 16:33:47, haraken wrote: > > I would move UnspecedTimer to the unthrottled task runner as well. Done.
The CQ bit was checked by nhiroki@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 nhiroki@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/2669883003/#ps80001 (title: "unthrottle TaskType::UnspecedTimer")
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by nhiroki@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": 80001, "attempt_start_ts": 1486258717408820,
"parent_rev": "112c3d120a80a3e37590571b61a6a924ead2b935", "commit_rev":
"04055dd2786e27cde8d210b68e7a726fab6d0c42"}
Message was sent while issue was closed.
Description was changed from ========== Scheduler: Enqueue non-JS-timer tasks into the unthrottled task runner We found throttling some of non-JS-timer tasks may break existing web pages, so this CL tentatively makes them unthrottled. In addition, this CL switches a task type for PostMessageTimer from TaskType::Timer to TaskType::PostedMessage because the timer is used only for performing postMessage() and actually not used as a JS timer. BUG=684947 ========== to ========== Scheduler: Enqueue non-JS-timer tasks into the unthrottled task runner We found throttling some of non-JS-timer tasks may break existing web pages, so this CL tentatively makes them unthrottled. In addition, this CL switches a task type for PostMessageTimer from TaskType::Timer to TaskType::PostedMessage because the timer is used only for performing postMessage() and actually not used as a JS timer. BUG=684947 Review-Url: https://codereview.chromium.org/2669883003 Cr-Commit-Position: refs/heads/master@{#448173} Committed: https://chromium.googlesource.com/chromium/src/+/04055dd2786e27cde8d210b68e7a... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as https://chromium.googlesource.com/chromium/src/+/04055dd2786e27cde8d210b68e7a...
Message was sent while issue was closed.
On 2017/02/05 04:06:18, commit-bot: I haz the power wrote: > Committed patchset #4 (id:80001) as > https://chromium.googlesource.com/chromium/src/+/04055dd2786e27cde8d210b68e7a... if "We found throttling some of non-JS-timer tasks may break existing web pages," then there are bugs in other code not the schedular. The other code should work under any schedulaing policy. Scedulaing is only a priority mechanisim. Your fix hide the real bugs
Message was sent while issue was closed.
On 2017/02/05 04:57:48, RE66 wrote: > On 2017/02/05 04:06:18, commit-bot: I haz the power wrote: > > Committed patchset #4 (id:80001) as > > > https://chromium.googlesource.com/chromium/src/+/04055dd2786e27cde8d210b68e7a... > > if "We found throttling some of non-JS-timer tasks may break existing web > pages," > then there are bugs in other code not the schedular. > The other code should work under any schedulaing policy. > Scedulaing is only a priority mechanisim. > Your fix hide the real bugs “throttling some of non-JS-timer tasks may break existing web pages” could be a confusing phrase. “throttling some of non-JS-timer tasks may make existing web pages useless” would represents more accurately what I wanted to say here. This is a workaround not for a scheduler bug but for a priority issue. As you mentioned, web applications should work under any scheduling policy, but some applications do not really make sense under task throttling. For example, a chat application would be expected to work responsively even while it's in a background tab so that it can check new messages and notify them to users. Before expanding the scope of task throttling, we should consider providing an opt-out mechanism for such applications and/or identify what kind of tasks shouldn't be throttled.
Message was sent while issue was closed.
On 2017/02/07 10:42:10, nhiroki (slow) wrote: > On 2017/02/05 04:57:48, RE66 wrote: > > On 2017/02/05 04:06:18, commit-bot: I haz the power wrote: > > > Committed patchset #4 (id:80001) as > > > > > > https://chromium.googlesource.com/chromium/src/+/04055dd2786e27cde8d210b68e7a... > > > > if "We found throttling some of non-JS-timer tasks may break existing web > > pages," > > then there are bugs in other code not the schedular. > > The other code should work under any schedulaing policy. > > Scedulaing is only a priority mechanisim. > > Your fix hide the real bugs > > “throttling some of non-JS-timer tasks may break existing web pages” could be a > confusing phrase. “throttling some of non-JS-timer tasks may make existing web > pages useless” would represents more accurately what I wanted to say here. > > This is a workaround not for a scheduler bug but for a priority issue. As you > mentioned, web applications should work under any scheduling policy, but some > applications do not really make sense under task throttling. For example, a chat > application would be expected to work responsively even while it's in a > background tab so that it can check new messages and notify them to users. > Before expanding the scope of task throttling, we should consider providing an > opt-out mechanism for such applications and/or identify what kind of tasks > shouldn't be throttled. Furthermore, unfortunately we've seen some applications that do break when all we do is change priorities or enable throttling. For example some database libraries have timeouts for transactions, and many applications don't fail gracefully when those timeouts fire.
Message was sent while issue was closed.
On 2017/02/07 10:49:05, Sami (slow -- travelling) wrote: > On 2017/02/07 10:42:10, nhiroki (slow) wrote: > > On 2017/02/05 04:57:48, RE66 wrote: > > > On 2017/02/05 04:06:18, commit-bot: I haz the power wrote: > > > > Committed patchset #4 (id:80001) as > > > > > > > > > > https://chromium.googlesource.com/chromium/src/+/04055dd2786e27cde8d210b68e7a... > > > > > > if "We found throttling some of non-JS-timer tasks may break existing web > > > pages," > > > then there are bugs in other code not the schedular. > > > The other code should work under any schedulaing policy. > > > Scedulaing is only a priority mechanisim. > > > Your fix hide the real bugs > > > > “throttling some of non-JS-timer tasks may break existing web pages” could be > a > > confusing phrase. “throttling some of non-JS-timer tasks may make existing web > > pages useless” would represents more accurately what I wanted to say here. > > > > This is a workaround not for a scheduler bug but for a priority issue. As you > > mentioned, web applications should work under any scheduling policy, but some > > applications do not really make sense under task throttling. For example, a > chat > > application would be expected to work responsively even while it's in a > > background tab so that it can check new messages and notify them to users. > > Before expanding the scope of task throttling, we should consider providing an > > opt-out mechanism for such applications and/or identify what kind of tasks > > shouldn't be throttled. > > Furthermore, unfortunately we've seen some applications that do break when all > we do is change priorities or enable throttling. For example some database > libraries have timeouts for transactions, and many applications don't fail > gracefully when those timeouts fire. I think that we need to be firm and to force authors of these libraries adapt and handle these timeouts. However, we will need better communication about this before we commit to it. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
