1353900 - Add a mechanism for single-file perf microbenchmarks (needed for stylo)

Status: RESOLVED FIXED

(Testing :: Talos, enhancement)

Description

[Bobby Holley (:bholley)]

We discussed this on a call today, and rwood said he could put something together assuming jmaher approves.

The basic idea is that, to measure various style system optimizations, we want to be able to put together simple benchmark testcases that will get enormously slower if the optimization is missing. According to wlach and rwood, putting this infrastructure together is straightforward, though the additional process of adding new tests to it may require some fussing to determine baselines and whatnot.

There are two reasons I think the baselining should be pretty easy:
(1) These tests should have a very high signal-to-noise ratio.
(2) Where possible, we plan to also include a "reference" test that should have the same performance characteristics as the "test", unless the optimization is missing.

Comment 1

[Bobby Holley (:bholley)]

I offered to put together an initial testcase of the form we'd like to add. Here's one for the bloom filter.

Comment 2

[Bobby Holley (:bholley)]

Attachment: util.js

Comment 3

[Bobby Holley (:bholley)]

Attachment: bloom-basic.html

Comment 4

[Bobby Holley (:bholley)]

Attachment: bloom-basic-ref.html

Comment 5

[Bobby Holley (:bholley)]

With stock Gecko, both testcases take 360ms. With Gecko's bloom filter disabled, the "ref" takes 2 seconds and the "test" takes 11 seconds.

With stylo, both testcases take 65ms. \o/

Comment 6

[Bobby Holley (:bholley)]

rwood, is there anything else you need to move forward here?

Comment 7

[Bobby Holley (:bholley)]

CCing a few other people who might be interested in this framework.

Comment 8

[Robert Wood [:rwood]]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #6)
> rwood, is there anything else you need to move forward here?

No, thanks, that should be enough for me to try building it out as a Talos pageloader test.

Comment 9

[Robert Wood [:rwood]]

Attachment: Bug 1353900 - Add talos stylo bloom-basic test;

Review commit: https://reviewboard.mozilla.org/r/127288/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/127288/

Comment 10

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

Comment on attachment 8855440 [details]
Bug 1353900 - Add talos stylo bloom-basic test;

https://reviewboard.mozilla.org/r/127288/#review130020

r+ if you get the taskcluster configs in place- keep in mind this will require buildbot changes;

After this lands, we will need to...

* update: https://wiki.mozilla.org/Buildbot/Talos/Tests with information about the new tests.
* run on try with 20 retriggers on each operating system so we can ensure some basic level of stability.
* Will these run on all OS, or just linux64-stylo ?

::: testing/talos/talos.json:89
(Diff revision 1)
> +        "stylo": {
> +            "tests": ["bloom_basic", "bloom_basic_ref"]
> +        },
> +        "stylo-e10s": {
> +            "tests": ["bloom_basic", "bloom_basic_ref"]
> +        },

adding these in require buildbot-config changes- also taskcluster config changes as we schedule linux via taskcluster but execute via buildbot.  another option is to run this in an existing task- I would prefer a standalone task though :)

Comment 11

[Bobby Holley (:bholley)]

This looks great!

If you could call it something other than 'stylo' - maybe 'microbench' or 'html_microbench' or 'reftestperf' or something else, it would probably make it more useful long-term. The 'stylo' codename will go away, and we may want to use this mechanism for other things that are not strictly style-system-related.

Also, if there's any way (perhaps in a followup refactoring) to reduce the number of manifest modifications required to add a test, it will probably make it less error-prone for us to add new ones.

Comment 12

[Bobby Holley (:bholley)]

(In reply to Joel Maher ( :jmaher) from comment #10)
> Comment on attachment 8855440 [details]
> Bug 1353900 - Add talos stylo bloom-basic test;
> 
> https://reviewboard.mozilla.org/r/127288/#review130020
> 
> r+ if you get the taskcluster configs in place- keep in mind this will
> require buildbot changes;
> 
> After this lands, we will need to...
> 
> * update: https://wiki.mozilla.org/Buildbot/Talos/Tests with information
> about the new tests.
> * run on try with 20 retriggers on each operating system so we can ensure
> some basic level of stability.

Ok. I think these testcases are likely to be very stable.

> * Will these run on all OS, or just linux64-stylo ?

We want these on all OSes. These are generic style system benchmarks that should do the right thing under either Stylo or Gecko.

> 
> ::: testing/talos/talos.json:89
> (Diff revision 1)
> > +        "stylo": {
> > +            "tests": ["bloom_basic", "bloom_basic_ref"]
> > +        },
> > +        "stylo-e10s": {
> > +            "tests": ["bloom_basic", "bloom_basic_ref"]
> > +        },
> 
> adding these in require buildbot-config changes- also taskcluster config
> changes as we schedule linux via taskcluster but execute via buildbot. 
> another option is to run this in an existing task- I would prefer a
> standalone task though :)

Will we need buildbot changes each time we add another test like this, or only once to add the "html_microbench" suite?

Comment 13

[Bobby Holley (:bholley)]

maybe just "microbenchmarks" would be the best name.

Comment 14

[William Lachance (:wlach)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #13)
> maybe just "microbenchmarks" would be the best name.

That's liable to be confused with the old platform microbenchmarks (which I guess we're keeping around now?). Most of the existing Talos tests have pretty weird names (TART, CART, etc.) and I don't think it causes too many problems -- the important thing is that people know what you're talking about.

