1536121 - cleanup display item representation

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, task, P2)

Description

[Aria Beingessner [:Gankra]]

as described in https://github.com/servo/webrender/issues/3334

(push the enum to the top so less items have junk fields)

Comment 1

[Aria Beingessner [:Gankra]]

Attachment: Bug 1536121 - add some finer-grain display list stats (hacky/debug-only).

Comment 2

[Aria Beingessner [:Gankra]]

Attachment: Bug 1536121 - cleanup display list code a little bit to prep for refactor.

• make all enums repr(u8) (compiler bug blocking this long fixed)
• enable feature(nll) to remove hack, and catch more unused_mut
• remove cache markers (abandonned design)
• don't always push empty SetFilters before PushStackingContext
• remove dead pub methods

Depends on D25844

Comment 3

[Aria Beingessner [:Gankra]]

try push (clean!): https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea85debaa128383037f8a9906f3cfc5df01fb23e&selectedJob=240063589

talos: https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=ea85debaa128383037f8a9906f3cfc5df01fb23e&selectedTimeRange=172800

talos analysis:

Looks like a wash, which I consider acceptable since this removes a couple performance hacks to create cleaner/better code that we can optimize more in the future. There are two seemingly concerning linux64-shippable-qr regressions, but we don't see those regressions on linux64-qr, so it's either noise or something that's resolved in nightly.

Comment 4

[Aria Beingessner [:Gankra]]

Attachment: Bug 1536121 - rearchitect the webrender display-list.

disclaimer: this isn't an amazing cleanup, but more of a major step that
unlocks the ability to do more minor cleanups and refinements. There's some
messy things and inconsistencies here and there, but we can hopefully iron
them out over time.

1. The primary change here is to move from
   struct { common_fields, enum(specific_fields) }
   to
   enum (maybe_common_fields, specific_fields)

most notably this drops the common fields from a ton of things
that don't need them PopXXX, SetXXX, ClipChain, etc.

2. Additionally some types have had some redundant states shaved off,
   for instance, rect no longer has both bounds and a clip_rect, as
   the intersection of the two can be used. This was done a bit conservatively
   as some adjustments will need to be done to the backend to fully eliminate
   some states, and this can be done more incrementally.

2.5. As a minor side-effect of 2, we now early-reject some primitives whose
bounds and clip_rect are disjoint.

3. A HitTest display item has been added, which is just a Rect without
   color. In addition to the minor space wins from this, this makes it much
   easier to debug display lists

4. Adds a bunch of comments to the display list, making it easier to understand
   things.

The end result of all these changes is a significantly smaller and easier to
understand display list. Especially on pages like gmail which have so many
clip chains. However this ultimately just makes text an even greater percentage
of pages (often 70-80%).

Comment 5

[Aria Beingessner [:Gankra]]

scratch that talos analysis, i misunderstood what the "shippable" build meant. That's the most important and most optimized one. My change appears to be hindering PGO from doing some magic enhancement.

Comment 6

[Aria Beingessner [:Gankra]]

So after some discussion I'm going to go ahead and OK the linux64 scrolling regressions. This is the first step in some major cleanups, and it's worth the hit in this one place. I expect future work that builds off this will reclaim the lost performance.

Comment 7

[Aria Beingessner [:Gankra]]

pausing checking-needed while I hash out a detail with aosmond

Comment 8

[Aria Beingessner [:Gankra]]

ok we're comfortable with moving forward

Comment 9

[Narcis Beleuzu [:NarcisB]]

:Gankro , the patch failed to land with message:

Details: We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. (255, 'applying /tmp/tmpEmkblI\npatching file gfx/wr/webrender_api/src/display_list.rs\nHunk #2 FAILED at 37\n1 out of 13 hunks FAILED -- saving rejects to file gfx/wr/webrender_api/src/display_list.rs.rej\nabort: patch failed to apply', '')

Comment 10

[Aria Beingessner [:Gankra]]

rebased

Comment 11

[Andreea Pavel [:apavel]]

On Fri, April 19, 2019, 3:10 AM GMT+3, by [e-mail].
Revisions: D25845 diff 92706 ← D27439 diff 92707
Details: We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. (255, 'applying /tmp/tmpwKqppc\npatching file gfx/wr/webrender/src/display_list_flattener.rs\nHunk #4 FAILED at 594\nHunk #5 succeeded at 687 with fuzz 2 (offset 58 lines).\nHunk #8 succeeded at 834 with fuzz 1 (offset 58 lines).\nHunk #9 FAILED at 818\nHunk #12 FAILED at 983\n3 out of 14 hunks FAILED -- saving rejects to file gfx/wr/webrender/src/display_list_flattener.rs.rej\nabort: patch failed to apply', '') 0 AM GMT+3, by [e-mail].
Revisions: D25845 diff 92706 ← D27439 diff 92707
Details: We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. (255, 'applying /tmp/tmpwKqppc\npatching file gfx/wr/webrender/src/display_list_flattener.rs\nHunk #4 FAILED at 594\nHunk #5 succeeded at 687 with fuzz 2 (offset 58 lines).\nHunk #8 succeeded at 834 with fuzz 1 (offset 58 lines).\nHunk #9 FAILED at 818\nHunk #12 FAILED at 983\n3 out of 14 hunks FAILED -- saving rejects to file gfx/wr/webrender/src/display_list_flattener.rs.rej\nabort: patch failed to apply', '')

Comment 12

[Aria Beingessner [:Gankra]]

first time using lando myself

please work!!!

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/430032511561
cleanup display list code a little bit to prep for refactor. r=gw
https://hg.mozilla.org/integration/autoland/rev/0fc395a2ac71
rearchitect the webrender display-list. r=gw

Comment 14

[Dorel Luca [:dluca]]

Backed out 2 changesets (bug 1536121) for Reftest failures

Log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=241955700&repo=autoland&lineNumber=2420

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=0fc395a2ac7155ffece75daf20d103144374fbfe

Backout:
https://hg.mozilla.org/integration/autoland/rev/02a9bf38ebf276bcd4950a7ff6286e8688da15b3

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/63ab1f265266
cleanup display list code a little bit to prep for refactor. r=gw
https://hg.mozilla.org/integration/autoland/rev/9550416c06ab
rearchitect the webrender display-list. r=gw

Comment 16

[Aria Beingessner [:Gankra]]

was a simple rebase error in flatten_iframe

Comment 17

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/63ab1f265266
https://hg.mozilla.org/mozilla-central/rev/9550416c06ab

Comment 18

[Ionuț Goldan [:igoldan]]

Noticed at least one small perf improvement:

== Change summary for alert #20834 (as of Tue, 07 May 2019 03:11:42 GMT) ==

Improvements:

2% displaylist_mutate linux64-shippable-qr opt e10s stylo 3,971.27 -> 3,901.32

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=20834