612190 - startup talos test should include browser paint

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

[Timothy Nikkel (:tnikkel)]

While investigating a Tshutdown regression from bug 601547 I realized that corresponding startup test doesn't include any painting of the browser at all (see bug 601547 comment 25).

Our startup tests should include a paint because the user doesn't see anything until a paint happens. Do we have any such test?

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

I'd vote to add this as an additional test. Ts tests how long it takes to launch the app and dump output from the content window onload event. This is valuable on it's own. Adding paint time into that could make finding non-painting issues harder.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Make MozAfterPaint actually fire immediately after paint

This needs to be try-tested, but I think it's the right thing. MozAfterPaint fires synchronously at the end of painting --- unless a 100ms timer expires first, in which case we fire it then. (This latter handles cases where an invalidation doesn't actually lead to a followup paint, perhaps because the content is offscreen, covered, etc.)

This changes the timing of the MozAfterPaint, but other than that it shouldn't change anything.

We can probably do away with the timer stuff once all painting is happening synchronously under the refresh driver.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

With that patch, assuming it works, it would be very easy to modify the Txul harness (or create a new Txul variant) to ensure the test finishes after at least one MozAfterPaint has fired.

Comment 4

[Timothy Nikkel (:tnikkel)]

For the record it was Ts where I observed no paint happening during the timed portion of the test.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Right. We should support both.

Comment 6

[Joe Drew (not getting mail)]

We should make Txul and ts have sub tests, in the same way tp and tdhtml do. Then we get both one number and the ability to have it broken out.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That is a very good idea!

Comment 8

[Jim Mathies [:jimm]]

Do any of our Talos slaves have hardware acceleration? Or will these changes only record gdi based paint timing?

Comment 9

[Joe Drew (not getting mail)]

All of our Win7 talos machines are hardware accelerated D3D10/D2D, and WinXP are D3D9.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'm having a little trouble parsing comment 2.  Which parts of it describe the old behavior and which parts of it describe the new behavior?  And what exactly is this patch trying to change?

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The entire comment describes the new behavior.

The old behavior is that the first Invalidate for the document since the previous MozAfterPaint fired dispatches an XPCOM event to asynchronously fire MozAfterPaint.

The new behavior is that first Invalidate for the document since the previous MozAfterPaint fired causes a MozAfterPaint to be fired synchronously at the end of processing the next platform paint event for the window containing the document, OR off a timer 100ms from now, whichever is earlier.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #11)
> The new behavior is that first Invalidate for the document since the previous
> MozAfterPaint fired causes a MozAfterPaint to be fired synchronously at the end
> of processing the next platform paint event for the window containing the
> document

To be precise, for *any* window containing content for the document (e.g. painting a select dropdown will dispatch any pending MozAfterPaint events for the whole document tree).

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 491704 [details] [diff] [review]
Make MozAfterPaint actually fire immediately after paint

~nsRootPresContext should just call CancelDidPaintTimer() rather than
writing its contents out by hand.

Did you intend that you cancel the timer in the middle of when it's firing; it looks like the current code does that.  I suppose it's ok.

r=dbaron.  Sorry for the delay.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #13)
> ~nsRootPresContext should just call CancelDidPaintTimer() rather than
> writing its contents out by hand.

Done.

> Did you intend that you cancel the timer in the middle of when it's firing; it
> looks like the current code does that.  I suppose it's ok.

Yes.

Comment 15

[Jim Mathies [:jimm]]

Requesting blocking so this can land. This blocks blocking bug 594821.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

a=me. It feels oogy approving my own patch, but it should obviously land.

Please do a tryserver run though. It could affect reftests.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

a=dbaron as well

Comment 18

[Jim Mathies [:jimm]]

Pushed to try for a full test run on all platforms.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

What were the results?

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Ah, it looks like there weren't any results!

Comment 21

[Jim Mathies [:jimm]]

