For historical context: see bug 1317048 comment 15 for background here. (Basically, the static SVG content in tsvgx doesn't *need* ASAP paint mode enabled [and ASAP paint mode makes us measure the wrong thing for these files, once we take bug 1283302's patch]. So we want to move this content into its own suite with ASAP paint mode disabled.)

Comment on attachment 8812030 [details] Bug 1318530 - Split talos test suite 'svgx' by moving all static tests to new suite 'svg_static'. https://reviewboard.mozilla.org/r/93940/#review94086 Two commit message nits: > Bug 1318530 - Split talos test suite 'svgx' by moving all static tests to new suite 'svg-static' and disable ASAP mode for 'svg-static'. > This was a regression caused after pushing patch for Bug 1283302. (1) Drop the mention of ASAP mode here -- this patch doesn't actually disable ASAP mode in the suite (because the suite doesn't exist yet). I suspect you're referring to the "%" characters that you're adding to the manifest, but the "%" isn't what controls ASAP mode, per my comment below. (ASAP mode is controlled in the harness -- looks like it's in prefs that are optionally set in https://dxr.mozilla.org/mozilla-central/source/testing/talos/talos/test.py -- search that file for ASAP to see where). (2) Probably just drop the second line (or reword to be clearer, if you like). "This was a regression" doesn't make sense here. (what was a regression?) I think you mean "this helps avoid introducing a regression [...]" but really the whole patch-stack is what will help avoid introducing that regression, not just this one part, so this context is probably better to leave in comments on the bugzilla page (like comment 3), rather than in the metadata of the first changeset. ::: testing/talos/talos/tests/svg-static/svg-static.manifest:1 (Diff revision 1) > # gearflowers image RE the naming here: I suspect the directory needs to be named "svg_static" (with an underscore) for consistency with the sibling directories, which seem to use underscores for separation, like "svg_opacity" and "v8_7": https://dxr.mozilla.org/mozilla-central/source/testing/talos/talos/tests jmaher can confirm whether this matters. (I don't know why we use underscores in the folder names vs. dashes in the testsuite names. Maybe this was simply an oversight when we created the svg-opacity suite, and it's a mistake we shouldn't copy here? *shrug*, /me defers to jmaher) ::: testing/talos/talos/tests/svg-static/svg-static.manifest:5 (Diff revision 1) > # gearflowers image > -http://localhost/tests/svgx/gearflowers.svg > +%http://localhost/tests/svgx/gearflowers.svg > > # some generic image compositing tests > -http://localhost/tests/svgx/composite-scale.svg > +%http://localhost/tests/svgx/composite-scale.svg It looks like you're adding "%" characters before all of these lines, but you don't actually want to do that, since we're not intending for them to need any scripting. (I believe "%" just controls whether the harness measures load-time vs. deferring to the test to report arbitrary numbers via JavaScript. We did initially talk about adding % characters and making these tests script-dependent -- but that was before we decided to fork the suite instead. Now that we're forking the suite, we're hoping to leave these files as static (no scripting) & simply have the harness simply measure their load time (no "%"), without ASAP mode.)

