1654398 - ts_paint is using firstPaint to measure time to first paint. It should use firstPaint2 instead.

Status: RESOLVED FIXED

(Testing :: Talos, task, P3)

Description

[Mike Conley (:mconley) (:⚙️)]

This should be using firstPaint2: https://searchfox.org/mozilla-central/rev/3b6958c26049c1e27b2790a43154caaba9f6dd4a/testing/talos/talos/startup_test/tspaint_test.html#29

firstPaint2 was introduced in bug 1556568 as the successor to firstPaint, and is a more accurate measurement. We should use that instead.

Alternatively, we could maybe use this as an opportunity to port ts_paint to mozperftest.

Comment 1

[Brad Werth [:bradwerth]]

I'll do it. This will be an interesting argument with perf-checking automation...

Comment 2

[Brad Werth [:bradwerth]]

Attachment: Bug 1654398: Make ts_paint measurement check firstPaint2 timings instead of firstPaint.

As noted in Bug 1556568, firstPaint records when we complete the first
render that the OS tells us to do (in nsViewManager::Refresh). This
sometimes can happen much later than we would like, skewing results. It
also is sensitive to whether or not the call to FlushRendering is sync
or async, since it fires as soon as the call is complete. For these
reasons, we'd like ts_paint to measure something more consistent, which
this patch accomplishes by changing the measurement to use firstPaint2.

firstPaint2 records in nsView::DidCompositeWindow, which measures that
the pixels have been put on the screen, no matter who asked us to do it.
This solves both problems and is the superior measure.

Because this most always measure ts_paint at a later time than current
code, this will get flagged as a performance regression. We need to
instead treat it as a measurement recalibration.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bb41b62cad53
Make ts_paint measurement check firstPaint2 timings instead of firstPaint. r=perftest-reviewers,sparky

Comment 4

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/bb41b62cad53

Comment 5

[Timothy Nikkel (:tnikkel)]

Surprised we haven't seen a perf alert from this yet, there is a definite jump in the graph exactly when this landed.

https://treeherder.mozilla.org/perfherder/graphs?highlightAlerts=1&highlightChangelogData=1&highlightCommonAlerts=0&replicates=0&series=autoland,4768701,1,1&timerange=1209600&zoom=1711027493824,1711065063313,393.5213633246192,478.21056605794365

Comment 6

[Brad Werth [:bradwerth]]

Maybe perf team is running interference on it, disabled the alert.

Comment 7

[Greg Mierzwinski [:sparky]]

It takes time for the alert to be produced, and then it takes additional time for a sheriff to investigate it. The alert has been produced, and is being triaged now.