Closed Bug 725748 Opened 8 years ago Closed 8 years ago

Measure MozAfterPaint times in capture phase

Categories

(Testing :: Talos, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ttaubert, Assigned: jmaher)

References

Details

Attachments

(1 file)

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.
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.
Just a matter of passing "true" instead of "false" as the third parameter to addEventListener.
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?
Assignee: nobody → jmaher
Status: NEW → ASSIGNED
Attachment #597266 - Flags: review?(ttaubert)
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).
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/jmaher@mozilla.com-48215a782c33
Comment on attachment 597266 [details] [diff] [review]
fix mozafterpaint (1.0)

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

Thanks!
Attachment #597266 - Flags: review?(ttaubert) → review+
Oh, while I actually think this is a safe fix, I'm not sure if I can give r+ for talos test modifications?
Attachment #597266 - Flags: review?(jhammel)
do we also want this for tp5 (and related tests)?
Comment on attachment 597266 [details] [diff] [review]
fix mozafterpaint (1.0)

wfm
Attachment #597266 - Flags: review?(jhammel) → review+
http://hg.mozilla.org/build/talos/rev/7733a8494f97
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Are these changes deployed, or just checked-in?
I never received an answer to comment #8.  These are checked in, but not deployed.
(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?
I think we want it for all performance tests that use MozAfterPaint.
it looks like the pageloader bits already do this, so we just need to deploy this change for ts, ts_paint.
(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?
in bug 731893 I filed for a new talos.zip
Depends on: 731893
You need to log in before you can comment on or make changes to this bug.