1754562 - Yanks caused by GC Microslices and DOM operations are very apparent with APZ kinectic scrolling

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Simonas Kazlauskas [:nagisa/:simukis]]

During kinetic APZ scrolling animation stutters and yanks can be seen routinely.

https://share.firefox.dev/3GRf5MZ this 5 second profile showcases 2 such yank events. The first is between 2.2s and 2.3s during a 49ms GCSlice where no frame renders occurred. And another starting at around 3.15s during some sort of DOM processing in the parent process.

Both of these seemingly very minor stutters are very apparent when the otherwise reasonably smooth kinetic scroll is in progress. Note that the yanks reported in the web page's thread are irrelevant here – scrolling happens just fine regardless.

I don't know if there is a solution other than prohibiting any uninterruptible execution that lasts longer than the time remaining till next vsync.

Comment 1

[Steve Fink [:sfink] [:s:]]

This is where I get fuzzy on how rendering, APZ scrolling, etc. work. But if the main thread is needed in order to keep the scrolling going, I agree that the GCSlice should not be getting such a large budget. My limited understanding is that the idle task scheduler found things to be idle and believed that vsyncs were not relevant, so it handed out a maximally-sized 49ms slice to the GC to work with. It sounds like you're saying that that was incorrect, and that the thread needed to do something else in time for the vsync. Why was its participation necessary during APZ scrolling? And how could this situation be communicated to the idle task scheduler? If this is always the case during APZ scrolling, then either GetIdleDeadlineHint or its callers seem like they ought to take it into account.

I'm unlikely to be the one who would make that fix, but I'd still like to understand what's going on. I'm currently more on the GC side, and I could truncate the GC if it is known to be interfering with something, but it seems like more of an idle scheduling problem especially given that DOM processing causes similar stutters.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Was WebRender irresponsive at that moment, isn't it? I can see a blank in each case, the GC one and the DOM one, in the Renderer thread. CCing Jeff.

Comment 3

[Timothy Nikkel (:tnikkel)]

What kind of cpu is this profile recorded on? Is there maybe contention for the cpu cores and other threads don't get scheduled when the GC and the sessionstore is happening on the parent process main thread?

Comment 4

[Simonas Kazlauskas [:nagisa/:simukis]]

i5-10210U. The machine in question doesn't do particularly much besides running Firefox and an audio-only gstreamer pipeline in the background. Looking at prometheus metrics (with all caveats that these metrics entail, such as fairly low resolution), the machine had between 0.6 to 1.1 of a logical core loaded during the profiling session.

Comment 5

[Timothy Nikkel (:tnikkel)]

Okay, so probably not that then.

Would you be able to take another profile, but using the "Graphics" preset? This won't have screenshots though.

Comment 6

[Simonas Kazlauskas [:nagisa/:simukis]]

https://share.firefox.dev/34MAU2S A profile with graphics preset. This particular one, unlike the original profile, was captured in a mostly fresh firefox profile (i.e. the profile that I fire up once every few months for bug reproduction purposes…). There are two relevant stutter events: one at 4.5s and another at 8.7s.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

This is an interesting case. Parent process gets idle time since there isn't much running there - refresh driver isn't ticking and it just gets OS level events but those aren't really queued anywhere, so it thinks there is idle time.
I guess this is XPCOM, since that is where idle handling lives.
Parent process should consider the activity of the child processes or check pending OS level events. That latter is difficult in general, since not
all idle processing is interruptible.
Limiting parent process idle period to frame time - layout.idle_period.time_limit if a foreground page has active refreshdriver should work here.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Bug 1754562, make parent process' idle detection be aware of active RefreshDrivers in the other processes, r=mstange

Comment 9

[Simonas Kazlauskas [:nagisa/:simukis]]

Happy to test try builds if I'm given a tar.bz2 and its checksum.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

On which platform are you? I guess linux.
https://treeherder.mozilla.org/logviewer?job_id=367502880&repo=try
scroll down top left area to find target.tar.bz2 and there seems to be target.checksum too.

(that isn't from the latest try run, but should work in normal cases. It may just crash when running some tests without any window.)

Comment 11

[Simonas Kazlauskas [:nagisa/:simukis]]

This patch definitely improves things significantly. The stutters that used to happen every couple seconds are now mostly gone, and I went through dozens of rounds of capturing a profile trying to reproduce a stutter, whereas originally it would take me just one or two attempts.

The overall problem still seems to be present, however. https://share.firefox.dev/3Lrz34f between 5s and 8s demonstrates one such very major event. At around 5.4s there's also still a 61ms long GCSlice which prevented rendering from taking place.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

I wonder why the JS heap is so large that tracing through it in parent process takes so much time.
But interestingly child process does have a long running script and while that is happening, there aren't vsyncs so parent process does get some
more idle time and runs longer slices. And when child process has ticks, parent process processes shorter slices.
So idle handling does look better.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/39bd7bdd12be
make parent process' idle detection be aware of active RefreshDrivers in the other processes, r=mstange

Comment 14

[Cristina Cozmuta (:CrissCozmuta)]

Backed out for causing multiple failures. CLOSED TREE

Backout link: https://hg.mozilla.org/integration/autoland/rev/0dc32606a8d8e8284a3337c70bb5a97f480953e0

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Crunnable&revision=39bd7bdd12bef4c744c5cdd0c1264b0aa62b84eb&selectedTaskRun=Yla_f56FQ_ySZ-X8gjmhPQ.0

Link to failure logs:
https://treeherder.mozilla.org/logviewer?job_id=369194079&repo=autoland&lineNumber=116442
https://treeherder.mozilla.org/logviewer?job_id=369194421&repo=autoland&lineNumber=2964
https://treeherder.mozilla.org/logviewer?job_id=369194284&repo=autoland&lineNumber=1659

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/051b1970a155
make parent process' idle detection be aware of active RefreshDrivers in the other processes, r=mstange

Comment 16

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/051b1970a155

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

(removing leftover needinfo)