Closed Bug 1411804 Opened 3 years ago Closed 3 years ago

Add performance tests for display list construction

Categories

(Testing :: Talos, enhancement)

Version 3
enhancement
Not set
normal

Tracking

(firefox58 fixed)

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: dvander, Assigned: dvander)

References

Details

Attachments

(1 file, 1 obsolete file)

Display list construction is the biggest bottleneck in our painting code. Luckily, the Layout team is about to enable "retained display lists" which should give us major performance improvements. The idea is that after the initial pageload, the display list can be incrementally modified every frame rather than rebuilt from scratch.

Unfortunately Talos's pageload tests won't capture these changes - we won't see any benefit, and we won't be able to see whether the new algorithm regresses in the future.

We do have Telemetry probes, and we expect those numbers to move significantly. But since this project is a big deal we should have at least one performance test for it.
Attached patch patchSplinter Review
Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8af7c706722253d1796521cdab5e06590a46cbb7

I added the test to g2 since that seems to run on Windows. The average runtime looks to be 20 seconds, and it runs for 5 iterations.
Attachment #8922152 - Attachment is obsolete: true
Attachment #8922216 - Flags: review?(jmaher)
Attachment #8922216 - Flags: review?(matt.woodrow)
Comment on attachment 8922216 [details] [diff] [review]
patch

Review of attachment 8922216 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good from the test perspective, should be stressing the display list building pretty extensively. Great to have coverage of this in the tree!
Attachment #8922216 - Flags: review?(matt.woodrow) → review+
as a note, this looks good, I did some retriggers and am waiting for results to come in, there is a large backlog on windows7 and linux right now.  I would prefer to put this test in 'g4' as that is a job that runs faster than 'g2'.
Comment on attachment 8922216 [details] [diff] [review]
patch

Review of attachment 8922216 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/talos/talos.json
@@ +30,5 @@
>              "talos_options": ["--disable-stylo"],
>              "pagesets_name": "tp5n.zip"
>          },
>          "g2-e10s": {
> +            "tests": ["damp", "tps", "displaylist_mutate"],

please put this in g4-e10s as that has a much shorter runtime than g2.  The retriggers and results look good!
Attachment #8922216 - Flags: review?(jmaher) → review+
Pushed by danderson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9421a340f682
Add a Talos test for displaylist mutation. (bug 1411804, r=jmaher, r=mattwoodrow)
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/39ad112131fe
Add a Talos test for displaylist mutation. . Follow-up: Add empty line to fix linting issue. r=me DONTBUILD
Backed out for failing eslint in testing/talos/talos/tests/layout/benchmarks/displaylist_mutate.html:

https://hg.mozilla.org/integration/mozilla-inbound/rev/c2dc77f89b472c80c6cf3d15a5cedfda4df3de25

Push with failures (the fix for the flake8 has been folded into the backout): https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=9421a340f6821851da7f72c3ff18fc306a1d4aa1&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=retry&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=140286005&repo=mozilla-inbound

TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/testing/talos/talos/tests/layout/benchmarks/displaylist_mutate.html:26:5 | 'i' is already defined. (no-redeclare)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/testing/talos/talos/tests/layout/benchmarks/displaylist_mutate.html:34:8 | Infix operators must be spaced. (space-infix-ops)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/testing/talos/talos/tests/layout/benchmarks/displaylist_mutate.html:36:15 | use performance.now() instead of new Date() for timing measurements (mozilla/avoid-Date-timing)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/testing/talos/talos/tests/layout/benchmarks/displaylist_mutate.html:55:56 | Strings must use doublequote. (quotes)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/testing/talos/talos/tests/layout/benchmarks/displaylist_mutate.html:62:13 | use performance.now() instead of new Date() for timing measurements (mozilla/avoid-Date-timing)
Flags: needinfo?(dvander)
Pushed by danderson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7a32fbcb477
Add a Talos test for displaylist mutation. (bug 1411804, r=jmaher, r=mattwoodrow)
Pushed by danderson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8eb20e6a5e5
Add a Talos test for displaylist mutation. (bug 1411804, r=jmaher, r=mattwoodrow)
Flags: needinfo?(dvander)
https://hg.mozilla.org/mozilla-central/rev/f8eb20e6a5e5
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.