New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 703608 link

Starred by 5 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Timer flood destroys main thread rendering performance

Project Member Reported by skyos...@chromium.org, Mar 21 2017

Issue description

Demo from Firefox:

https://people-mozilla.org/~bkelly/timer-flood/index.html

(Full article: https://blog.wanderview.com/blog/2017/03/13/firefox-52-settimeout-changes/)

I took a quick look at the demo and it does pretty poorly on Chrome for two reasons:

1. We never prioritize compositing tasks when there's no active use case. I think we could either do that or add a new use case for main thread animation.

2. Animated GIFs are updated by a timer, which ends up competing against the flood of JS timers. I think we could post that timer into a compositing task queue.
 
Cc: -delph...@chromium.org
Owner: delph...@chromium.org
Status: Started (was: Available)
Adding comment from scheduler-dev thread:

>> 2. Animated GIFs are updated by a timer, which ends up competing against the flood of JS timers. I think we could post that timer into a compositing task queue.

Alex> That wouldn't actually help though unless we are prioritising compositing tasks.  Should it tie into the rAF machinery somehow?

Right, the fix for #1 would essentially end up prioritizing compositing tasks (both BeginMainFrame and this timer) either in UseCase::NONE or a new use case.
Current status: https://codereview.chromium.org/2810423003/

It's implemented and now is very smooth for the given test case, however some of the tsan tests now no longer complete: e.g. WebRtcMediaRecorderTest.RecordWithTransparency/2

Specifically what stops them completing is the prioritization of compositing tasks (even with code to fall back to normal prioritization for slow compositing).
This should be fixed in https://codereview.chromium.org/2810423003
Cc: tommycli@chromium.org
Partly reverted in https://chromium-review.googlesource.com/c/508792/

Regarding the CrSettingsRouteDynamicParametersTest.All flakiness ( issue 724124 ), it's absolutely possible the test is "wrong" in that it might be relying on some undefined behavior that happened to work before this CL. I can't speak for the WebRtc tests but we've hit issues like this a lot.

The meat of the test is here: https://cs.chromium.org/chromium/src/chrome/test/data/webui/settings/cr_settings_browsertest.js?type=cs&q=CrSettingsRouteDynamicParametersTest&l=1428

It looks like the popstate event's synchronicity is dependent on the non-blocking events flag: https://html.spec.whatwg.org/multipage/browsers.html#session-history

and the test itself makes 2 navigations after loading the page.

So please open a bug on CrSettingsRouteDynamicParametersTest.All if you think the flakiness should be attributed to our test rather than the correctness of your CL.
We think we have a solution to the WebRtc tests which involves posting its tasks onto the compositor task queue instead of the default queue.

Thanks for the info Michael, I'll take a look at that.
WebRtc tests are now fixed with https://chromium-review.googlesource.com/c/538583

I've also figured out the CrSettingsRouteDynamicParametersTest test which is asserting due to an IntersectionObserver callback that only expects a single entry but is now getting two. Fix is very simple.
All of that IntersectionObserver stuff was added to Settings *after* my comment #5, so your fix for CrSettingsRouteDynamicParametersTest addresses a new failure case. The original flakiness may or may not still be present :-\
Thanks for checking that. I can't detect any flakiness (having run the test 100 times) now but admittedly that was only on Linux. I will check again before submitting my follow up.
Project Member

Comment 10 by bugdroid1@chromium.org, Jul 5 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c4374fd8312a91467b300590ad4f65512b393441

commit c4374fd8312a91467b300590ad4f65512b393441
Author: Dan Elphick <delphick@chromium.org>
Date: Wed Jul 05 16:01:35 2017

Fix potential assertion failure in settings

Remove IntersectionObserver callback assertion that there be only one
entry. Since it's only checking if the element is visible or not just
ignore all but the last entry.

The assertion failure causes CrSettingsRouteDynamicParametersTest to fail in
combination with another uncommitted change to increase the priority of the
compositor task queue.

Bug: 703608
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: I3c162153e1ed32c81a02dd9eddcd3762974c118a
Reviewed-on: https://chromium-review.googlesource.com/558247
Reviewed-by: Michael Giuffrida <michaelpg@chromium.org>
Commit-Queue: Dan Elphick <delphick@chromium.org>
Cr-Commit-Position: refs/heads/master@{#484287}
[modify] https://crrev.com/c4374fd8312a91467b300590ad4f65512b393441/chrome/browser/resources/settings/settings_ui/settings_ui.js

FYI, the link in comment 0 is now dead.  I recreated the test site here:

https://timer-flood.glitch.me/
Project Member

Comment 12 by bugdroid1@chromium.org, Jul 7 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/43ee7b7f69d08a4de4cbc78250c1f9f6c87c7573

commit 43ee7b7f69d08a4de4cbc78250c1f9f6c87c7573
Author: Dan Elphick <delphick@chromium.org>
Date: Fri Jul 07 08:12:45 2017

Don't assert IntersectionObserver entries length

IntersectionObserverCallback does not guarantee that there will only be
on entry per observed element. In particular changes to the priority of
the compositor task queue can make this more likely.

Bug: 703608
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: Ied990536971d95bf049cfb2a97529d8fa526ee70
Reviewed-on: https://chromium-review.googlesource.com/561697
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Commit-Queue: Dan Elphick <delphick@chromium.org>
Cr-Commit-Position: refs/heads/master@{#484855}
[modify] https://crrev.com/43ee7b7f69d08a4de4cbc78250c1f9f6c87c7573/ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js

Project Member

Comment 13 by bugdroid1@chromium.org, Jul 7 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/67fac36a35318d03fe141887898821e42dfaecb4

commit 67fac36a35318d03fe141887898821e42dfaecb4
Author: Dan Elphick <delphick@chromium.org>
Date: Fri Jul 07 08:57:32 2017

Increase priority of compositor TQ in NONE Usecase

This allows animated GIFs to keep updating even in the presence of timer
storms.

Reland change partially reverted in
WebRtc and CrSettings tests have been landed.

https: //chromium-review.googlesource.com/c/508792/ now that fixes to
Bug: 703608
Change-Id: Id014fda26b7168658c679a7bf74359485c5262e5
Reviewed-on: https://chromium-review.googlesource.com/559345
Reviewed-by: Sami Kyöstilä <skyostil@chromium.org>
Commit-Queue: Dan Elphick <delphick@chromium.org>
Cr-Commit-Position: refs/heads/master@{#484863}
[modify] https://crrev.com/67fac36a35318d03fe141887898821e42dfaecb4/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc
[modify] https://crrev.com/67fac36a35318d03fe141887898821e42dfaecb4/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc

Status: Fixed (was: Started)
Project Member

Comment 15 by bugdroid1@chromium.org, Jul 17 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4b9c6f94315e003ef4742c192e81252a823d7682

commit 4b9c6f94315e003ef4742c192e81252a823d7682
Author: Dan Elphick <delphick@chromium.org>
Date: Mon Jul 17 12:27:56 2017

Revert "Increase priority of compositor TQ in NONE Usecase"

This reverts commit 67fac36a35318d03fe141887898821e42dfaecb4.

Reason for revert: Causes several CPU load, time and memory regressions. Fixes crbug/740469

Original change's description:
> Increase priority of compositor TQ in NONE Usecase
> 
> This allows animated GIFs to keep updating even in the presence of timer
> storms.
> 
> Reland change partially reverted in
> WebRtc and CrSettings tests have been landed.
> 
> https: //chromium-review.googlesource.com/c/508792/ now that fixes to
> Bug: 703608
> Change-Id: Id014fda26b7168658c679a7bf74359485c5262e5
> Reviewed-on: https://chromium-review.googlesource.com/559345
> Reviewed-by: Sami Kyöstilä <skyostil@chromium.org>
> Commit-Queue: Dan Elphick <delphick@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#484863}

TBR=skyostil@chromium.org,delphick@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 703608,  740469 
Change-Id: I172f61626446462bb5582c942ae58d09a13dad71
Reviewed-on: https://chromium-review.googlesource.com/573902
Commit-Queue: Dan Elphick <delphick@chromium.org>
Reviewed-by: Alex Clarke <alexclarke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#487051}
[modify] https://crrev.com/4b9c6f94315e003ef4742c192e81252a823d7682/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc
[modify] https://crrev.com/4b9c6f94315e003ef4742c192e81252a823d7682/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc

Status: Assigned (was: Fixed)
Owner: altimin@chromium.org
Similar bug report from dproy@:

"I was playing with this example in jsbin (code here.) We queue 1000 40ms tasks using setTimeout, and each task updates a count in a div. On Chrome, the counter does not increment smoothly. It jumps by around 50 each time, so the DOM updates happen about once every 50 tasks. Intriguingly, if I continuously scroll on the page for a few seconds, the counter starts incrementing smoothly.

I tested the same page on Firefox (57), Safari, and Edge, and on all those browsers, the counter increments smoothly."
Cc: divya.pa...@techmahindra.com krajshree@chromium.org manoranj...@chromium.org alexclarke@chromium.org altimin@chromium.org
 Issue 785939  has been merged into this issue.
Note sure if this is the same issue, but I noticed in devtools Performance tab that a bunch of timers seems to take up a bunch of time even if their handlers are very short.

For example, in the attached screenshot you see a bunch of timers due to the use of lodash APIs, and despite the handlers taking a very small amount of time, each "Timer Fired" takes approximately 0.45 ms (see the tooltip from the one I was hovering on).

Seems like it is spending way too much time in order to fire tiny handlers, and it causes animation "jank".

Is that the same issue listed here?
Screen Shot 2018-11-19 at 1.14.42 PM.png
110 KB View Download
I'm not sure if that's what happening here, but even a short timer task can become a long one if it involves resolving several promises. Promises are resolved in microtasks immediately after the task that triggered them.

Sign in to add a comment