896243 - Clean up tpaint / twinopen code

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

[Justin Dolske [:Dolske]]

Attachment: standalone tpaint

I've been poking though the code responsible for our tpaint measurements, and it's a bit of a mess. Nothing obviously _wrong_, just a lot of cruftyness. My favorite is the (unused) SERVER_URL pointing to mcom.com (!).

Specifically, I'm talking about the code here:

http://mxr.mozilla.org/build/source/talos/talos/startup_test/twinopen/

Attached is a significantly cleaned up version. The part I'm mostly unsure of is how to integrate this with our build infra -- who else might be calling the old winopen.xul, how, and exactly what needs to be reported/how.

Who might own an eventual review of this? Or at least be aware of other issues around twiddling this code?

Comment 1

[Justin Dolske [:Dolske]]

A few other notes:

1) Comments indicate a few prefs that need to be set for the test to run. It's otherwise unprivileged. Presumably the test harness would take care of this.

2) The 1000ms delay between window openings (as in existing code) seems like overkill. I'd like to bump this down to 100ms (or even 0). But that can be done in a followup, to limit risk per change.

3) The existing test throws away the first measurement (because, AIUI, it's often abnormally larger). I suspect that larger value is due to the overhead loading the test file from disk the first time. I'm using a data: URI here, and at least at first glance the first measurement is not significantly different than the rest.

4) Switched from Date.getTime() to performance.now(). Shouldn't matter in practice when the overall values are on the order of a hundred ms, but it's easy. Might need to Math.round() some final results, lest our other infra be unhappy with non-integer results.

Comment 2

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

I will take a look at integrating this into talos this week.  We will probably get it rolled onto M-C by the end of the week if the changes are straightforward.

Comment 3

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

Attachment: integrated into talos repository (1.0)

Tested on try server, numbers are stable and the same as prior to this patch.  Thanks for the cleanup here!

Comment 4

[Justin Dolske [:Dolske]]

Comment on attachment 779298 [details] [diff] [review]
integrated into talos repository (1.0)

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

Awesome, thanks for picking up the integration bits. A few comments inline.

::: talos/startup_test/tpaint.html
@@ +15,5 @@
> + * XXX When run in the browser, need to:
> + *     1) allow popups
> + *     2) set dom.send_after_paint_to_content = true
> + *     3) set browser.link.open_newwindow = 2 (else it opens in a tab)
> + * XXX would be good to sanity check that child is in a window, not a tab.

Kill my last XXX. Not worth worrying about.

I'd clarify the first XXX as "XXX When run manually, outside of automation, need to:".

@@ +46,5 @@
> +  } else {
> +    numbers.sort( function (a,b){ return a-b; } );
> +    var n = Math.floor( numbers.length / 2 );
> +    return numbers.length % 2 ? numbers[n] : ( numbers[n-1] + numbers[n] ) / 2;
> +  }

I know this is cut'n'paste, but you could nuke everything but the final else block. I think it's unlikely we'd ever reduce the number of runs down to 2, 1, or zero (!).

@@ +54,5 @@
> +  var min = 99999, max = 0, avg = 0;
> +  // XXX skip first number?
> +  // suspect that this was only higher in original test
> +  // because it was loading from disk.
> +  // XXX compute median too?

These 4 comment lines can go away now.

When you wrote "Tested on try server, numbers are stable", did you specifically check the first number reported (ie, first window opened)? It looked the same as the other runs on my local system, but I wanted to be sure it wasn't going to be weird on build infra or other platforms.

We could continue to skip the first window if we want to be extra paranoid.

@@ +56,5 @@
> +  // suspect that this was only higher in original test
> +  // because it was loading from disk.
> +  // XXX compute median too?
> +  var count = openTimes.length;
> +  for (var i = 0; i < count; i++) {

(would need |var i = 1| here to really skip the first window, as the original test did)

@@ +73,5 @@
> +  if (auto) {
> +    dumpLog("__start_report" + openTimes.join('|') + "__end_report");
> +    var now = (new Date()).getTime();
> +    dumpLog("__startTimestamp" + now + "__endTimestamp\n");
> +    dumpLog("openingTimes="+openTimes.slice(1)+"\n");

Oh, actually, you wouldn't see the first window being wonky because it's not reported here due to the .slice(1). Unless it was so bad as to skew the average/median results. So we should either retest or do a followup bug to remove the first-window skip.

Comment 5

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

cleaned up the nits, verified on try server and removed the slice(1).  All is looking well.  

Landed:
https://hg.mozilla.org/build/talos/rev/b582727872a6

This will be rolled out in our next talos update, probably next week.