1803713 - Need to implement AddPendingScrollUpdateForNextTransaction

Status: RESOLVED FIXED

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

Description

[Hiroyuki Ikezoe (:hiro)]

The WebRender backend doesn't override it.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1803713 - Implement WebRenderLayerManager::AddPendingScrollUpdateForNextTransaction. r?botond

Comment 2

[Hiroyuki Ikezoe (:hiro)]

There were .orig files when I was trying to implement this and hoped it fixes bug 1801098, but it didn't fix the bug.

Comment 3

[Botond Ballo [:botond]]

I believe this is a regression from bug 1727682, which copied the mPendingScrollUpdates field from LayerManager to WebRenderLayerManager, but did not copy the override of the function which populates the field.

Comment 4

[Botond Ballo [:botond]]

One thing I don't understand is: why was this regression not caught by this assertion in helper_scrollto_tap.html? The check is supposed to assert that paint skipping does happen.

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1727682

Comment 6

[Hiroyuki Ikezoe (:hiro)]

With D167628, the paint-skip test which will be landed in bug 1826476 significantly improves. Though, I don't quite understand the reason, some of tests with apz.paint_skipping.enabled=false are also affected/regressed. Theoretically D167628 should not have any impacts on apz.paint_skipping.enabled=false code path, so I guess it just means it's because less numbers of trials?

https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=02ae0933206fd89631c7763a5c8d26879b8bd186&newProject=try&newRevision=503fb50659600a02767a35956adafb4913685d61&framework=1&page=1&filter=scroll

Comment 7

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)

Though, I don't quite understand the reason, some of tests with apz.paint_skipping.enabled=false are also affected/regressed. Theoretically D167628 should not have any impacts on apz.paint_skipping.enabled=false code path, so I guess it just means it's because less numbers of trials?

I did trigger more runs on the affected ones, indeed D167628 affects there, but only on Linux/Windows opt builds. Oddly it doesn't affect shippable builds. I suppose shippable builds are PGO builds, thus the pref apz.paint_skipping.enabled pref should be default, true. It's surprising PGO builds is optimized for the cases of apz.paint_skipping.enabled=false.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/78c95de36253
Implement WebRenderLayerManager::AddPendingScrollUpdateForNextTransaction. r=botond

Comment 9

[Atila Butkovits]

Backed out for causing failures at z-index-blend-will-change-overlapping-layers.html.

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

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=78c95de362537cac618da9de10f895e056e53dd9&selectedTaskRun=V94n4kLVTc6nEt2yr_GD7g.0

Failure logs:
https://treeherder.mozilla.org/logviewer?job_id=484684808&repo=autoland&lineNumber=17875
https://treeherder.mozilla.org/logviewer?job_id=484684772&repo=autoland&lineNumber=4551

Comment 10

[Hiroyuki Ikezoe (:hiro)]

There's definitely a bug related our paint-skipping code. The reason why the test fails is that we apply a scrollBy(0,100) twice there because of this FrameMetrics::UpdatePendingScrollInfo call. I don't quite understand why we need to update the FrameMetrics' visual scroll offsets, specifically the fix for bug 1685009.

From https://bugzilla.mozilla.org/show_bug.cgi?id=1685009#c4

(In reply to Botond Ballo [:botond] from comment #4)

Based on the Pernosco session, what seems to be happening is, a paint-skip transaction is taking the is-first-paint branch of NotifyLayersUpdated().

In the is-first-paint branch, APZ accepts the layer's FrameMetrics wholesale, including both the visual and layout scroll offsets. However, when applying a paint-skipped scroll to the layer's FrameMetrics on the content side, we only update the layout scroll offset, resulting in the two scroll offsets being out of sync in the layer's FrameMetrics copy. When APZ then accepts the layer's FrameMetrics wholesale, it is stuck with the out-of-sync offsets.

My best guess is the the is-first-paint in question was skipped because of the race we fixed in bug 1845646 so that the APZC's FrameMetrics were out of date? (Note that there are multiple is-first-paint updates happen, I don't know the reason but I observed it in bug 1845646)

^ this guess was wrong. Even without the fix for bug 1845646, I can't see bug 1685009 at all.

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1803713 - Add assertions for functions to apply a ScrollPositionUpdate in FrameMetrics. r?botond

These functions are only used for instant scroll updates, for smooth
cases we never update FrameMetrics directly, rather we create a smooth
scroll animation.

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1803713 - Do not update the APZC's metrics if there's any scorll updates from the main-thread. r?botond

In the case of paint skipping, each APZC's FrameMetrics are not in sync
with the metrics on the main-thread actually, but specifically for the
visual scroll offset of sub scroll containers it doesn't matter since
we will process each scroll position update in the order in which each
scroll positon update happened on the main-thread, thus the final
destination of the visual scroll offset should reach to the correct
position.

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1803713 - Clobber the first-paint flag after we processed the scroll metatda. r?botond

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/702bf1b90a3a
Implement WebRenderLayerManager::AddPendingScrollUpdateForNextTransaction. r=botond
https://hg.mozilla.org/integration/autoland/rev/e496cbbef08d
Add assertions for functions to apply a ScrollPositionUpdate in FrameMetrics. r=botond
https://hg.mozilla.org/integration/autoland/rev/320e8130aa77
Do not update the APZC's metrics if there's any scorll updates from the main-thread. r=botond
https://hg.mozilla.org/integration/autoland/rev/01fb02e222a0
Clobber the first-paint flag after we processed the scroll metatda. r=botond
https://hg.mozilla.org/integration/autoland/rev/8e8305f85074
apply code formatting via Lando

Comment 15

[amarc]

https://hg.mozilla.org/mozilla-central/rev/702bf1b90a3a
https://hg.mozilla.org/mozilla-central/rev/e496cbbef08d
https://hg.mozilla.org/mozilla-central/rev/320e8130aa77
https://hg.mozilla.org/mozilla-central/rev/01fb02e222a0
https://hg.mozilla.org/mozilla-central/rev/8e8305f85074

Comment 16

[Mayank Bansal]

70%-80% improvement on all "skip*" Talos tests on all the 3 platforms.
(A perf alert has not yet been filed and validated by the perf-sheriffs, so take this with a grain of salt)

Comment 17

[Andrej (:aglavic)]

Perfherder has detected a talos performance change from push 8e8305f85074074769f6d51a556f210132f2c25b.

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
75%	tscrollx_paint_skip	linux1804-64-shippable-qr	e10s fission stylo webrender-sw	0.61 -> 0.15
75%	tscrollx_paint_skip	windows11-64-shippable-qr	e10s fission stylo webrender-sw	0.60 -> 0.15
74%	tscrollx_paint_skip	macosx1470-64-shippable	e10s fission stylo webrender-sw	0.47 -> 0.12
74%	tscrollx_paint_skip	linux1804-64-shippable-qr	e10s fission stylo webrender	0.96 -> 0.25
70%	tscrollx_paint_skip	macosx1470-64-shippable	e10s fission stylo webrender	0.53 -> 0.16
...	...	...	...	...
45%	tp5o_scroll_paint_skip	windows11-64-shippable-qr	e10s fission stylo webrender-sw	1.07 -> 0.59

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests.

If you need the profiling jobs you can trigger them yourself from treeherder job view or ask a sheriff to do that for you.

You can run these tests on try with ./mach try perf --alert 43203

For more information on performance sheriffing please see our FAQ.