cleanup display item representation

RESOLVED FIXED in Firefox 68

Status

()

task
P2
normal
RESOLVED FIXED
2 months ago
11 days ago

People

(Reporter: Gankro, Assigned: Gankro)

Tracking

(Blocks 1 bug)

Trunk
mozilla68
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox67 affected, firefox68 fixed)

Details

(Whiteboard: [wr-q2][wr-april])

Attachments

(2 attachments, 1 obsolete attachment)

Assignee

Description

2 months ago

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

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

Assignee: nobody → a.beingessner
Assignee

Comment 2

2 months ago
  • 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

Whiteboard: [wr-q2][wr-april]
Assignee

Comment 3

a month ago

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.

Assignee

Comment 4

a month ago

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.

  1. 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.

  1. 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

  2. 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%).

Attachment #9055276 - Attachment is obsolete: true
Assignee

Comment 5

a month ago

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.

Assignee

Comment 6

a month ago

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.

Keywords: checkin-needed
Assignee

Comment 7

a month ago

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

Keywords: checkin-needed
Assignee

Comment 8

a month ago

ok we're comfortable with moving forward

Keywords: checkin-needed

: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', '')

Flags: needinfo?(a.beingessner)
Assignee

Comment 10

a month ago

rebased

Flags: needinfo?(a.beingessner)
Keywords: checkin-needed

On Fri, April 19, 2019, 3:10 AM GMT+3, by apavel@mozilla.com.
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 apavel@mozilla.com.
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', '')

Flags: needinfo?(a.beingessner)
Keywords: checkin-needed
Assignee

Comment 12

28 days ago

first time using lando myself

please work!!!

Flags: needinfo?(a.beingessner)

Comment 13

28 days ago
Pushed by abeingessner@mozilla.com:
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 15

27 days ago
Pushed by abeingessner@mozilla.com:
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
Assignee

Comment 16

27 days ago

was a simple rebase error in flatten_iframe

Flags: needinfo?(a.beingessner)
Assignee

Updated

27 days ago
Type: enhancement → task

Comment 17

27 days ago
bugherder
Status: NEW → RESOLVED
Last Resolved: 27 days ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

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

You need to log in before you can comment on or make changes to this bug.