(In reply to Daniel Holbert [:dholbert] from comment #4) > ::: testing/talos/talos/tests/svg-static/svg-static.manifest:1 > (Diff revision 1) > > # gearflowers image > > RE the naming here: I suspect the directory needs to be named "svg_static" > (with an underscore) for consistency with the sibling directories, which > seem to use underscores for separation, like "svg_opacity" and "v8_7": > https://dxr.mozilla.org/mozilla-central/source/testing/talos/talos/tests > > jmaher can confirm whether this matters. (I don't know why we use > underscores in the folder names vs. dashes in the testsuite names. Maybe > this was simply an oversight when we created the svg-opacity suite, and it's > a mistake we shouldn't copy here? *shrug*, /me defers to jmaher) Tagging jmaher for needinfo here (both on the directory name and the manifest filename). jmaher, should we match svg-opacity here (which has its manifest at "svg_opacity/svg_opacity.manifest" for some reason, with underscores?) https://dxr.mozilla.org/mozilla-central/source/testing/talos/talos/tests/svg_opacity

Comment on attachment 8812031 [details] Bug 1318530 - Add boilerplate code to test.py to enable new test suite 'tsvg_static'. https://reviewboard.mozilla.org/r/93942/#review94092 ::: testing/talos/talos/test.py:666 (Diff revision 1) > 'dom.send_after_paint_to_content': False} > filters = filter.ignore_first.prepare(5) + filter.median.prepare() > unit = 'ms' > > +@register_test() > +class tsvgx-static(PageloaderTest): Two nits: (1) drop the "x" (2) probably use an underscore instead of a dash here. Background: I *think* the "x" in "tsvgx" is meant to indicate "ASAP mode". (The only other testsuite that ends in an "x" is "tscrollx", which also uses ASAP mode) So, this new testsuite should *not* have an x. I think it wants to be named "tsvg-static". I don't know offhand whether a dash or an underscore is appropriate here, but as above we're matching tsvgr_opacity, then it'd want an underscore instead of a dash. (since the boilerplate here for that test uses an underscore) ::: testing/talos/talos/test.py:674 (Diff revision 1) > + for static content only. > + """ > + tpmanifest = '${talos}/tests/svg-static/svg-static.manifest' > + tpcycles = 1 > + tppagecycles = 25 > + tpmozafterpaint = False I think you might want to set this to true... Generally you probably want to copy all of these values from tsvgr_opacity (which is another static-svg-content talos suite). ::: testing/talos/talos/test.py:677 (Diff revision 1) > + """No ASAP mode""" > + preferences = {'layout.frame_rate': 0, > + 'docshell.event_starvation_delay_hint': 1, > + 'dom.send_after_paint_to_content': False} This triple-quoted thing is just a comment, I think. :) To actually get "No ASAP mode", you need to adjust the preferences = { ... } on the next line. You probably want to just remove the comment & "preferences" assignment entirely -- I think you want to match the configuration of "tsvgr_opacity" in this file, which has no explicit preferences.

there is no specific requirement for _ vs -. Overall these patches look great and the review comments are accurate :) an important thing is the % in the manifest for static, those specific test files need the pageloader addon to take measurement, they do not report numbers from internal code. we will need to add some code to talos.json for this new test: https://dxr.mozilla.org/mozilla-central/source/testing/talos/talos.json?q=path%3Atalos.json&redirect_type=single also update the wiki: https://wiki.mozilla.org/Buildbot/Talos/Tests luckily that is most of the work for this new test- i am glad to see that we can quickly identify the differences here and make this work.

Thanks! It looks like "_" is the standard, for Talos testsuites -- and I was misled by the fact that we have some documentation around "svg-opacity" (with a hyphen) but really other tests are consistent about using an underscore, and even svg-opacity uses an underscore around its name on its in-tree incarnations (and all mentions of "svg-opacity" area really typos which should actually say "tsvgr_opacity" at this point.) SO: for this bug, let's call the new testsuite "tsvg_static" everywhere we can (with a "t" and with an underscore).

