1430494 - MergeDisplayLists optimization: Skip new-list loop and associated operations when list is empty

Status: RESOLVED FIXED

(Core :: Web Painting, defect, P2)

Description

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

The main loop in MergeDisplayLists goes through aNewList items. If that list is empty, the loop exits immediately, as expected.
However there is still some work happening before the loop (E.g.: lookup tables being built, reused bit being reset on old items) that could also be avoided.

Some early measurements indicate it could reduce the display list merging time by 20%! (Tested on one machine, with the "maze" page from bug 1409047).
I'll do more tests before landing -- lesson from bug 1426044 comment 21.

Comment 1

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1430494 - Skip new-list loop and its related operations if list is empty -

Review commit: https://reviewboard.mozilla.org/r/212832/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/212832/

Comment 2

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8942575 [details]
Bug 1430494 - Skip new-list loop and its related operations if list is empty -

https://reviewboard.mozilla.org/r/212832/#review218458

::: layout/painting/RetainedDisplayListBuilder.cpp:476
(Diff revision 1)
> -  }
> +    }
> +  }
>  
>    // Reuse the remaining valid items from the old display list.
>    while (nsDisplayItem* old = aOldList->RemoveBottom()) {
>      if (!IsAnyAncestorModified(old->FrameForInvalidation())) {

Matt, in bug 1420737 you added `&& !old->IsReused()` to the test here.
Because this patch skips the SetReused loop on old items if the new list is empty, the added test would have to change to `&& (!old->IsReused() || aNewList->IsEmpty())`.

Comment 3

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8942575 [details]
Bug 1430494 - Skip new-list loop and its related operations if list is empty -

https://reviewboard.mozilla.org/r/212832/#review219054

Comment 4

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Thank you Matt for the review.

Testing RDL-merging times on Try, I'm getting mostly zero-to-small improvements, but also in some cases 10-20% improvements.
So I think it's worth landing this small change.

I'll wait for bug 1420737 to land in central, as it's touching the same code, and will require a non-trivial rebase as explained in comment 2.

Comment 5

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8942575 [details]
Bug 1430494 - Skip new-list loop and its related operations if list is empty -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/212832/diff/1-2/

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/50a5f0a08899
Skip new-list loop and its related operations if list is empty - r=mattwoodrow

Comment 7

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/115899fa5acc
Backed out changeset 50a5f0a08899 for dependency with bug 1420737 that causes merge conflict on CLOSED TREE

Comment 8

[Tiberius Oros[:tiberius_oros]]

Backed out for dependency with bug 1420737 that causes merge conflict

https://hg.mozilla.org/integration/autoland/rev/115899fa5acc514905bcdab3f8d8c67c4c8d92a5

https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=50a5f0a088995c5e5fce91aab24306148491bec9

Comment 9

[Natalia Csoregi [:nataliaCs]]

The failures which contained assertion counts are about the displaylist present in this patch.
Caused gl failures, for example - https://treeherder.mozilla.org/logviewer.html#?job_id=156981773&repo=autoland&lineNumber=2352.
With the backout done for conflicting with the merges, there were no more FrameLayerBuilder.cpp assertion issues

Comment 10

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Thank you Natalia for reporting the problem. I've found the issue (`aNewList` was being modified, so I couldn't rely on it staying empty later on.)

Fix looks good in try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0468efaa51be5ad8931367f06131554c2d6f903

Will reland soon.

Comment 11

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8942575 [details]
Bug 1430494 - Skip new-list loop and its related operations if list is empty -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/212832/diff/2-3/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0e00d010cae3
Skip new-list loop and its related operations if list is empty - r=mattwoodrow

Comment 13

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/0e00d010cae3