1655102 - Long "APZScroll Payload Presented" delays recorded when pausing between pans

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect, P3)

Description

[Markus Stange [:mstange]]

Profile: https://share.firefox.dev/3hwMHDf

When pausing between multiple subsequent scroll gestures, the "APZScroll Payload Presented" markers in the profile often cover the duration of the pause. This seems unintentional. The markers should be going from the timestamp of the event that caused the scroll position change, to the timestamp of the composite that presented that scroll position. Either we have a mistake in the measuring, or the last scroll position update before a pause doesn't properly get presented.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

I can look into this along with bug 1641070

Comment 2

[Kartikaya Gupta (email:[e-mail])]

I wasn't seeing this exactly when I tried to repro locally. Some latencies were in the 500ms range but they didn't line up with pausing/resuming of scrolling. I couldn't use the profiler with WR enabled in my local debug build (hit bug 1655888) so I tried with WR disabled. Not sure if that's a factor.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

I tested this again with logging and believe the problem is that we're creating scroll payloads for input events that don't result in scrolling. In this case, the pan-momentumend event triggers a call to OnPan in case there is some scroll delta on that event. In the case that there is no delta, we still end up creating the payload and attempting the scroll which is basically a no-op. That therefore doesn't trigger a composite, and the payload only gets recorded on the next composite which can be some time later.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

It's easy enough to fix this specific case but this might be a general class of problems with other input events too. I can think of ways to fix the entire class of problems (e.g. use some RAII thing around the entire input handling mechanism and only register the payload if the input triggered a call to ScheduleComposite). However, I think that approach might paper over legitimate bugs where we get an input event and erroneously don't request a composite. As-is, that sort of bug will show up as a payload with high latency but with the RAII approach we wouldn't even generate a payload and so would lose a potentially useful detection mechanism.

So for now I'm inclined to just do the spot fix and if we keep running into issues like this in the future we can consider a more holistic fix.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1655102 - Don't record a scroll payload if we don't expect scrolling to happen. r?mstange

This is a bit of a spot fix for the specific scenario encountered. If there are
other scenarios in which we encounter a similar problem we should consider a
higher level fix.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fe1395e1fdf7
Don't record a scroll payload if we don't expect scrolling to happen. r=botond

Comment 7

[Markus Stange [:mstange]]

Thanks! I agree that a spot fix seems appropriate.

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/fe1395e1fdf7