(Note: the "r" in tsvgr_opacity is *not* something we need to copy in the new suite name here. That's a vestigial thing, which seems to stand for "row-major", and it's there because we used to run this suite in both row- and column-major configurations https://wiki.mozilla.org/Buildbot/Talos/Tests#Row_Major_vs._Column_Major . It seems we don't bother with that anymore, so there's no need to make that distinction for new suites. And in particular, the "r" does NOT stand for "rendering" as I initially thought it might. :))

(In reply to Joel Maher ( :jmaher) from comment #7) > we will need to add some code to talos.json for this new test: > https://dxr.mozilla.org/mozilla-central/source/testing/talos/talos.json?q=path%3Atalos.json&redirect_type=single (We're suspecting this should be added to the arrays for "svgr" and "svgr-e10s" in that file, since that's where all the other SVG-related tests are listed [and a few others]. Hopefully we're correct about that. :) I believe neerja's going to push an updated patch with that added in part 2.) > also update the wiki: > https://wiki.mozilla.org/Buildbot/Talos/Tests We might hold off on this until we've gotten some testing [on Try and/or on a few cycles after landing], to see if this actually sticks & solves the problem it's intending to solve.

These are try runs for the 'svgr' talos test suite for all platforms using the patches posted above: Without patch for Bug 1283302: https://treeherder.mozilla.org/#/jobs?repo=try&revision=158bce2092b25ad8643fdfa69ecc890fa2eed2ee With patch for Bug 1283302: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1438781bcf46cd783128b7ea2d2490e131ab011c

Comment on attachment 8812031 [details] Bug 1318530 - Add boilerplate code to test.py to enable new test suite 'tsvg_static'. https://reviewboard.mozilla.org/r/93942/#review94350 ::: testing/talos/talos/test.py:663 (Diff revision 4) > unit = 'ms' > > +@register_test() Per the linter orange on the Try run, it looks like we have a (robot-enforced!) coding style requirement that there are 2 blank lines between functions here. So, please add 1 additional blank line before & after this new function. ::: testing/talos/talos/test.py:692 (Diff revision 4) > unit = 'ms' > > - > @register_test() ...and please revert this change (removing a blank line here), since it triggers the same linter issue.

Here's a perfherder link for Talos comparisons between the Try runs in comment 17 (with the "new" revision being the one with bug 1283302's patch): https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=158bce2092b25ad8643fdfa69ecc890fa2eed2ee&newProject=try&newRevision=1438781bcf46cd783128b7ea2d2490e131ab011c&framework=1&showOnlyImportant=0 (This link will be more useful as more results come in.)

(Oh, now the retriggers seem to be working. Not sure what happened before. Sorry for the false alarm.)

Comment on attachment 8812030 [details] Bug 1318530 - Split talos test suite 'svgx' by moving all static tests to new suite 'svg_static'. https://reviewboard.mozilla.org/r/93940/#review94366 this looks great

Comment on attachment 8812031 [details] Bug 1318530 - Add boilerplate code to test.py to enable new test suite 'tsvg_static'. https://reviewboard.mozilla.org/r/93942/#review94368 I am glad this is mozafterpaint and not asap for the static files.

Comment on attachment 8812388 [details] Bug 1318530 - Add new test 'tsvg_static' to talos.json https://reviewboard.mozilla.org/r/94152/#review94370 so simple

looking at perfherder, I see some noise in the new svg_static tests, but the previous svgx test suite had some bi-modal data/noise and I think we split the noise out of svgx so it is all contained in a smaller set of tests :)

Yeah, I think the noise looks better-contained and much lower magnitude, so I think we're looking good. I'll go ahead and land this. Thanks, Neerja and Joel!

