startup talos test should include browser paint

RESOLVED FIXED

Status

Testing
Talos
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: tnikkel, Assigned: jmaher)

Tracking

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 -)

Details

Attachments

(3 attachments, 10 obsolete attachments)

9.50 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
11.23 KB, patch
alice
: review+
Details | Diff | Splinter Review
5.14 KB, patch
jmaher
: review+
bhearsum
: review+
Details | Diff | Splinter Review
(Reporter)

Description

7 years ago
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?
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.

Updated

7 years ago
Depends on: 613377
Created attachment 491704 [details] [diff] [review]
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.
Attachment #491704 - Flags: review?(dbaron)
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.
(Reporter)

Comment 4

7 years ago
For the record it was Ts where I observed no paint happening during the timed portion of the test.
Right. We should support both.
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.
That is a very good idea!

Updated

7 years ago
Blocks: 594821

Updated

7 years ago
blocking2.0: --- → ?

Comment 8

7 years ago
Do any of our Talos slaves have hardware acceleration? Or will these changes only record gdi based paint timing?
All of our Win7 talos machines are hardware accelerated D3D10/D2D, and WinXP are D3D9.
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?
blocking2.0: ? → ---
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.
(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 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.
Attachment #491704 - Flags: review?(dbaron) → review+
Whiteboard: [needs landing]
(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

7 years ago
Requesting blocking so this can land. This blocks blocking bug 594821.
blocking2.0: --- → ?
blocking2.0: ? → -
a=me. It feels oogy approving my own patch, but it should obviously land.

Please do a tryserver run though. It could affect reftests.
a=dbaron as well

Comment 18

7 years ago
Pushed to try for a full test run on all platforms.
What were the results?
Ah, it looks like there weren't any results!

Comment 21

7 years ago
(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

7 years ago
Looks like those last two lines should be 

frame->PresContext()->NotifyDidPaintForSubtree()?
It built for me, see http://hg.mozilla.org/try/rev/28177ea3b6ea

Comment 24

7 years ago
(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.
(Reporter)

Comment 25

7 years ago
See also bug 522375 which added code to track when the first paint happens.
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.
Whiteboard: [needs landing]

Comment 27

7 years ago
So this bug is fixed?
No. Someone still needs to do what I said in comment #26.

Comment 29

7 years ago
(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.
Ehsan, can you look into this?
Assignee: nobody → ehsan
Depends on: 626168

Comment 31

7 years ago
Created attachment 505935 [details]
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.
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.
Assignee: ehsan → anodelman
Assignee: anodelman → jhammel

Comment 33

7 years ago
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

7 years ago
(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

7 years ago
Created attachment 506922 [details] [diff] [review]
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.
Attachment #505935 - Attachment is obsolete: true
(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

7 years ago
Created attachment 506962 [details] [diff] [review]
updated twinopen

Here we go. Tested locally.
Attachment #506922 - Attachment is obsolete: true

Comment 38

7 years ago
(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

7 years ago
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

7 years ago
Created attachment 506973 [details] [diff] [review]
updated twinopen

Fixed that paint reporting to dump the right data.
Attachment #506962 - Attachment is obsolete: true
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

7 years ago
(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.
(Assignee)

Comment 43

7 years ago
let me work on getting this patch running in a talos staging environment.
(Assignee)

Comment 44

7 years ago
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?
(Assignee)

Comment 45

7 years ago
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

7 years ago
(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?
(Assignee)

Comment 47

7 years ago
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

7 years ago
(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?
(Assignee)

Comment 49

7 years ago
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

7 years ago
(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

7 years ago
Created attachment 510608 [details] [diff] [review]
updated twinopen

sans the header string change for page load data.
Attachment #506973 - Attachment is obsolete: true

Comment 52

7 years ago
Comment on attachment 510608 [details] [diff] [review]
updated twinopen

whoops, need to change something.
Attachment #510608 - Attachment is obsolete: true

Comment 53

7 years ago
Created attachment 510610 [details] [diff] [review]
twinopen patch

Comment 54

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

Comment 55

7 years ago
I think jmaher's been working on that today. He was having some problems with the test not completing.
(Assignee)

Comment 56

7 years ago
Created attachment 510750 [details] [diff] [review]
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.
Assignee: jhammel → jmaher
Attachment #510610 - Attachment is obsolete: true

Comment 57

7 years ago
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?
(Reporter)

Comment 58

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

Comment 59

7 years ago
Created attachment 511292 [details] [diff] [review]
updates to ts and twinopen

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

Comment 60

7 years ago
: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

7 years ago
(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.
(Assignee)

Comment 62

7 years ago
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

7 years ago
(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

7 years ago
(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

7 years ago
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.
(Assignee)

Comment 66

7 years ago
Created attachment 513550 [details] [diff] [review]
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

7 years ago
Joel, any chance you could post a comparison of the numbers between the old txul and the new tpaint for linux, mac and win?
(Assignee)

Comment 68

7 years ago
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
(Assignee)

Comment 69

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

Comment 70

7 years ago
(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

7 years ago
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?
I'm not super-excited about doing that :-)
If you think it's worth it, Kevin Gadd might be able to help
(Assignee)

Comment 74

7 years ago
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

7 years ago
(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?
(Assignee)

Updated

7 years ago
Depends on: 635898
(Assignee)

Comment 76

7 years ago
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.
(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

7 years ago
(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.
(Assignee)

Comment 79

7 years ago
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

7 years ago
(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.
(Assignee)

Comment 81

7 years ago
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?
Attachment #511292 - Attachment is obsolete: true
Attachment #513550 - Attachment is obsolete: true
Attachment #519989 - Flags: review?(jmathies)

Comment 82

7 years ago
(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?
(Assignee)

Comment 83

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

instead of:
+  shutdown : True

Comment 84

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

ah, config change. Looks ok to me!
Attachment #519989 - Flags: review?(jmathies) → review+

Comment 85

7 years ago
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.
(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

7 years ago
Any updates on the roll out Joel? It doesn't look like this is live on graphs yet.
(Assignee)

Comment 88

7 years ago
Created attachment 522736 [details] [diff] [review]
[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"
Attachment #510750 - Attachment is obsolete: true
Attachment #519989 - Attachment is obsolete: true
Attachment #522736 - Flags: review?(anodelman)
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!
Attachment #522736 - Flags: review?(anodelman) → review+
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.
(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).
Created attachment 522840 [details] [diff] [review]
[checked in]add paint tests to buildbot-configs (take 1)
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.
Running on tools staging overnight.
jmaher: have the names 'tpaint' and 'ts_paint' been added to the production graph server?

Comment 96

7 years ago
(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.
(Assignee)

Comment 97

7 years ago
tpaint, ts_paint were added to the databases, just not checked into the HG repo.  This should be done now.
Attachment #522840 - Flags: review?(jmaher)
(Assignee)

Comment 98

7 years ago
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.
Green test run for all platforms last night.
(Assignee)

Comment 100

7 years ago
http://hg.mozilla.org/build/talos/pushloghtml?changeset=bd4f59ed70a0
Attachment #522840 - Flags: review?(bhearsum)
(Assignee)

Updated

7 years ago
Depends on: 646514
(Assignee)

Updated

7 years ago
Attachment #522840 - Flags: review?(jmaher) → review+
Comment on attachment 522840 [details] [diff] [review]
[checked in]add paint tests to buildbot-configs (take 1)

Seems sensible.
Attachment #522840 - Flags: review?(bhearsum) → review+
Attachment #522736 - Attachment description: updated patch to completely remove tpaint changes from existing code paths (1.0) → [checked in]updated patch to completely remove tpaint changes from existing code paths (1.0)
Comment on attachment 522840 [details] [diff] [review]
[checked in]add paint tests to buildbot-configs (take 1)

changeset:   3957:a081caf7398f
Attachment #522840 - Attachment description: add paint tests to buildbot-configs (take 1) → [checked in]add paint tests to buildbot-configs (take 1)
Moving deployment to bug 649175
Depends on: 649175
This was rolled out in bug 649175.  I'll leave it to jmaher to confirm and close this bug.
(Assignee)

Comment 105

6 years ago
verified on the graph server!
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Depends on: 652012
You need to log in before you can comment on or make changes to this bug.