Comment 15

[Bobby Holley (:bholley)]

Ok, if we don't want the string 'microbench' in the name, then I guess my preferred name is perf-reftest.

Comment 16

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

we only need a buildbot change to get the new job added overall; adding a new test will end up being a subtest for the overall test and just need an in-tree bit of code landed.

Comment 17

[Robert Wood [:rwood]]

Comment on attachment 8855440 [details]
Bug 1353900 - Add talos stylo bloom-basic test;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127288/diff/1-2/

Comment 18

[Robert Wood [:rwood]]

Comment on attachment 8855440 [details]
Bug 1353900 - Add talos stylo bloom-basic test;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127288/diff/2-3/

Comment 19

[Robert Wood [:rwood]]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #15)
> Ok, if we don't want the string 'microbench' in the name, then I guess my
> preferred name is perf-reftest.

Ok, name is changed. FYI after these land, you'll be able to run this suite locally with:

./mach talos-test --suite perf-reftest

Or each test individually:

./mach talos-test --activeTests bloom_basic

Comment 20

[Robert Wood [:rwood]]

(In reply to Joel Maher ( :jmaher) from comment #16)
> we only need a buildbot change to get the new job added overall; adding a
> new test will end up being a subtest for the overall test and just need an
> in-tree bit of code landed.

:jmaher, is there any harm in landing this patch first without the configs (the tests won't run automatically anyway right? Is there any automation that just runs ALL talos tests together?). I'm hoping maybe :wcosta could add the tc/buildbot configs, I'm not really 100% sure how to do that when they use both buildbot and taskcluster. At least this one time, then I would know how to do it in the future.

Comment 21

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

yes, lets do that- we can get this working with mach, and then work on the scheduling.  

for taskcluster, add the definition here:
https://dxr.mozilla.org/mozilla-central/source/taskcluster/ci/test/tests.yml#1274

and the test set:
https://dxr.mozilla.org/mozilla-central/source/taskcluster/ci/test/test-sets.yml#66


This doesn't handle the buildbotbridge work, but thatis sort of automatic.

for buildbot-configs, add regular and e10s entries here:
https://hg.mozilla.org/build/buildbot-configs/file/c6ef67e55f4c/mozilla-tests/config.py#l197

Comment 22

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

Comment on attachment 8855440 [details]
Bug 1353900 - Add talos stylo bloom-basic test;

https://reviewboard.mozilla.org/r/127288/#review130050

changing to a r+ so scheduling can be done in another bug.

Comment 23

[Robert Wood [:rwood]]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #11)
> 
> Also, if there's any way (perhaps in a followup refactoring) to reduce the
> number of manifest modifications required to add a test, it will probably
> make it less error-prone for us to add new ones.

Thanks for the input :bholley. Right now there's not much we can do re: adding manifests. If we want each test to be separate and report separate numbers then at the moment they each need a manifest. Changing that would be a significant change to the talos framework.

However this is something we should consider for talos improvements in the future though (I like the idea you mentioned yesterday of just dropping in an html file, I've filed Bug 1354291 to at least research).

Comment 24

[Robert Wood [:rwood]]

:bholley, would you mind providing a short description of these two tests? Then I will include that when I update our Talos test descriptions page for this new suite [1]. Thanks!

[1] https://wiki.mozilla.org/Buildbot/Talos/Tests#Test_Descriptions

Comment 25

[Bobby Holley (:bholley)]

"Ensures that we correctly fast-reject ancestor selectors that clearly don't match any ancestor elements. If the bloom filter isn't working correctly, the 'test' page will take orders of magnitude longer. The 'ref' page will take somewhat longer, but not nearly as much."

Would it be possible for us to keep the test descriptions in-tree, and have the wiki link to them? That would eliminate yet-another step when adding new tests.

Comment 26

[William Lachance (:wlach)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #25)
> "Ensures that we correctly fast-reject ancestor selectors that clearly don't
> match any ancestor elements. If the bloom filter isn't working correctly,
> the 'test' page will take orders of magnitude longer. The 'ref' page will
> take somewhat longer, but not nearly as much."
> 
> Would it be possible for us to keep the test descriptions in-tree, and have
> the wiki link to them? That would eliminate yet-another step when adding new
> tests.

This would probably be a good use of the in-tree documentation:

http://gecko.readthedocs.io/en/latest/

Comment 27

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

yes, a readme or txt file in-tree sounds good!

Comment 28

[Robert Wood [:rwood]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3408e94494c0

Comment 29

[Robert Wood [:rwood]]

(In reply to Robert Wood [:rwood] from comment #28)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=3408e94494c0

Try looks great, performance tab details:

    bloom_basic http: opt: 552.96
    bloom_basic_ref http: opt: 554.67


Same but for the e10s job:

    bloom_basic http: opt e10s: 553.44
    bloom_basic_ref http: opt e10s: 552.88

I'm going to land this.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ad206b7a2644
Add talos stylo bloom-basic test; r=jmaher

Comment 31

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ad206b7a2644

Comment 32

[Bobby Holley (:bholley)]

Thanks rwood! Cameron is going to be working on adding a bunch more of these for the various style system optimizations we need to preserve for stylo. Given that there's some overhead involved in getting each one landing, we're going to iterate on the testcases for the next month or so, and then land them all together. So stay tuned.

Comment 33

[Robert Wood [:rwood]]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #32)

Ok great thanks for the heads up!