Closed Bug 1430494 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: Web Painting, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox59 --- wontfix
firefox60 --- fixed

People

(Reporter: gerald, Assigned: gerald)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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 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 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
Attachment #8942575 - Flags: review?(matt.woodrow) → review+
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.
Depends on: 1420737
Priority: -- → P2
Pushed by gsquelart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/50a5f0a08899
Skip new-list loop and its related operations if list is empty - r=mattwoodrow
Backout by toros@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/115899fa5acc
Backed out changeset 50a5f0a08899 for dependency with bug 1420737 that causes merge conflict on CLOSED TREE
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
Flags: needinfo?(gsquelart)
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.
Flags: needinfo?(gsquelart)
Pushed by gsquelart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0e00d010cae3
Skip new-list loop and its related operations if list is empty - r=mattwoodrow
https://hg.mozilla.org/mozilla-central/rev/0e00d010cae3
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.