Closed
Bug 612190
Opened 14 years ago
Closed 13 years ago
startup talos test should include browser paint
Categories
(Testing :: Talos, defect)
Testing
Talos
Tracking
(blocking2.0 -)
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | - |
People
(Reporter: tnikkel, Assigned: jmaher)
References
Details
Attachments
(3 files, 10 obsolete files)
9.50 KB,
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
11.23 KB,
patch
|
anodelman
:
review+
|
Details | Diff | Splinter Review |
5.14 KB,
patch
|
jmaher
:
review+
bhearsum
:
review+
|
Details | Diff | Splinter Review |
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•14 years ago
|
||
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.
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•14 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.
Comment 6•14 years ago
|
||
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•14 years ago
|
blocking2.0: --- → ?
Comment 8•14 years ago
|
||
Do any of our Talos slaves have hardware acceleration? Or will these changes only record gdi based paint timing?
Comment 9•14 years ago
|
||
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•14 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•14 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•14 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•14 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•14 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•14 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•14 years ago
|
||
So this bug is fixed?
No. Someone still needs to do what I said in comment #26.
Comment 29•14 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
Comment 31•14 years ago
|
||
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•14 years ago
|
||
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
Updated•14 years ago
|
Assignee: anodelman → jhammel
Comment 33•14 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•14 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•14 years ago
|
||
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
Comment 36•14 years ago
|
||
(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•14 years ago
|
||
Here we go. Tested locally.
Attachment #506922 -
Attachment is obsolete: true
Comment 38•14 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•14 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•14 years ago
|
||
Fixed that paint reporting to dump the right data.
Attachment #506962 -
Attachment is obsolete: true
Comment 41•14 years ago
|
||
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•14 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•14 years ago
|
||
let me work on getting this patch running in a talos staging environment.
Assignee | ||
Comment 44•13 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•13 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•13 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•13 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•13 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•13 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•13 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•13 years ago
|
||
sans the header string change for page load data.
Attachment #506973 -
Attachment is obsolete: true
Comment 52•13 years ago
|
||
Comment on attachment 510608 [details] [diff] [review] updated twinopen whoops, need to change something.
Attachment #510608 -
Attachment is obsolete: true
Comment 53•13 years ago
|
||
Comment 54•13 years ago
|
||
Joel can we run this in staging and see how it goes?
Comment 55•13 years ago
|
||
I think jmaher's been working on that today. He was having some problems with the test not completing.
Assignee | ||
Comment 56•13 years ago
|
||
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•13 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•13 years ago
|
||
I think so. That is the test I originally noticed the problem on and filed this bug about.
Comment 59•13 years ago
|
||
Assuming the consensus is we simply upgrade our existing tests, this patch updates ts and twinopen to include mozafterpaint.
Assignee | ||
Comment 60•13 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•13 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•13 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•13 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•13 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•13 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•13 years ago
|
||
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•13 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•13 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•13 years ago
|
||
Should we move forward with making tpaint and ts_paint? Can I provide any more data?
Comment 70•13 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•13 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•13 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•13 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 | ||
Comment 76•13 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•13 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•13 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•13 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•13 years ago
|
||
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•13 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•13 years ago
|
||
inside sample.config for the ts_paint section, I now have: + shutdown : False instead of: + shutdown : True
Comment 84•13 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•13 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.
Comment 86•13 years ago
|
||
(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•13 years ago
|
||
Any updates on the roll out Joel? It doesn't look like this is live on graphs yet.
Assignee | ||
Comment 88•13 years ago
|
||
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 89•13 years ago
|
||
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+
Comment 90•13 years ago
|
||
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•13 years ago
|
||
(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•13 years ago
|
||
Comment 93•13 years ago
|
||
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•13 years ago
|
||
Running on tools staging overnight.
Comment 95•13 years ago
|
||
jmaher: have the names 'tpaint' and 'ts_paint' been added to the production graph server?
Comment 96•13 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•13 years ago
|
||
tpaint, ts_paint were added to the databases, just not checked into the HG repo. This should be done now.
Updated•13 years ago
|
Attachment #522840 -
Flags: review?(jmaher)
Assignee | ||
Comment 98•13 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.
Comment 99•13 years ago
|
||
Green test run for all platforms last night.
Assignee | ||
Comment 100•13 years ago
|
||
http://hg.mozilla.org/build/talos/pushloghtml?changeset=bd4f59ed70a0
Updated•13 years ago
|
Attachment #522840 -
Flags: review?(bhearsum)
Assignee | ||
Updated•13 years ago
|
Attachment #522840 -
Flags: review?(jmaher) → review+
Comment 101•13 years ago
|
||
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+
Updated•13 years ago
|
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 102•13 years ago
|
||
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)
Comment 104•13 years ago
|
||
This was rolled out in bug 649175. I'll leave it to jmaher to confirm and close this bug.
Assignee | ||
Comment 105•13 years ago
|
||
verified on the graph server!
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•