1456638 - Checkerboarding while scrollbar-dragging due to slow display list build on specific page with table layout

Status: NEW

(Core :: Web Painting, defect, P3)

Description

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Attachment: test.7z

STR:
1a. Open this website page: https://ftp.mozilla.org/pub/firefox/tinderbox-builds/mozilla-inbound-win32/
or
1bI. Unpack "test.7z" archive file
1bII. Open unpacked "test.htm" file
2a. Scroll it with scrollbar
or
2b. Scroll it with "Page Down" keyboard button
and see that white blank page content is shown on some website pages while scrolling

Comment 1

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Attachment: screencast.mp4

Comment 2

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Attachment: Mozregression-GUI log.txt

Mozregression-GUI log didn't find specific regression range (see "Mozregression-GUI log.txt" attachment),
but at least it pointed that:
- last good build is 2016-12-27 (rev: 3119a9a0b5dee60ac77b7596ae5dbe0658f598ad)
- first bad build is 2016-12-28 (rev: d7b6af32811bddcec10a47d24bd455a1ec1836fc),
so basing on this and looking on regression range push log:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=3119a9a0b5dee60ac77b7596ae5dbe0658f598ad&tochange=d7b6af32811bddcec10a47d24bd455a1ec1836fc
I deduced that it could be due to bug #1324117, which enabled APZ scrollbar dragging,
setting "apz.drag.enabled" preference to "false" value in "about:config" confirmed this, as bug didn't occur.

Comment 3

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

This looks like checkerboarding. If you can get a Gecko profile that might show if there's things we can improve, but otherwise there's not much we can do here.

Comment 4

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Attachment: Scrolling down and up with mouse scroll wheel.7z

Comment 5

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Attachment: Scrolling down and up with Page Up and Page Down keyboard buttons.7z

Comment 6

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Attachment: Scrolling down and up with window scrollbar.7z

Comment 7

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Sure, please have a look, I also want to add that it's also reproducible with scrolling doiwn and yp with mouse scroll wheel


