725748 - Measure MozAfterPaint times in capture phase

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

Bug 715402 regresses MozAfterPaint measurements but only because delayedStartup() blocks subsequent event handlers (not because MozAfterPaint is fired later).

talos/startup_test/tspaint_test.html
talos/startup_test/twinopen/tpaint-window.html

These tests should register MozAfterPaint event handlers for the capturing phase so that other event handlers are executed afterwards.

Comment 1

[Joel Maher ( :jmaher ) (UTC -8)]

is there an example of how to register a MozAfterPaint event handler?  We do this in the pageloader code as well, so I want to make sure I do the right thing here.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Just a matter of passing "true" instead of "false" as the third parameter to addEventListener.

Comment 3

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: fix mozafterpaint (1.0)

So this is a really simple patch, but it doesn't account for all the pageloader data.  In addition, this will probably change the existing numbers that we are collecting which means we need to do a whole round of side by side staging.  We just did some side by side staging and haven't even changed the values on tbpl yet, so a lot more churn and confusion for everybody.

Do we want this same stuff for the pageloader tests (tdhtml, tp5, tsspider)?  
Do we care about any of the old numbers?  Can we just accept a shift in the numbers?

Comment 4

[:Gavin Sharp [email: [e-mail]]]

In theory that shouldn't have much impact on the numbers, because there currently should be no other MozAfterPaint handlers involved, so the only difference in workload is a little bit of extra time in event dispatch code (should be negligible).

Comment 5

[Mozilla RelEng Bot]

Try run for 48215a782c33 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=48215a782c33
Results (out of 55 total builds):
    success: 55
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-48215a782c33

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 597266 [details] [diff] [review]
fix mozafterpaint (1.0)

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

Thanks!

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Oh, while I actually think this is a safe fix, I'm not sure if I can give r+ for talos test modifications?

Comment 8

[Joel Maher ( :jmaher ) (UTC -8)]

do we also want this for tp5 (and related tests)?

Comment 9

[Jeff Hammel]

Comment on attachment 597266 [details] [diff] [review]
fix mozafterpaint (1.0)

wfm

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

http://hg.mozilla.org/build/talos/rev/7733a8494f97

Comment 11

[Dietrich Ayala (:dietrich)]

Are these changes deployed, or just checked-in?

Comment 12

[Joel Maher ( :jmaher ) (UTC -8)]

I never received an answer to comment #8.  These are checked in, but not deployed.

Comment 13

[Dietrich Ayala (:dietrich)]

(In reply to Joel Maher (:jmaher) from comment #12)
> I never received an answer to comment #8.  These are checked in, but not
> deployed.

Does anyone know who can answer that?

Comment 14

[:Gavin Sharp [email: [e-mail]]]

I think we want it for all performance tests that use MozAfterPaint.

Comment 15

[Joel Maher ( :jmaher ) (UTC -8)]

it looks like the pageloader bits already do this, so we just need to deploy this change for ts, ts_paint.

Comment 16

[Dietrich Ayala (:dietrich)]

(In reply to Joel Maher (:jmaher) from comment #15)
> it looks like the pageloader bits already do this, so we just need to deploy
> this change for ts, ts_paint.

Ok, thaks. What needs to happen for that? Should we file a separate bug, or can that happen here?

Comment 17

[Joel Maher ( :jmaher ) (UTC -8)]

in bug 731893 I filed for a new talos.zip