implement PerformancePaintTiming
Categories
(Core :: Performance, enhancement, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox84 | --- | fixed |
People
(Reporter: 709922234, Assigned: sefeng)
References
()
Details
(Keywords: dev-doc-complete, perf-alert, Whiteboard: , [wptsync upstream])
Attachments
(8 files, 2 obsolete files)
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details |
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:66.0) Gecko/20100101 Firefox/66.0
Updated•6 years ago
|
Updated•6 years ago
|
Comment 1•6 years ago
|
||
Moving to Performance module as it's part of performance API. Feel free to reset if I got it wrong.
Updated•5 years ago
|
Updated•5 years ago
|
We're very glad to see some movement on this. Is this being worked on actively by Mozilla now? Any planned timeline for this project?
Comment 3•5 years ago
|
||
Chrome / Blink has been shipping this API for a while, and WebKit is starting its implementation in https://webkit.org/b/78011. The current plan is to just implement first contentful paint for now.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 4•5 years ago
|
||
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 5•5 years ago
|
||
Some tests made some assumptions about the number of returned entries
by performance.getEntries, and these assumptions are not valid
anymore once we added new entries.
Depends on D66463
Assignee | ||
Comment 6•5 years ago
|
||
In this patch, we changed a lot of tests from PRECONDITION_FAILED to
timeout because we added the PerformancePaintTiming API so the
precodition is no longer failed. TIMEOUT is required because
these tests expect two performance paint entries, but we only
support one of them.
Depends on D68645
Assignee | ||
Comment 7•5 years ago
|
||
Depends on D68646
Updated•5 years ago
|
Assignee | ||
Comment 8•5 years ago
|
||
The current tests workflow works as wait for three frames, then
check if the performance entry shows up. This workflow is flaky
because the three frames piece is hardcoded and there are
chances that the paint takes longer than three frames because it
really depends on the current workload of the system.
We fix it by loading the image into memory first, so that it's
decoded before the painting.
Depends on D68647
Comment 9•5 years ago
|
||
(In reply to Sean Feng [:sefeng] from comment #8)
The current tests workflow works as wait for three frames, then
check if the performance entry shows up. This workflow is flaky
because the three frames piece is hardcoded and there are
chances that the paint takes longer than three frames because it
really depends on the current workload of the system.We fix it by loading the image into memory first, so that it's
decoded before the painting.
Can you propose it as an upstream change to WPT if you haven't already?
Comment 10•5 years ago
|
||
Changes to WPT in mozilla-central get upstreamed automatically, fwiw.
Updated•5 years ago
|
Assignee | ||
Comment 11•5 years ago
|
||
(In reply to Noam Rosenthal from comment #9)
(In reply to Sean Feng [:sefeng] from comment #8)
The current tests workflow works as wait for three frames, then
check if the performance entry shows up. This workflow is flaky
because the three frames piece is hardcoded and there are
chances that the paint takes longer than three frames because it
really depends on the current workload of the system.We fix it by loading the image into memory first, so that it's
decoded before the painting.Can you propose it as an upstream change to WPT if you haven't already?
Yup, as what Emilio said :)
Updated•4 years ago
|
Assignee | ||
Comment 12•4 years ago
|
||
Depends on D68888
Comment 13•4 years ago
|
||
Comment 15•4 years ago
|
||
Backed out for marionette failures on test_refresh_firefox.py
backout: https://hg.mozilla.org/integration/autoland/rev/8ede6180a0d054f3bca8957c6bcd26d5339eeedb
failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=313354842&repo=autoland&lineNumber=62370
[task 2020-08-18T20:28:00.139Z] 20:28:00 INFO - 1597782480133 Marionette DEBUG 3 <- [1,16,null,{"value":null}]
[task 2020-08-18T20:28:00.140Z] 20:28:00 ERROR - TEST-UNEXPECTED-ERROR | browser/components/migration/tests/marionette/test_refresh_firefox.py TestFirefoxRefresh.testFxANoSync | KeyError: 'keyFetchToken'
[task 2020-08-18T20:28:00.140Z] 20:28:00 INFO - Traceback (most recent call last):
[task 2020-08-18T20:28:00.140Z] 20:28:00 INFO - File "Z:\task_1597781383\build\venv\lib\site-packages\marionette_harness\marionette_test\testcases.py", line 196, in run
[task 2020-08-18T20:28:00.140Z] 20:28:00 INFO - testMethod()
[task 2020-08-18T20:28:00.141Z] 20:28:00 INFO - File "Z:\task_1597781383\build\tests\marionette\tests\browser\components\migration\tests\marionette\test_refresh_firefox.py", line 609, in testFxANoSync
[task 2020-08-18T20:28:00.141Z] 20:28:00 INFO - self.checkFxA()
[task 2020-08-18T20:28:00.141Z] 20:28:00 INFO - File "Z:\task_1597781383\build\tests\marionette\tests\browser\components\migration\tests\marionette\test_refresh_firefox.py", line 423, in checkFxA
[task 2020-08-18T20:28:00.141Z] 20:28:00 INFO - self.assertEqual(result["accountData"]["keyFetchToken"], "top-secret")
[task 2020-08-18T20:28:00.141Z] 20:28:00 INFO - TEST-INFO took 15656ms
[task 2020-08-18T20:28:00.149Z] 20:28:00 INFO - 1597782480138 Marionette DEBUG 3 -> [0,17,"Marionette:GetContext",{}]
[task 2020-08-18T20:28:00.149Z] 20:28:00 INFO - 1597782480138 Marionette DEBUG 3 <- [1,17,null,{"value":"chrome"}]
[task 2020-08-18T20:28:00.149Z] 20:28:00 INFO - 1597782480140 Marionette DEBUG 3 -> [0,18,"WebDriver:DeleteSession",{}]
[task 2020-08-18T20:28:00.150Z] 20:28:00 INFO - 1597782480142 Marionette DEBUG 3 <- [1,18,null,{"value":null}]
[task 2020-08-18T20:28:00.165Z] 20:28:00 INFO - Exiting due to channel error.
[task 2020-08-18T20:28:00.180Z] 20:28:00 INFO - [GFX1-]: Receive IPC close with reason=AbnormalShutdown
[task 2020-08-18T20:28:00.180Z] 20:28:00 INFO - Exiting due to channel error.
Comment 16•4 years ago
|
||
Comment 18•4 years ago
|
||
Comment 19•4 years ago
|
||
Backed out for geckoview failures on scrollToVerticalOnZoomedContentAuto.
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=313888836&repo=autoland
Backout link: https://hg.mozilla.org/integration/autoland/rev/81a9b77172db760892dc00fd5d3c9c6fa0dd891f
Assignee | ||
Comment 20•4 years ago
|
||
Our current code consider gradient-only backgrounds
as contentful, however they aren't contentful according
to the latest spec.
Updated•4 years ago
|
Comment 21•4 years ago
|
||
Comment 22•4 years ago
|
||
Backed out for causing browsertime failures.
Failure logs: https://treeherder.mozilla.org/logviewer.html#?job_id=314251643&repo=autoland
https://treeherder.mozilla.org/logviewer.html#?job_id=314251648&repo=autoland
https://treeherder.mozilla.org/logviewer.html#?job_id=314251652&repo=autoland
Backout link: https://hg.mozilla.org/integration/autoland/rev/fdb0f33f3b56fa70106a937dc00888223c23ac4e
Assignee | ||
Comment 23•4 years ago
|
||
Browsertime failures are caused by https://github.com/sitespeedio/browsertime/pull/1347. Will need to wait a bit for it gets fixed.
Assignee | ||
Comment 24•4 years ago
|
||
I want to take this opportunity to discuss whether we should stick with timeToContentfulPaint
or switch to first-contentful-paint
for browsertime after the patches are landed.
A brief background is first-contentful-paint
is what this bug introduced, it's exposed to the web publicly and this is the timestamp during displayList building about when we found a contentful frame. timeToContentfulPaint
is behind a pref (default is false) and this is the timestamp in the compositor level about when we composited the transactions.
If we want to stick with timeToContentful
, we need to make a slight modification to https://searchfox.org/mozilla-central/rev/d54712b9644b49cec6cc90a9e0c325fdfab04e7c/testing/raptor/raptor/results.py#506-510, otherwise raptor will start to use first-contentful-paint
automatically which may provide some unexpected numbers.
The reason for making them separate is because we don't want to expose accurate timestamps to the web which may inferences the complexity of the content.
Using timeToContentful
is better because it's more accurate to when the frame is presented on the screen.
If we can agree on using timeToContentful
, I can make the patch to fix raptor.
Thoughts sparky, Andrew?
Comment 25•4 years ago
|
||
(In reply to Sean Feng [:sefeng] from comment #24)
The reason for making them separate is because we don't want to expose accurate timestamps to the web which may inferences the complexity of the content.
Hi Sean. I don't understand exactly why timeToContentfulPaint
is disabled by default.
It's not a privacy issue, right?
Does it have a negative performance impact?
Comment 26•4 years ago
|
||
Note that by the spec, the timestamp should represent the time when "Update the rendering" occurs: https://html.spec.whatwg.org/#update-the-rendering, section 14, and not a time representing a subsequent internal operation.
This is equivalent to timing in IntersectionObservers... So when the first contentful element appears on the page, it will trigger the IntersectionObserver and the paint timing API with the exact same timestamp.
That's also what is done in WebKit.
Comment 27•4 years ago
|
||
... This was done to avoid exposing internal timestamps to the web, which can lead to all kinds of interesting timing-attacks.
For interpoerability, I would suggest to either conform to that or discuss here: https://github.com/w3c/paint-timing/issues/62
Updated•4 years ago
|
Comment 28•4 years ago
|
||
(In reply to Noam Rosenthal from comment #27)
... This was done to avoid exposing internal timestamps to the web, which can lead to all kinds of interesting timing-attacks.
For interpoerability, I would suggest to either conform to that or discuss here: https://github.com/w3c/paint-timing/issues/62
So that sounds fine from the point of view of spec compliance and interop, though at least for the purposes of intersection observer and so on that seems a bit silly as one can get the high resolution timestamp anyways via performance.now()
, right? What am I missing there?
Comment 29•4 years ago
|
||
(In reply to Emilio Cobos Álvarez (:emilio) from comment #28)
(In reply to Noam Rosenthal from comment #27)
... This was done to avoid exposing internal timestamps to the web, which can lead to all kinds of interesting timing-attacks.
For interpoerability, I would suggest to either conform to that or discuss here: https://github.com/w3c/paint-timing/issues/62So that sounds fine from the point of view of spec compliance and interop, though at least for the purposes of intersection observer and so on that seems a bit silly as one can get the high resolution timestamp anyways via
performance.now()
, right? What am I missing there?
You can only get the hires timestamp when you're in the JS event loop, but not between internal browser operations.
What I meant about intersection observers, is that conceptually they should have the same timestamp as paint timing - as the elements "intersect" for the user when they're painted.
Keeping all these timestamps aligned with the standard "update the rendering" phase is not of huge importance but makes it possible to inform the developer what the timestamp stands for in a transparent and interoperable way
Comment 30•4 years ago
|
||
Sean (and Greg can correct me if I'm wrong), I believe that those raptor tests will be replaced by Browsertime tests.
If the difference is small (which it was in the case that I saw), I can see the advantage of having the timestamps aligned and using first-contentful-paint
.
Assignee | ||
Comment 31•4 years ago
|
||
If the difference is small (which it was in the case that I saw), I can see the advantage of having the timestamps aligned and using first-contentful-paint.
Thanks, Andrew, sounds good. I'll run some browsertime tests to more sites so that we can be confident here.
Keeping all these timestamps aligned with the standard "update the rendering" phase is not of huge importance but makes it possible to inform the developer what the timestamp stands for in a transparent and interoperable way
Thanks, Noam, and yeah agreed, I am making the change to conform that, so we will return the same time as IntersectionObservers.
Comment 32•4 years ago
|
||
The part that :sefeng points to will be staying after the migration since it parses the results from the browsertime.json files.
As long as the difference between what we have now and what we will have is a systemic error (or negligible) across all test pages then I don't see an issue with these changes.
Assignee | ||
Comment 33•4 years ago
|
||
Updated•4 years ago
|
Assignee | ||
Comment 34•4 years ago
|
||
I did a smoke browsertime runs to see the differences between FCP and timeToContentfulPaint. Link
Most of the sites report a 2-5% difference, some of the highest are like a 9% difference. Although the samples are small, I'd still assume that a 3% - 5% improvement to FCP is going to appear in our test result once we land the FCP patches. The 9% ones look odd though, maybe it indicates a performance issue that we can improve, the performance between displayList building to the actual paints.
Comment 35•4 years ago
|
||
To avoid confusion, I think we should also rename timeToContentfulPaint to first-contentful-composite; this will make the difference clearer. So then the question of which one we should use in browsertime really comes down to the following question: Do we want capture improvements and regressions in the graphics pipeline in the browsertime tests? If we do, then we need to use first-contentful-composite.
Or we could capture both timestamps: first-contentful-paint for comparison with Chrome, and first-contentful-composite as a Firefox-specific value to optimize against.
Assignee | ||
Comment 36•4 years ago
|
||
Bump the browsertime hash to add a fix from upstream which fixes a
firstPaint related bug when FCP is supported.
Assignee | ||
Comment 37•4 years ago
|
||
Since this bug, Firefox will start to use the same conversion as
Chrome for reporting FCP. This patch cleans our code a little bit
to adapt that.
Depends on D90108
Assignee | ||
Comment 38•4 years ago
|
||
I agree with what Markus said. I think renaming timeToContentfulPaint
to first-contentful-composite
, capturing both to use first-contentful-paint to compare with Chrome and keep first-contentful-composite
as a Firefox-specific value for optimization is a good idea.
Comment 39•4 years ago
|
||
Comment 41•4 years ago
|
||
Backed out 10 changesets (bug 1518999) for GeckoView failures in PanZoomControllerTest.scroll. CLOSED TREE
Log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=315891350&repo=autoland&lineNumber=8254
Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&revision=7b5bdd071d05f5bb4a7ce794cd73bcfa63a4e27c
Backout:
https://hg.mozilla.org/integration/autoland/rev/cf9dc3cf3bf9e0c15404735951aa583c4cfc72fd
Comment 42•4 years ago
|
||
Initial commit alert:
== Change summary for alert #27073 (as of Mon, 28 Sep 2020 06:18:11 GMT) ==
Improvements:
4% espn fcp android-hw-g5-7-0-arm7-api-16-shippable opt warm 3,451.19 -> 3,299.62
4% espn fcp android-hw-g5-7-0-arm7-api-16-shippable opt warm 3,508.47 -> 3,374.33
3% google-maps FirstVisualChange android-hw-g5-7-0-arm7-api-16-shippable opt cold 1,119.12 -> 1,083.08
3% google-maps SpeedIndex android-hw-g5-7-0-arm7-api-16-shippable opt cold 1,203.33 -> 1,164.92
3% espn FirstVisualChange android-hw-g5-7-0-arm7-api-16-shippable opt warm 3,522.42 -> 3,416.58
3% espn ContentfulSpeedIndex android-hw-g5-7-0-arm7-api-16-shippable opt warm 3,536.85 -> 3,432.25
3% google-maps PerceptualSpeedIndex android-hw-g5-7-0-arm7-api-16-shippable opt cold 1,203.92 -> 1,168.42
3% google-maps android-hw-g5-7-0-arm7-api-16-shippable opt warm 967.32 -> 940.49
3% instagram loadtime android-hw-g5-7-0-arm7-api-16-shippable opt warm 2,763.48 -> 2,688.79
3% instagram SpeedIndex android-hw-g5-7-0-arm7-api-16-shippable opt warm 2,577.35 -> 2,512.58
2% instagram PerceptualSpeedIndex android-hw-g5-7-0-arm7-api-16-shippable opt warm 1,487.05 -> 1,455.58
2% instagram LastVisualChange android-hw-g5-7-0-arm7-api-16-shippable opt warm 2,807.70 -> 2,749.17
For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=27073
Updated•4 years ago
|
Updated•4 years ago
|
Updated•4 years ago
|
Comment 44•4 years ago
|
||
Updated•4 years ago
|
Comment 45•4 years ago
|
||
Backed out 10 changesets (bug 1654103, bug 1672023, bug 1518999) for PanZoomControllerTest.touchEventForResult gv-junit failures.
Backout link: https://hg.mozilla.org/integration/autoland/rev/9006d6f3cb29754037aa0b5ef4c9b2ae67006459
Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=319608253&repo=autoland&lineNumber=8614
Assignee | ||
Comment 47•4 years ago
|
||
Sorry about missing another junit failure. I think I found the issue and quite confident that I should get everything covered this time. Try push.
Will try to land the patches again on Monday.
Comment 48•4 years ago
|
||
Comment 49•4 years ago
|
||
Comment 50•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3fbb53f6d6ef
https://hg.mozilla.org/mozilla-central/rev/47629f89cb6a
https://hg.mozilla.org/mozilla-central/rev/18977625f8a4
https://hg.mozilla.org/mozilla-central/rev/434cacd927bf
https://hg.mozilla.org/mozilla-central/rev/b61d813af80d
https://hg.mozilla.org/mozilla-central/rev/0af7c44c8e72
https://hg.mozilla.org/mozilla-central/rev/412e72819153
https://hg.mozilla.org/mozilla-central/rev/ea8ed7d8298a
https://hg.mozilla.org/mozilla-central/rev/00cc34581b34
Comment 51•4 years ago
|
||
== Change summary for alert #27482 (as of Tue, 03 Nov 2020 06:07:29 GMT) ==
Improvements:
Ratio | Suite | Test | Platform | Options | Absolute values (old vs new) |
---|---|---|---|---|---|
16% | youtube | fcp | android-hw-g5-7-0-arm7-api-16-shippable | warm | 906.58 -> 762.88 |
16% | fcp | android-hw-g5-7-0-arm7-api-16-shippable | warm | 358.10 -> 301.62 | |
15% | fcp | android-hw-g5-7-0-arm7-api-16-shippable | cold | 459.19 -> 389.46 | |
13% | youtube | fcp | android-hw-g5-7-0-arm7-api-16-shippable | cold | 1,245.27 -> 1,088.21 |
11% | youtube | fcp | android-hw-g5-7-0-arm7-api-16-shippable | cold | 1,240.33 -> 1,098.25 |
6% | youtube | android-hw-g5-7-0-arm7-api-16-shippable | warm | 694.03 -> 652.98 | |
5% | android-hw-g5-7-0-arm7-api-16-shippable | warm | 645.54 -> 610.41 | ||
5% | android-hw-g5-7-0-arm7-api-16-shippable | cold | 820.02 -> 783.07 |
For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=27482
Comment 52•4 years ago
|
||
Adding dev-doc-needed.
For whoever documents this — the PerformancePaintTiming interface is already documented, I believe this just requires the docs to be checked, and BCD to be updated to include Fx/FxA support information when the time comes.
Comment 54•4 years ago
|
||
Looks like the docs for this are pretty much done.
See https://github.com/mdn/sprints/issues/3900 for the full details.
Updated•3 years ago
|
Description
•