Timer flood destroys main thread rendering performance |
||||||
Issue descriptionDemo 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.
,
Apr 7 2017
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.
,
May 5 2017
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).
,
May 18 2017
This should be fixed in https://codereview.chromium.org/2810423003
,
May 23 2017
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.
,
May 23 2017
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.
,
Jul 5 2017
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.
,
Jul 5 2017
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 :-\
,
Jul 5 2017
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.
,
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
,
Jul 6 2017
FYI, the link in comment 0 is now dead. I recreated the test site here: https://timer-flood.glitch.me/
,
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
,
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
,
Jul 7 2017
,
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
,
Jul 17 2017
,
Jan 9 2018
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."
,
Apr 3 2018
Issue 785939 has been merged into this issue.
,
Nov 19
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?
,
Nov 20
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 |
||||||
Comment 1 by delph...@chromium.org
, Apr 7 2017Owner: delph...@chromium.org
Status: Started (was: Available)