Scrolling down and up with mouse scroll wheel (attachment in comment #4)
https://perfht.ml/2HNqjHO

Scrolling down and up with Page Up and Page Down keyboard buttons (attachment in comment #5)
https://perfht.ml/2HOjzco

Scrolling down and up with window scrollbar (attachment in comment #6)
https://perfht.ml/2qX2wve


If you need more tests or profiles, just :ni me. Thanks!


P.S. Chromium doesn't have this problem and also have "smooth" scrolling, as you can see in "screencast.mp4" attachment file.

Comment 8

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

Your profiles have a *lot* of unnecessary threads. That by itself will make things worse because it increases the profiler overhead and impacts performance. But anyway, in profiles the display list building time is long in the content process, mostly because of tables, it seems. This happens both during paint and during hit-testing (in the mousewheel profile).

Moving to web painting since Matt might have thoughts on how to improve the performance of tables based on this, but given how old that code I'm not sure if there's much left in terms of optimizing that.

Comment 9

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #8)
> Your profiles have a *lot* of unnecessary threads.
> That by itself will make things worse
> because it increases the profiler overhead and impacts performance.

Yes, I caught all threads.

These was my Gecko Profiler 0.21 settings:
- Interval = 1ms
- Buffer size = 90MB
- Threads = (blank, to catch all threads)
- Features: Stack walk, Responsiveness, JavaScript, Task tracer

I tested it on Mozilla Firefox Nightly 61.0a1 (2018-04-25) (64-bit) on clean new fresh profile without any addons (extensions, plugins, themes, etc.)


Could you recommencement me proper settings of Gecko Profiler, so I will redo the tests to minimize noise for developers? Thanks.

Comment 10

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

For this case the "GeckoMain,Compositor,Paint" threadlist should be sufficient. The rest of your options seem fine.

Comment 11

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Attachment: Scrolling down and up with mouse scroll wheel v2.7z

Comment 12

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Attachment: Scrolling down and up with Page Up and Page Down keyboard buttons v2.7z

Comment 13

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Attachment: Scrolling down and up with window scrollbar v2.7z

Comment 14

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Thanks!

I redone tests with Gecko Profiler 0.21 with these settings:
- Interval = 1ms
- Buffer size = 90MB
- Threads = GeckoMain,Compositor,Paint
- Features: Stack walk, Responsiveness, JavaScript, Task tracer
on Mozilla Firefox Nightly 61.0a1 (2018-04-25) (64-bit) on clean new fresh profile without any addons (extensions, plugins, themes, etc.)
and here are less noisy profiles:

Scrolling down and up with mouse scroll wheel
https://perfht.ml/2HSuwdm (mouse setting: 10 pages per scroll, but bug is not reproducible as before, it's probably due to less threads saved by Gecko Profiler)
https://perfht.ml/2HR3zGG (attachment in comment #11) (so I set - mouse setting: 100 pages per scroll , and it was reproducible again)

Scrolling down and up with Page Up and Page Down keyboard buttons (attachment in comment #12)
https://perfht.ml/2qXm1DU

Scrolling down and up with window scrollbar (attachment in comment #13)
https://perfht.ml/2r3SWXJ

Comment 15

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

(Virtual_ManPL [:Virtual] wrote in comment #14)
> 10 pages per scroll
> 100 pages per scroll
Ups, my bad, I meant "lines per scroll", not "pages per scroll".

Comment 16

[Matt Woodrow (:mattwoodrow)]

Yeah, this is a massive table and building the display list takes too long unfortunately :(

The biggest chunk of time is spent iterating all the rows of the table and checking to see if they are within the dirty rect, which ends up being false for most of them.

I had a look at the table reflow code, and it appears that the rows should always be increasing in position (in the block direction), so I think once we get past the dirty rect, we can skip all following rows.

We might want to consider something to allow us to skip rows before the dirty rect quicker too, similar to nsLineBox.

Comment 17

[Mayank Bansal]

with the test.7z attachment, sometimes the page spends large amounts of time in text-shaping under Harfbuzz :

https://perfht.ml/2z352nC

Comment 18

[Daniel Holbert [:dholbert]]

Here's a profile I just captured at the FTP link from comment 0 (hitting home/end keys a few times):
http://bit.ly/2RwoQ9o

Comment 19

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

This bug also affects the new about:config page, see bug 1523028 for a long meandering discussion. I'm going to bump the qf priority of this bug to match that of bug 1523028, and I'm copying some of the profile links from that bug here:

http://bit.ly/2RgBaKS
http://bit.ly/2Rmu6fL
http://bit.ly/2RmvWNH (webrender enabled)
http://bit.ly/2Rrj7Se (webrender enabled)
http://bit.ly/2CQoDZs
http://bit.ly/2CUvqRY
http://bit.ly/2WAdaXd
http://bit.ly/2WuI8je

I looked at some of these profiles and it looks like much of the time is spent in rasterizing rather than DL building, so it does seem like there should be room for improvement. We might be rasterizing or layerizing poorly.

Comment 20

[Tim Nguyen :ntim]

Attachment: alternate-layout.diff

Hey :kats, :dholbert and :mconley

This layout restricts the painting only to the table contents when scrolling, whereas without the patch applied, the painting is also done outside of the table area when scrolling. (I tested this with the paint flashing tool).

Does this layout actually improve the performance when scrolling ?

Comment 21

[Daniel Holbert [:dholbert]]

That patch seems to help a bit, but not significantly so.

Here's a profile with current mozilla-central (--enable-optimize --disable-debug --enable-debug-symbols), hitting "End" & "Home" to scroll to the extremes of about:config, scrolling 8 times:
https://perfht.ml/2GxrLN8

And here's a profile of the same steps, with ntim's comment 20 patch:
https://perfht.ml/2GrETU5

(both of those short URLs are already filtered for mozilla::PresShell::Paint in their backtraces)

A few metrics:

                              Unpatched   Patched
Total time spent in ::Paint   1827ms      1686ms
Longest rasterize duration    128ms       117ms  (both happen at around 8s in their respective profiles)

So, a bit better but not better enough to be meaningful. (i.e. I still notice the flashes of white, and the 128ms vs. 117ms improvement isn't really perceptible)

Comment 22

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

Clearing my needinfo since dholbert answered your question.

Comment 23

[Mike Conley (:mconley) (:⚙️)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #22)

Clearing my needinfo since dholbert answered your question.

Ditto!

Comment 24

[Neha Kochar [:neha]]

Bulk change for all regression bugs with status-firefox67 as 'fix-optional' to be marked 'affected' for status-firefox68.

Comment 25

[Tim Spurway [:tspurway]]

[bulk change 69 status -> --- b/c to stop re-triaging old regressions every release]

Triage Owners: please do not set release status tracking flags in new releases unless this bug will actually be worked on

Comment 26

[Timothy Nikkel (:tnikkel)]

This looks to be all pre-webrender. If there is still a problem here we need new STR to look into.