(to clarify: the noise is "lower magnitude" with this bug's patches & bug 1283302's patch, as compared to what we saw in tsvgx from these static SVG files when bug 1283302 landed on trunk most recently. (Now it's a few percent variation, rather than tens of percent). Hooray!) Setting needinfo=me to add this new SVG test to the Wiki page, assuming this sticks.

Drat -- I just noticed the autoland request (from 3 days ago) failed to execute due to some obscure hg error: { hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland searching for changes remote: abort: could not lock working directory of /repo/hg/mozilla/integration/autoland: Read-only file system abort: stream ended unexpectedly (got 0 bytes, expected 4) } I'll go ahead & land this directly on inbound, since I just landed bug 1283302 on inbound and this patch helps avoid talos regression reports from that bug.

Er, actually I'll land this on Autoland after all, so that we'll have at least one repo (Autoland) where we establish a baseline for this test's numbers before bug 1283302's patch is applied.

Pushed by dholbert@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/47dddc6dc461 Split talos test suite 'svgx' by moving all static tests to new suite 'svg_static'. r=jmaher https://hg.mozilla.org/integration/autoland/rev/15ac263b9c43 Add boilerplate code to test.py to enable new test suite 'tsvg_static'. r=jmaher https://hg.mozilla.org/integration/autoland/rev/d9f4e403f9a0 Add new test 'tsvg_static' to talos.json r=jmaher

https://hg.mozilla.org/mozilla-central/rev/47dddc6dc461 https://hg.mozilla.org/mozilla-central/rev/15ac263b9c43 https://hg.mozilla.org/mozilla-central/rev/d9f4e403f9a0

As a note, the perf alert for this patch shows on: https://treeherder.mozilla.org/perf.html#/alerts?id=4300

Thanks! The perf alert makes sense. The reported metric is the geometric mean of all the subtests, and in this bug we split some of the cheaper (lower-reported-value) subtests into a separate suite. So it makes sense that the reported metric is now higher, because it's the mean of a now-smaller set of values, which are on the high end of the original set. https://wiki.mozilla.org/Buildbot/Talos/Tests#tsvgx - Looking at the subtest breakdown, there's only one thing that doesn't make sense: https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=autoland&originalRevision=59e448d0eba8b55b2927cf8071f6d3beb2c56f5a&newProject=autoland&newRevision=d9f4e403f9a03a5f40ba14f039ab5b24dc6b352f&originalSignature=34dbf2ccb1c7a8c08b0fa6c3c33238e0f936fe64&newSignature=34dbf2ccb1c7a8c08b0fa6c3c33238e0f936fe64&framework=1 The old-test-to-new-test mapping there is a bit messed up (the new subtest results are named like "tsvgx 1.xml opt" for some reason, when really they should be named & mapped to "tsvgx hixie-001.xml opt"). However, if I manually do that mapping in my head, the numbers all match up and are in the same range (e.g. old 433.00, new 436.25, for the first subtest). So the values make sense -- I'm just confused about the mapping. jmaher, do you know what's going on there? Is there something we can do to keep the subtests with their original names? (Or does it matter?)

oh, this is a quirk in talos pageloader addon. If the subtests have similar names, it strips the common text from the beginning of the subtest. Prior to the change, we had a mix of tests, now we have one suite with just hixie-*, so we strip hixie- from the subtest name. I think it is ok- we have a "new test" and we can treat it as such- with perfherder we have alerts to help us indicate what the root cause is and map it to bugs, so the confusion is greatly reduced in 99% of the cases.

OK, thanks for clearing that up! All is well here, then, I think.

(In reply to Joel Maher ( :jmaher) from comment #7) > also update the wiki: > https://wiki.mozilla.org/Buildbot/Talos/Tests I updated the "tsvgx" documentation in that wiki page... https://wiki.mozilla.org/Buildbot/Talos/Tests#tsvgx ...to remove anything about the bits that have been removed (and e.g. update its mentions of "12 svg pages" to "7"). I deleted the example data associated with the now-removed files, and updated the numbering on the other files. I also added a new section for tsvg_static: https://wiki.mozilla.org/Buildbot/Talos/Tests#tsvg_static But, I wasn't sure what the right thing was to put for all of the fields, so I left some stuff blank. jmaher, could you help me with the following? (let me know how to come up with the correct value, or just make tweaks yourself if that's easier) 1) "Graphserver name": I have no idea what this is or where it's encoded. (According to that wiki page, for tsvgx this is apparently "SVG-ASAP". But I have no idea where (if anywhere) that's actually encoded in our infrastructure... Maybe this isn't actually a field that usefully exists anymore?) 2) "Description": I just copied the description from tsvgr_opacity ("Row Major and 25 cycles/page") but I'm not sure that's particularly useful description. I do have a more useful description directly below that table, though. 3) "Example data": I'm not sure how to generate the textual representation of the example data that's mentioned here (compact & semicolon-separated). I tried running "./mach talos-test --activeTests tsvg_static", but that prints out values split across several lines & space-separated instead of semicolon-separated.

thanks for getting this created. The graph server name is not needed anymore, I removed that. As for description, I moved your comment from below the *table* and put it in the description field- thanks for the comment. Regarding example data, this requires a little bit of text formatting to make it easier to see in a wiki. I took data from a live log on mozilla-inbound and copied out the raw data from pageloader and tweaked it to be readable.