(In reply to comment #20)
> Ah, it looks like there weren't any results!

I ran it a second time, there's a build error but I haven't had a chance to look at it yet.

Comment 22

[Jim Mathies [:jimm]]

Looks like those last two lines should be 

frame->PresContext()->NotifyDidPaintForSubtree()?

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It built for me, see http://hg.mozilla.org/try/rev/28177ea3b6ea

Comment 24

[Jim Mathies [:jimm]]

(In reply to comment #23)
> It built for me, see http://hg.mozilla.org/try/rev/28177ea3b6ea

Cool, the patch you posted was missing a couple changes.

Comment 25

[Timothy Nikkel (:tnikkel)]

See also bug 522375 which added code to track when the first paint happens.

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/mozilla-central/rev/43a6717878fa

It should now be pretty easy to modify Txul to report the time after the first paint of the window.

Comment 27

[Tomas]

So this bug is fixed?

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

No. Someone still needs to do what I said in comment #26.

Comment 29

[Jim Mathies [:jimm]]

(In reply to comment #28)
> No. Someone still needs to do what I said in comment #26.

I've got a modified version of winopen.xul I'm working with. I'll post it here in a bit. I've been doing some experimenting with the patch in bug 594821, I don't see any improvement in MozAfterPaint numbers but I get improvement in internal numbers tied to show -> first paint on the window. So I need to do some more experimenting. I do see the txul regression though. Not sure yet what's going on.

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Ehsan, can you look into this?

Comment 31

[Jim Mathies [:jimm]]

Attachment: modified version of winopen.xul

This is what I've been working with, it's basically the txul test modified to run on the desktop and report mozafterpaint numbers in the same results form.

Comment 32

[(no longer active)]

Jim told me on IRC that this needs someone familiar with the Talos suite to look into integration issues.  He'll provide more info shortly.

Comment 33

[Jim Mathies [:jimm]]

Those files were plucked from here:

http://mxr.mozilla.org/mozilla/source/testing/performance/talos/startup_test/twinopen/

Aside from the changes to record paint times in the child window, the function reportResults() was modified to fill the paint times into form elements and I've also added some script to 'ignore first open' which can be removed. 

The output talos picks up comes from a set of dump statements, those have been stripped from my script as well, but it should be easy to add those back in. (copy paste the old data dumps for open times and add similar for paint times.)

From there, it's just integration work with the talos slaves, and getting graph server configured to record the data output. (best guess anyway) I can assist where ever help is needed.

There are also different phases to this test that are configurable, I haven't tested everything to be sure it's working, but I'd be happy to do that as needed.

Comment 34

[Jim Mathies [:jimm]]

(In reply to comment #33)
> Aside from the changes to record paint times in the child window, the function
> reportResults() was modified to fill the paint times into form elements and
> I've also added some script to 'ignore first open' which can be removed. 

Correction, 'ignore first open' is part of the original test, above that I added some code to 'ignore highest' which isn't. I found this gave more accurate results.

Comment 35

[cmtalbert]

Attachment: Applying Jimm's changes to current talos tree

Jim, 

I'm a little confused at what exactly the desired outcome here is.  Do we want to update the existing Twinopen/Txul numbers with these numbers?  Or do we want to create a new test aka Twinopenpaint to track these numbers?

I tend to think we want to replace the Twinopen/txul with these numbers.  

Also, the location of Talos has changed, it's now (thankfully) in hg: hg.mozilla.org/build/talos.  I've attached a patch which is simply the changes in your zip file applied to a current clone of the talos hg repo.  Please make sure your changes are applied correctly, so I know we're starting with the right diff.  It looks like there are some changes that are in this diff which are a result of the code you edited being old.

Plan forward - let's get the right diff to the hg talos repo and figure out if you want to update the existing numbers going forward or if you want a new test.

Once we get that worked out, my team can help with the integration.

Comment 36

[Shawn Wilsher :sdwilsh]

(In reply to comment #35)
> I tend to think we want to replace the Twinopen/txul with these numbers.  
This.  Right now it's not measuring what we actually care about.

Comment 37

[Jim Mathies [:jimm]]

Attachment: updated twinopen

Here we go. Tested locally.

Comment 38

[Jim Mathies [:jimm]]

(In reply to comment #35)
> Created attachment 506922 [details] [diff] [review]
> I'm a little confused at what exactly the desired outcome here is.  Do we want
> to update the existing Twinopen/Txul numbers with these numbers?  Or do we want
> to create a new test aka Twinopenpaint to track these numbers?
> 
> I tend to think we want to replace the Twinopen/txul with these numbers.  
> 

I think we wanted to keep both sets. FWIW, there is a difference between the two numbers, so having both might be useful.

Comment 39

[Jim Mathies [:jimm]]

It might be useful to dig into where the numbers go. In the current test, the following data gets dumped:

+        dumpLog("__start_report" + openingTimes.join('|') + "__end_report");
+        var now = (new Date()).getTime();
+        dumpLog("__startTimestamp" + now + "__endTimestamp\n");
+        dumpLog("openingTimes="+openingTimes.slice(1)+"\n");
+        dumpLog("avgOpenTime:" + avgOpenTime + "\n" );
+        dumpLog("minOpenTime:" + minOpenTime + "\n" );
+        dumpLog("maxOpenTime:" + maxOpenTime + "\n" );
+        dumpLog("medOpenTime:" + medOpenTime + "\n" );
+        dumpLog("__xulWinOpenTime:" + medOpenTime + "\n");

Note, medOpenTime and __xulWinOpenTime are the same value. I'm not sure what gets populated where on graphs. Also, this test tracks window close times, but AFAICT that data isn't reported by the test. 

The data I've added is here:

+        dumpLog("__start_report" + openingTimes.join('|') + "__end_report");
+        var now = (new Date()).getTime();
+        dumpLog("paintTimes="+openingTimes.slice(1)+"\n");
+        dumpLog("avgPaintTime:" + avgOpenTime + "\n" );
+        dumpLog("minPaintTime:" + minOpenTime + "\n" );
+        dumpLog("maxPaintTime:" + maxOpenTime + "\n" );
+        dumpLog("medPaintTime:" + medOpenTime + "\n" );

Comment 40

[Jim Mathies [:jimm]]

Attachment: updated twinopen

Fixed that paint reporting to dump the right data.

Comment 41

[(no longer active)]

If we choose to report both numbers, which one would be used in the regression detection script, and on the charts on the graph server?  Both?

Comment 42

[Jim Mathies [:jimm]]

(In reply to comment #41)
> If we choose to report both numbers, which one would be used in the regression
> detection script, and on the charts on the graph server?  Both?

I think we would want both. Txul measures page load, Txulp measures the time it takes to paint the window. (Actually we are only measuring paint of content, but I believe chrome is included as well.) It might be interesting to figure out what occurs between the page load event and the first mozafterpaint to get a feel for what the difference is measuring. I'd guess it's the amount of time it takes to do the actual painting.

Comment 43

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

let me work on getting this patch running in a talos staging environment.

Comment 44

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

running this patch on talos staging yields a failure in twinopen (also known as txul) where it times out.  I haven't spent any time debugging this, is this expected to pass with just the talos patch on a recent build of firefox?

Comment 45

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

some more data, talos chrome/nochrome runs as a set of tests: ts:tdhtml:twinopen:tsspider

running twinopen by itself on my desktop works fine, but running as a set of tdhtml:twinopen fails.  Oddly enough these use unique profiles, but something is causing it to hang.

What I see is the child window open and it just sits there.  On a normal run it opens a child window and pretty much immediately that child window will close.

Comment 46

[Jim Mathies [:jimm]]

(In reply to comment #45)
> some more data, talos chrome/nochrome runs as a set of tests:
> ts:tdhtml:twinopen:tsspider
> 
> running twinopen by itself on my desktop works fine, but running as a set of
> tdhtml:twinopen fails.  Oddly enough these use unique profiles, but something
> is causing it to hang.
> 
> What I see is the child window open and it just sits there.  On a normal run it
> opens a child window and pretty much immediately that child window will close.

There's a report dumped by the test that has some header/footer wrappers around  data. The old test dumped twinopen using a __start_report/__end_report. Since this now dumps two sets of data that was changed to __start_load_report/__end_load_report and the paint data is in a __xxx_paint_report report. You could try changing the twinopen header back, maybe talos is looking for that and not finding it?

Comment 47

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

when twinopen is run by itself, it produces the output numbers.  We would need to modify the output regex to accept this format or make it fit into the existing format.  This is an easy problem to fix.

The problem I have is when twinopen is run in a set of tests, the browser hangs when it opens the child window.  This is how they are run inside of production talos.

Comment 48

[Jim Mathies [:jimm]]

(In reply to comment #47)
> when twinopen is run by itself, it produces the output numbers.  We would need
> to modify the output regex to accept this format or make it fit into the
> existing format.  This is an easy problem to fix.
> 
> The problem I have is when twinopen is run in a set of tests, the browser hangs
> when it opens the child window.  This is how they are run inside of production
> talos.

Just confirming, did your test include using our special talos profile with all the custom settings we use to make these tests work?

Comment 49

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

probably not, I just applied this patch (talos patch on this bug) and ran:
https://bugzilla.mozilla.org/attachment.cgi?id=506973

If needed, can you make a patch that included the profile changes as well?

Comment 50

[Jim Mathies [:jimm]]

(In reply to comment #49)
> probably not, I just applied this patch (talos patch on this bug) and ran:
> https://bugzilla.mozilla.org/attachment.cgi?id=506973
> 
> If needed, can you make a patch that included the profile changes as well?

Alice could probably answer that better than me, I don't think a simple patch can do it. We override various security settings and disable various browser settings to get things running smoothly. I did post a patch in the "modified version of winopen.xul" zip here that turns off one of the key security restrictions. That might be enough to confirm this runs properly on a talos slave.

To confirm this is working though we really need to setup and run a full talos test run with the winopen test changes.

Comment 51

[Jim Mathies [:jimm]]

Attachment: updated twinopen

sans the header string change for page load data.

Comment 52

[Jim Mathies [:jimm]]

Comment on attachment 510608 [details] [diff] [review]
updated twinopen

whoops, need to change something.

Comment 53

[Jim Mathies [:jimm]]

Attachment: twinopen patch

Comment 54

[cmtalbert]

Joel can we run this in staging and see how it goes?

Comment 55

[Jim Mathies [:jimm]]

I think jmaher's been working on that today. He was having some problems with the test not completing.

Comment 56

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

Attachment: twinopen patch

this patch passes on talos staging.  Once we figure out how we want to integrate this into talos and the various branches, I can create a more formal patch and get a review.

Comment 57

[Jim Mathies [:jimm]]

We also use similar logic for Ts:

http://mxr.mozilla.org/build/source/talos/startup_test/startup_test.html?force=1

Once the patch in bug 594821 lands, those numbers will also likely include painting. I wonder if we should also consider modifying Ts to include paint as well?

Comment 58

[Timothy Nikkel (:tnikkel)]

I think so. That is the test I originally noticed the problem on and filed this bug about.

Comment 59

[Jim Mathies [:jimm]]

Attachment: updates to ts and twinopen

Assuming the consensus is we simply upgrade our existing tests, this patch updates ts and twinopen to include mozafterpaint.

Comment 60

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

:jimm, this latest patch looks good overall.  The only concern with making the default measurement for ts and txul report the time to paint vs time to execute javascript code is that we need to backport the MozAfterPaint events and related code into all branches that talos supports.

I don't know all of the branches, but I believe it is 3.5, 3.6 and mozilla central.  Also I believe we run on a lot of the other branches like tracemonkey.

Comment 61

[Jim Mathies [:jimm]]

(In reply to comment #60)
> :jimm, this latest patch looks good overall.  The only concern with making the
> default measurement for ts and txul report the time to paint vs time to execute
> javascript code is that we need to backport the MozAfterPaint events and
> related code into all branches that talos supports.

So, how does having a new tpaint test piggybacked on top of twinopen (txul) get around this? Sounds like we have the same problem regardless of which change we make, unless we split the new tpaint test out as an individual test and only run on certain branches.

Comment 62

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

yeah, the alternative is to create a ts_paint and a tpaint test.  For mozilla-central we can run these instead (or in addition to) of ts and txul, and for other branches we can just do what we have done in the past.

Comment 63

[cmtalbert]

(In reply to comment #61)
> (In reply to comment #60)
> > :jimm, this latest patch looks good overall.  The only concern with making the
> > default measurement for ts and txul report the time to paint vs time to execute
> > javascript code is that we need to backport the MozAfterPaint events and
> > related code into all branches that talos supports.
> 
> So, how does having a new tpaint test piggybacked on top of twinopen (txul) get
> around this? Sounds like we have the same problem regardless of which change we
> make, unless we split the new tpaint test out as an individual test and only
> run on certain branches.

Right, if we split these out, then we can run the new tests on a per branch basis by modifying the buildbot configs.

Comment 64

[cmtalbert]

(In reply to comment #62)
> yeah, the alternative is to create a ts_paint and a tpaint test.  For
> mozilla-central we can run these instead (or in addition to) of ts and txul,
> and for other branches we can just do what we have done in the past.

Joel (forgive me if this is a stupid question) can we simply gate these by using a different config file?  My thinking is that on mozilla-central and what not we use a config file that says "use mozafterpaint when running twinopen/ts" and on older branches it says "do not use mozafterpaint when running twinopen/ts"  

This is of course assuming that for m-c we are replacing twinopen/ts with mozafterpaint enabled tests.  I think solving it this way would be less work than creating entirely new tests, new patches and running them through staging.

Comment 65

[Jim Mathies [:jimm]]

Does Txul run on older branches? We might also need to pass a param into the test that flips between onload/onload+mozafterpaint. Mozafterpaint was introduced in 3.5 I believe, so it would break twinopen on older branches.

Comment 66

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

Attachment: split out to tpaint and ts_paint

uploading patch that creates a tpaint (twinopen but measuring mozafterpaint) and ts_paint (ts but measuring mozafterpaint).  These work great locally and on talos staging, but they seem to fail when uploading the the graph server as the test name tpaint and ts_paint are unknown.

I know it is easy to configure different talos tests for different branches.

Comment 67

[Jim Mathies [:jimm]]

Joel, any chance you could post a comparison of the numbers between the old txul and the new tpaint for linux, mac and win?

Comment 68

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

windows 7 64 bit: 
Running test twinopen: 
		Started Fri, 18 Feb 2011 07:53:19
	Screen width/height:1280/1024
	colorDepth:24
	Browser inner width/height: 1006/676

NOISE: __start_report307|126|130|126|129|133|136|131|136|131|133|134|133|133|137|132|136|131|131|133__end_report__startTimestamp1298044461963__endTimestamp
NOISE: openingTimes=126,130,126,129,133,136,131,136,131,133,134,133,133,137,132,136,131,131,133
NOISE: avgOpenTime:132
NOISE: minOpenTime:126
NOISE: maxOpenTime:137
NOISE: medOpenTime:133
NOISE: __xulWinOpenTime:133
NOISE: __startSecondTimestamp1298044462510__endSecondTimestamp
Completed test twinopen: 
		Stopped Fri, 18 Feb 2011 07:54:29
Running test tpaint: 
		Started Fri, 18 Feb 2011 07:54:29
	Screen width/height:1280/1024
	colorDepth:24
	Browser inner width/height: 1006/676

NOISE: __start_report323|150|148|146|144|144|149|143|150|143|150|145|154|146|149|144|154|146|148|145__end_report__startTimestamp1298044532874__endTimestamp
NOISE: openingTimes=150,148,146,144,144,149,143,150,143,150,145,154,146,149,144,154,146,148,145
NOISE: avgOpenTime:147
NOISE: minOpenTime:143
NOISE: maxOpenTime:154
NOISE: medOpenTime:146
NOISE: __xulWinOpenTime:146
NOISE: __startSecondTimestamp1298044533444__endSecondTimestamp
Completed test tpaint: 
		Stopped Fri, 18 Feb 2011 07:55:40




Mac OSX 10.6.2 (snow leopard):
Running test twinopen: 
		Started Fri, 18 Feb 2011 08:27:14
	Screen width/height:1280/1024
	colorDepth:24
	Browser inner width/height: 1024/681

NOISE: __start_report298|248|247|302|246|247|246|245|247|246|248|246|247|247|247|247|250|245|246|246__end_report__startTimestamp1298046494287__endTimestamp
NOISE: openingTimes=248,247,302,246,247,246,245,247,246,248,246,247,247,247,247,250,245,246,246
NOISE: avgOpenTime:250
NOISE: minOpenTime:245
NOISE: maxOpenTime:302
NOISE: medOpenTime:247
NOISE: __xulWinOpenTime:247
NOISE: __startSecondTimestamp1298046494556__endSecondTimestamp
Completed test twinopen: 
		Stopped Fri, 18 Feb 2011 08:28:21
Running test tpaint: 
		Started Fri, 18 Feb 2011 08:28:21
	Screen width/height:1280/1024
	colorDepth:24
	Browser inner width/height: 1024/681

NOISE: __start_report289|256|267|310|256|256|257|254|257|255|255|255|255|256|255|256|260|255|255|255__end_report__startTimestamp1298046560833__endTimestamp
NOISE: openingTimes=256,267,310,256,256,257,254,257,255,255,255,255,256,255,256,260,255,255,255
NOISE: avgOpenTime:259
NOISE: minOpenTime:254
NOISE: maxOpenTime:310
NOISE: medOpenTime:256
NOISE: __xulWinOpenTime:256
NOISE: __startSecondTimestamp1298046561115__endSecondTimestamp
Completed test tpaint: 
		Stopped Fri, 18 Feb 2011 08:29:27



Fedora 32 bit linux:
Running test twinopen: 
		Started Fri, 18 Feb 2011 09:34:54
	Screen width/height:1280/1024
	colorDepth:24
	Browser inner width/height: 1024/682

NOISE: __start_report269|99|97|97|97|97|98|96|99|97|98|97|96|99|100|97|103|97|97|99__end_report__startTimestamp1298050554358__endTimestamp
NOISE: openingTimes=99,97,97,97,97,98,96,99,97,98,97,96,99,100,97,103,97,97,99
NOISE: avgOpenTime:98
NOISE: minOpenTime:96
NOISE: maxOpenTime:103
NOISE: medOpenTime:97
NOISE: __xulWinOpenTime:97
NOISE: __startSecondTimestamp1298050554837__endSecondTimestamp
Completed test twinopen: 
		Stopped Fri, 18 Feb 2011 09:36:01
Running test tpaint: 
		Started Fri, 18 Feb 2011 09:36:01
	Screen width/height:1280/1024
	colorDepth:24
	Browser inner width/height: 1024/682

NOISE: __start_report291|145|139|129|134|137|121|150|151|113|138|152|152|147|159|136|159|144|137|138__end_report__startTimestamp1298050622545__endTimestamp
NOISE: openingTimes=145,139,129,134,137,121,150,151,113,138,152,152,147,159,136,159,144,137,138
NOISE: avgOpenTime:141
NOISE: minOpenTime:113
NOISE: maxOpenTime:159
NOISE: medOpenTime:139
NOISE: __xulWinOpenTime:139
NOISE: __startSecondTimestamp1298050623052__endSecondTimestamp
Completed test tpaint: 
		Stopped Fri, 18 Feb 2011 09:37:10



On the MV-Office VPN, you can see all the results here:
http://tools-staging-master.mv.mozilla.com:8010/builders

Comment 69

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

Should we move forward with making tpaint and ts_paint?  Can I provide any more data?

Comment 70

[Jim Mathies [:jimm]]

(In reply to comment #69)
> Should we move forward with making tpaint and ts_paint?  Can I provide any more
> data?

Lets get them running!

Comment 71

[Jim Mathies [:jimm]]

I think the only useful place these can run currently is mc. Unless we want to back port those MozAfterPaint changes to 3.6. (Which might make for an interesting comparison.) Roc, any thoughts on doing that?

Comment 72

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'm not super-excited about doing that :-)

Comment 73

[Robert O'Callahan (:roc) (email my personal email if necessary)]

If you think it's worth it, Kevin Gadd might be able to help

Comment 74

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

It sounds like for the branches that we have MozAfterPaint implemented we should switch to the new tests and leave the old tests alone.  It will take a few bugs to releng to get the graph server updated for the new test names and turn off the old and add the new for mozilla-central and any other branches that have MozAfterPaint.

Comment 75

[Jim Mathies [:jimm]]

(In reply to comment #74)
> It sounds like for the branches that we have MozAfterPaint implemented we
> should switch to the new tests and leave the old tests alone.  It will take a
> few bugs to releng to get the graph server updated for the new test names and
> turn off the old and add the new for mozilla-central and any other branches
> that have MozAfterPaint.

Initially lets just file bugs to add ts_paint and tpaint to mc + graph server. We can leave TXul running for now. Would that be the simplest approach requiring the least amount of changes?

Comment 76

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

bug 635898 is filed for the graphserver change, I will do a staging run after they are in the graph server for a sanity check, then file a bug for changing the tests on mozilla-central.

Comment 77

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #74)
> It sounds like for the branches that we have MozAfterPaint implemented

MozAfterPaint is implemented on all the branches, it just doesn't work properly for these tests on the older branches.

Comment 78

[(dormant account)]

(In reply to comment #1)
> I'd vote to add this as an additional test. Ts tests how long it takes to
> launch the app and dump output from the content window onload event. This is
> valuable on it's own. Adding paint time into that could make finding
> non-painting issues harder.

I strongly disagree with this statement. ts is our standard startup test and our code contains a lot of workarounds to make this test happy(ie bug 641691). I think it's downright harmful to keep this test as is.

I think we should change ts to be more realistic(ie use firstpaint) and add a onload-fired test to cover the onload event if that's really valuable.

Comment 79

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

ok, now that tpaint and ts_paint are in the graph servers, I found a new problem: ts_paint_shutdown (the paint version of ts_shutdown).  This is automatically created when we do ts with shutdown=True.  

The question is do we want this, or do we want to ignore the shutdown tracking?  Also we have ts_places_* and ts_cold tests.

For the time being, I propose keeping what we have to get things online.  We can migrate other tests over by changing some config files in the future.

Comment 80

[Jim Mathies [:jimm]]

(In reply to comment #79)
> ok, now that tpaint and ts_paint are in the graph servers, I found a new
> problem: ts_paint_shutdown (the paint version of ts_shutdown).  This is
> automatically created when we do ts with shutdown=True.  
> 
> The question is do we want this, or do we want to ignore the shutdown tracking?
>  Also we have ts_places_* and ts_cold tests.
> 
> For the time being, I propose keeping what we have to get things online.  We
> can migrate other tests over by changing some config files in the future.

We don't need shutdown data from "ts_paint". I would just ignore this.

Comment 81

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

Attachment: tpaint and ts_paint

updated patch to only do ts_paint and tpaint (not ts_paint_shutdown).Passes green on try.  can we get a sanity check review on the patch for anything I am overlooking?

Comment 82

[Jim Mathies [:jimm]]

(In reply to comment #81)
> Created attachment 519989 [details] [diff] [review]
> tpaint and ts_paint
> 
> updated patch to only do ts_paint and tpaint (not ts_paint_shutdown).Passes
> green on try.  can we get a sanity check review on the patch for anything I am
> overlooking?

I don't see any changes from the previous patch. Did something in here have to change to remove the shutdown test?

Comment 83

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

inside sample.config for the ts_paint section, I now have:
+  shutdown : False

instead of:
+  shutdown : True

Comment 84

[Jim Mathies [:jimm]]

Comment on attachment 519989 [details] [diff] [review]
tpaint and ts_paint

ah, config change. Looks ok to me!

Comment 85

[Aki Sasaki (not active)]

Joel asked for feedback, so:

The only thing I'm concerned about is whether Ts and Twinopen numbers are going to shift. We can park this in staging for a bit to confirm or deny this.

Evidently tpaint and ts_paint are going to replace twinopen and ts, but only on recent branches, and there may be a gap between landing this patch in talos and switching over.  So knowing ahead of time whether there's going to be a shift in numbers would be a positive.

Alternately, if others are concerned, we can have another Talos tree closure where we land, run talos on all platforms, and note the numbers as a new baseline.

Alice should be back Monday and may have additional feedback.

Comment 86

[Shawn Wilsher :sdwilsh]

(In reply to comment #85)
> Alternately, if others are concerned, we can have another Talos tree closure
> where we land, run talos on all platforms, and note the numbers as a new
> baseline.
I would prefer this.

Comment 87

[Jim Mathies [:jimm]]

Any updates on the roll out Joel? It doesn't look like this is live on graphs yet.

Comment 88

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

Attachment: [checked in]updated patch to completely remove tpaint changes from existing code paths (1.0)

ok, here it is...green on talos staging and all.  This patch:
 - makes a tspaint_test.html file for ts_paint, no changes to ts code
 - makes a tpaint-child.html for tpaint, no changes to twinopen except for the .xul which shouldn't be in the code path for open/close testing
 - adds a cli option to PerfConfigurator.py to support --setPref so we don't have to change prefs for existing tests.

In order to run this, you need a command line like:
python PerfConfigurator.py --activeTests tpaint --setPref "dom.send_after_paint_to_content=true"

Comment 89

[alice nodelman [:alice] [:anode]]

Comment on attachment 522736 [details] [diff] [review]
[checked in]updated patch to completely remove tpaint changes from existing code paths (1.0)

Looks good to me.  With the separation from ts/txul we should be able to roll this out without affecting current numbers.  Win!

Comment 90

[alice nodelman [:alice] [:anode]]

Which branches should these new tests be active on?  I only see reference to 'new' branches, but we'll need to a real list to be able to write the code to roll this out.

Comment 91

[Shawn Wilsher :sdwilsh]

(In reply to comment #90)
> Which branches should these new tests be active on?  I only see reference to
> 'new' branches, but we'll need to a real list to be able to write the code to
> roll this out.
mozilla-central and any project branches that are based off of it (plus the branches we will be using for Firefox 5).

Comment 92

[alice nodelman [:alice] [:anode]]

Attachment: [checked in]add paint tests to buildbot-configs (take 1)

Comment 93

[alice nodelman [:alice] [:anode]]

I need to get a green staging run with the buildbot-config patch and the talos code patch, then we can request landing from releng.

Comment 94

[alice nodelman [:alice] [:anode]]

Running on tools staging overnight.

Comment 95

[alice nodelman [:alice] [:anode]]

jmaher: have the names 'tpaint' and 'ts_paint' been added to the production graph server?

Comment 96

[Jim Mathies [:jimm]]

(In reply to comment #95)
> jmaher: have the names 'tpaint' and 'ts_paint' been added to the production
> graph server?

doesn't look like it.

Comment 97

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

tpaint, ts_paint were added to the databases, just not checked into the HG repo.  This should be done now.

Comment 98

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

Comment on attachment 522840 [details] [diff] [review]
[checked in]add paint tests to buildbot-configs (take 1)

this looks good.  We can follow up in the future to turn off ts and twinopen on the branches where we have *paint tests running.

Comment 99

[alice nodelman [:alice] [:anode]]

Green test run for all platforms last night.

Comment 100

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

http://hg.mozilla.org/build/talos/pushloghtml?changeset=bd4f59ed70a0

Comment 101

[[e-mail] (:bhearsum)]

Comment on attachment 522840 [details] [diff] [review]
[checked in]add paint tests to buildbot-configs (take 1)

Seems sensible.

Comment 102

[alice nodelman [:alice] [:anode]]

Comment on attachment 522840 [details] [diff] [review]
[checked in]add paint tests to buildbot-configs (take 1)

changeset:   3957:a081caf7398f

Comment 103

[alice nodelman [:alice] [:anode]]

Moving deployment to bug 649175

Comment 104

[alice nodelman [:alice] [:anode]]

This was rolled out in bug 649175.  I'll leave it to jmaher to confirm and close this bug.

Comment 105

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

verified on the graph server!