Closed Bug 1353900 Opened 3 years ago Closed 3 years ago

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

Categories

(Testing :: Talos, enhancement)

Version 3
enhancement
Not set

Tracking

(firefox55 fixed)

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: bholley, Assigned: rwood)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Stylo])

Attachments

(4 files)

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.
I offered to put together an initial testcase of the form we'd like to add. Here's one for the bloom filter.
Attached file util.js
Attached file bloom-basic.html
Attached file bloom-basic-ref.html
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/
rwood, is there anything else you need to move forward here?
Flags: needinfo?(rwood)
CCing a few other people who might be interested in this framework.
(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.
Status: NEW → ASSIGNED
Flags: needinfo?(rwood)
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 :)
Attachment #8855440 - Flags: review?(jmaher) → review-
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.
(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?
maybe just "microbenchmarks" would be the best name.
(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.
Ok, if we don't want the string 'microbench' in the name, then I guess my preferred name is perf-reftest.
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.
(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
(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.
Flags: needinfo?(jmaher)
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
Flags: needinfo?(jmaher)
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.
Attachment #8855440 - Flags: review?(jmaher) → review+
(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).
Blocks: 1354296
: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
Flags: needinfo?(bobbyholley)
"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.
Flags: needinfo?(bobbyholley)
(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/
yes, a readme or txt file in-tree sounds good!
(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.
Pushed by rwood@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ad206b7a2644
Add talos stylo bloom-basic test; r=jmaher
https://hg.mozilla.org/mozilla-central/rev/ad206b7a2644
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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.
Depends on: 1356111
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #32)

Ok great thanks for the heads up!
Blocks: 1359145
You need to log in before you can comment on or make changes to this bug.