731159 - Consider adding a new paint related test - tresize

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

[Jim Mathies [:jimm]]

Attachment: html

This test does a better job of measuring paint than tpaint since tpaint also includes window creation. The test creates a single window that is resized 300 times outward in small increments.

Comment 1

[Jim Mathies [:jimm]]

Attachment: diff

Comment 2

[Jim Mathies [:jimm]]

jmaher, any chance we might be able to get a test like this added in the future? It's a great repaint test, and should also give us strong indication of how well (or poor) resize performance is under accelerated Windows and GDI rendering. AFAIK, we currently do not have perf measurements of window resize performance.

Comment 3

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

which platforms would this run on?
Would this work ok with the mozafterpaint?

Overall this isn't hard, I just want to make sure we get all our details sorted out before we dive into it.

Thanks for bringing this bug to my attention!

Comment 4

[Jim Mathies [:jimm]]

(In reply to Joel Maher (:jmaher) from comment #3)
> which platforms would this run on?

Desktop platforms where we have windows. I haven't tested this on anything but Window, but it uses generic xul window calls so it should run on mac and linux as well.

> Would this work ok with the mozafterpaint?

It relies entirely on MozAfterPaint for results:

+function painted() {
+  window.removeEventListener("MozAfterPaint", painted, true);
+  var paintTime = (new Date()).getTime();
+  dataSet[count].end = paintTime;
+  setTimeout(resizeCompleted, 20);
+}
+
+function resizeTest() {
+  try {
+    netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');
+    dims += increment;
+    var startTime = (new Date()).getTime();
+    dataSet[count] = {};
+    dataSet[count].start = startTime;
+    window.addEventListener("MozAfterPaint", painted, true);
+    window.resizeTo(dims,dims);
+  } catch(ex) { dump(ex + '\n'); }
+}

> 
> Overall this isn't hard, I just want to make sure we get all our details
> sorted out before we dive into it.
> 
> Thanks for bringing this bug to my attention!

Sure. I like this test because we have issues with resize performance on windows when running with accelerated rendering. We have no test that measures this, so if a patch lands that changes resize perf, we don't know about it except through tpaint. tpaint though includes windows creation, this test more accurately measures window refresh because it's purely a resize -> repaint = results type test.

Comment 5

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

Attachment: add tresize into the test harness (1.0)

I found out that looking at the raw data we have a huge value for the first diff then the rest are much smaller.  do we care about that difference?  If not, can we ignore the first value in the array?

NOISE: diffs: 58,23,39,44,24,43,43,37,24,41,48,39,29,20,54,44,37,35,21,29,46,28,25,17,18,25,30,31,31,37,34,43,38,30,22,25,28,44,31,44,17,10,55,38,48,28,29,25,34,26,39,32,28,42,25,38,24,34,30,30,25,37,28,42,36,26,35,26,36,36,41,25,37,25,37,23,30,36,47,19,34,8,23,19,38,22,31,30,48,40,34,33,29,21,36,47,36,23,32,66,16,39,51,29,29,15,16,14,14,24,16,28,32,29,19,29,16,18,21,31,31,14,16,29,16,29,27,22,21,18,30,30,30,15,14,15,23,18,13,28,31,27,26,34,16,29,31,14,18,23,14,18,18,22,26,27,38,16,15,28,16,28,30,16,17,24,32,16,25,41,49,26,21,30,42,49,24,20,17,24,17,16,25,35,26,27,35,33,45,26,29,33,33,27,65,57,36,47,31,31,30,35,16,16,26,17,24,31,26,20,41,77,49,34,28,18,17,22,26,19,15,26,15,16,28,25,20,9,17,17,10,19,28,15,14,15,15,24,31,14,26,25,40,22,46,50,36,30,34,44,29,36,24,31,30,65,29,26,34,31,31,27,26,19,11,14,34,29,15,15,29,28,29,26,32,23,24,25,36,28,30,32,29,35,26,28,31,33,27,28,31,18,15,26,31,15,18,25,31,16
NOISE: __start_report28.546666666666667__end_report

Comment 6

[Jim Mathies [:jimm]]

(In reply to Joel Maher (:jmaher) from comment #5)
> Created attachment 645062 [details] [diff] [review]
> add tresize into the test harness (1.0)
> 
> I found out that looking at the raw data we have a huge value for the first
> diff then the rest are much smaller.  do we care about that difference?  If
> not, can we ignore the first value in the array?

I think we do something similar for paint tests - basically scan the data set and clip out the highest and lowest to remove anomalous values. Skipping the first value if it is repeatedly high seems fine too, or maybe try delaying the start of the test. This initial high value might be related to delayed startup activity in the browser.

Comment 7

[Jeff Hammel]

Comment on attachment 645062 [details] [diff] [review]
add tresize into the test harness (1.0)

i thought enablePrivilege was deprecated but i guess specialpowers was never fully landed :(

This looks fine

Comment 8

[Jeff Hammel]

(Of course this will need graphserver definitions buildbot pieces etc to go into production)

Comment 9

[Jim Mathies [:jimm]]

In talos, do we have the ability to differentiate between accelerated and non-accelerated test machines? The numbers between these two (if we have two different sets) will likely be different.

Comment 10

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

landed in talos:
http://hg.mozilla.org/build/talos/rev/6dfcdb136fd7

still have a bunch of bugs to file/resolve to get this live (well in incubation period)

Comment 11

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

need to consider making this a pageloader test to take advantage of our output parsing and datazilla upload.

Comment 12

[Jeff Hammel]

I assume this will actually be for all platforms?  Changing the bugzilla field to match that assumption.  Please change it back if I assume wrongly

Comment 13

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

Attachment: allow tresize (startup_tests) to output raw values (1.0)

oh the joys, this hacks up talos startuptest so we can get raw values.

Comment 14

[Jeff Hammel]

Comment on attachment 648833 [details] [diff] [review]
allow tresize (startup_tests) to output raw values (1.0)

+    filters = [['mean', []]] 

You'll need to add filters to the TsBase keys if you want this to be output

Comment 15

[Jeff Hammel]

So applying this patch causes a crash without data:

(talos)│talos -a tresize -e `which firefox` --develop --datazilla-url file://${PWD}/tresize.json -n -d
setting debug
DEBUG: using testdate: 1344031460
DEBUG: actual date: 1344031460
RETURN:<a href = "http://hg.mozilla.org/mozilla-central/rev/89dcadd42ec4">rev:89dcadd42ec4</a>
qm-pxp01: 
		Started Fri, 03 Aug 2012 15:04:20
Running test tresize: 
		Started Fri, 03 Aug 2012 15:04:20
DEBUG: operating with platform_type : linux_
DEBUG: created profile
	Screen width/height:1600/900
	colorDepth:24
	Browser inner width/height: 1024/678


browser_name:Firefox
browser_version:undefined
buildID:20120803030521

DEBUG: initialized firefox
DEBUG: command line: /home/jhammel/firefox/firefox  -profile /tmp/tmpNNNrnc/profile http://localhost:15707/startup_test/tresize-test.html
Aborted (core dumped)
Failed tresize: 
		Stopped Fri, 03 Aug 2012 15:07:12
FAIL: Busted: tresize
FAIL: timeout exceeded
Traceback (most recent call last):
  File "/home/jhammel/mozilla/src/talos/src/talos/talos/run_tests.py", line 310, in run_tests
    talos_results.add(mytest.runTest(browser_config, test))
  File "/home/jhammel/mozilla/src/talos/src/talos/talos/ttest.py", line 358, in runTest
    raise talosError("timeout exceeded")
talosError: 'timeout exceeded'
Traceback (most recent call last):
  File "/home/jhammel/mozilla/src/talos/bin/talos", line 8, in <module>
    load_entry_point('talos==0.0', 'console_scripts', 'talos')()
  File "/home/jhammel/mozilla/src/talos/src/talos/talos/run_tests.py", line 355, in main
    run_tests(parser.config)
  File "/home/jhammel/mozilla/src/talos/src/talos/talos/run_tests.py", line 319, in run_tests
    raise e
talos.utils.talosError: 'timeout exceeded'

I didn't see anything in the error console and haven't had a chance to debug further :(

Comment 16

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

hmm, I only did this via perfconfigurator and then run_tests.  Maybe this depends on something else in my patch queue.

Comment 17

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

Attachment: allow tresize (startup_tests) to output raw values (1.1)

updated and seems to be working well.  We need a graph server bit deployed before running this live.

Comment 18

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

Attachment: add tresize to the graph server (1.0)

and the small bit for the graph server.

Comment 19

[Jeff Hammel]

Comment on attachment 649314 [details] [diff] [review]
add tresize to the graph server (1.0)

so I'm not feel how i feel about the "new tests" line, but probably fine

Comment 20

[Jeff Hammel]

Comment on attachment 649309 [details] [diff] [review]
allow tresize (startup_tests) to output raw values (1.1)

So this mostly looks good.  I do find TsResults.__init__ a bit confusing.  Pasting the resulting code instead of the diff:

        # gather the data
        self.results = []
        index = 0
        for line in lines:
            result = {}
            r = line.strip().split(',')
            if len(r) == 1:
                break;
            result['index'] = index
            result['page'] = r[0]
            #note: if we have len(r) >1, then we have pagename,raw_results
            result['runs'] = [float(i) for i in r[1:]]
            self.results.append(result)
            index += 1

        if not self.results:
            result = {}
            result['index'] = index
            result['page'] = 'NULL'
            result['runs'] = [float(val) for val in string.split('|')]
            self.results.append(result)

So the two blocks here are for two cases: the first block is for the cases where you have pagenames; the second is for the case where you have the existing case without page names.  These should at least be labeled as such.

I would prefer if we could stick with the existing crappy output format of pageloader for the multiple page case.

Comment 21

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

http://hg.mozilla.org/graphs/rev/bb36f295cd26

Comment 22

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

http://hg.mozilla.org/build/talos/rev/f55ac228921e

We still need to get the graph server definitions inserted into the database and then update the buildbot configs to run this as a new job.

Comment 23

[Jeff Hammel]

Attachment: datazilla output

ever since the landing from https://bugzilla.mozilla.org/show_bug.cgi?id=731159#c17 datazilla output looks different

Comment 24

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

can you explain this a bit more.  I would like to fix this!

Comment 25

[Jeff Hammel]

So I'm not even sure it is a bug or I would have noted ;)

So notice how the SVG results look:

"results": {
      "composite-scale-opacity.svg": [
        23.0, 
        17.0, 
        17.0, 
        18.0, 
...

Now look at the ts results in the same run:

"results": {
      "ts": [
        [
          "NULL", 
          [
            549.0
          ]

So while in the first case you have:

results {'pagename': [values]}

In the second case you have

results {'ts (suite name?)': ['NULL (page name)': [values]]}

To me...these seem disparate.  I would guess the second would look like

results {'ts': [values]} 

or

results {'NULL': [values]}

Comment 26

[Jeff Hammel]

> results {'ts (suite name?)': ['NULL (page name)': [values]]}

actually, its:

{'ts': ['NULL', [values]]}

Sorry, I should have caught this on review.  I'm also not sure about sending the literal string 'NULL' as the pagename. We should probably take e.g. the test URL (somehow).

Comment 27

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

Attachment: fix the startup tests raw results (1.0)

Comment 28

[Jeff Hammel]

Comment on attachment 649689 [details] [diff] [review]
fix the startup tests raw results (1.0)

tested locally with ts.  This does look a lot better :)

Comment 29

[Jeff Hammel]

-    format = 'tsformat'
+    format = 'tpformat'

Can you explain why this is necessary?

Comment 30

[Jeff Hammel]

(In reply to Jeff Hammel [:jhammel] from comment #29)
> -    format = 'tsformat'
> +    format = 'tpformat'
> 
> Can you explain why this is necessary?

I'm not *really* sure the ramifications of this.  This will change how we deal with test_name_extension but i'm not sure if that is it or not

Comment 31

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

yeah, I am thinking it might be better to copy the logic until we are ready to make a unified format.

Comment 32

[Jeff Hammel]

So as best I can tell, this format parameter is used in three places:

* deciding the test name extension for graphserver (BOO!):
- http://hg.mozilla.org/build/talos/file/5341fb47177e/talos/output.py#l171
- http://hg.mozilla.org/build/talos/file/5341fb47177e/talos/output.py#l207

FWIW I think this is a total mis-feature and should be removed, but we'd have to do something to ensure we're sending to graphserver correctly.  See https://bugzilla.mozilla.org/show_bug.cgi?id=763610

* decide what to do for datazilla:
- http://hg.mozilla.org/build/talos/file/5341fb47177e/talos/output.py#l366

Given this will change how graphserver handles the data, I'm going to have to reverse my r+, though I *think* you can probably just change the datazilla output code and have the fix work

Comment 33

[Jeff Hammel]

ABICT, testing locally, it is fine without the one-line change in results.py.  :jmaher, is there any reason to include this change?  With the rest of the patch, the format is then only used for the (evil!) test_name_extension differentiation weirdness

Comment 34

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

Attachment: fix the startup tests raw results (1.1)

updated to be better...also green on try.

Comment 35

[Jeff Hammel]

Comment on attachment 650149 [details] [diff] [review]
fix the startup tests raw results (1.1)

Looks good and its less lines of code :)

Comment 36

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

landed:
http://hg.mozilla.org/build/talos/rev/405ced321cc9

next steps are to add this to buildbot-configs and get that deployed.  I am going to test tresize in a hacked up talos.zip where it runs in addition to ts_paint so we can see what it looks like live.  Assuming that works fine we can roll this out without temporary staging.

Comment 37

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

Attachment: add tresize to the chrome

adds tresize to the chrome suite.

Comment 38

[Chris Cooper [:coop] (he/him)]

Comment on attachment 650846 [details] [diff] [review]
add tresize to the chrome

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

Sorry if this has been covered already, but if this is a replacement test, is there something else we should be turning off?

Comment 39

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

this is a brand new test we are turning on.  Sort of a first for us in a while :)

Comment 40

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

landed the buildbot-configs patch:
http://hg.mozilla.org/build/buildbot-configs/rev/8fea6fabd400

this will be live on the next reconfig.  I will mark this bug as resolved once the tests are showing up on tbpl.  Look for them in the 'c' talos suite run alongside tdhtml.

Comment 41

[Phil Ringnalda (:philor)]

I'm looking for, or rather at, them on Aurora, where chromer is busted by https://tbpl.mozilla.org/php/getParsedLog.php?id=14347563&tree=Mozilla-Aurora - "No definition found for test(s): ['tresize']"

Comment 42

[Phil Ringnalda (:philor)]

Aurora's closed.

Comment 43

[Rail Aliiev [:rail]]

Comment on attachment 650846 [details] [diff] [review]
add tresize to the chrome

Backed out: http://hg.mozilla.org/build/buildbot-configs/rev/d67d647b8760

Comment 44

[Rail Aliiev [:rail]]

Backed out from production.

Comment 45

[Jim Mathies [:jimm]]

any updates on this?

Comment 46

[cmtalbert]

I believe this is waiting on Joel to retry the deployment once the proper fixes are made to graphserver (which is why it failed).

Not sure which bug the next set of graph server changes are taking place in. I'll dig around to find out and either myself or Jmaher will update here.

Comment 47

[Phil Ringnalda (:philor)]

Was it graphserver that bounced it? I thought it was the way it was trying to run on aurora (because aurora runs chromer and the set of things in chromer changered) but aurora had no definition for it (because aurora has an older talos.zip).

Comment 48

[Jeff Hammel]

(In reply to Phil Ringnalda (:philor) from comment #47)
> Was it graphserver that bounced it? I thought it was the way it was trying
> to run on aurora (because aurora runs chromer and the set of things in
> chromer changered) but aurora had no definition for it (because aurora has
> an older talos.zip).

IIRC that is precisely correct

Comment 49

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

yeah, we need to create a new test suite in buildbot that is mozilla-central only.  When I am back on Thursday I can do that.  This should be turned on next week.

Comment 50

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

Attachment: add a new chrome suite to account for tresize instead of adding it to an existing suite (1.0)

as per my previous mixup, we need to have a new test suite so we can let this ride the trains.

Comment 51

[Armen [:armenzg]]

Comment on attachment 655099 [details] [diff] [review]
add a new chrome suite to account for tresize instead of adding it to an existing suite (1.0)

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

Forget what I mentioned on the meeting. This is good.

Comment 52

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

landed:
http://hg.mozilla.org/build/buildbot-configs/rev/3a473f1232a9