Closed Bug 1395247 Opened 7 years ago Closed 7 years ago

now that speedometer is landed in-tree for pgo generation, we should see if it is possible to run reliably inside of Talos

Categories

(Testing :: Talos, defect)

defect
Not set
normal

Tracking

(firefox58 fixed)

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: jmaher, Assigned: jmaher)

References

Details

(Whiteboard: [PI:October])

Attachments

(3 files, 3 obsolete files)

      No description provided.
speedometer landed in  bug 1356652.  we currently track it is a priority 1 test on the AWFY framework, if this is the only P1 test there, moving it to Talos sounds like a good path forward- it would be good to know if that works first :)
I have a different suggestion, but I don't know if it makes sense.  Would it be possible to have AWFY report test numbers to PerfHerder?  And if yes, would that give us features such as automatic regression detection for AWFY tests?
we report the AWFY numbers to perfherder already:
https://treeherder.mozilla.org/perf.html#/graphs?series=mozilla-inbound,1506441,1,5

the problem is the frequency and granularity of the numbers is so low, and this is not on many platforms.
a few steps here:
1) figure out in the build system how to get the speedometer test into the tests.talos.zip file
2) modify speedometer to post results to tpRecordTime(), this isn't too hard when run from index.html
3) add speedometer.manifest file to talos
4) add configs for speedometer

I am not sure about #1, that is sort of a mystery to me- I think #2, #3, and #4 are fairly easy
Attached patch speedometer.diff (obsolete) — Splinter Review
a diff of changes needed for speedometer to report numbers that we need- I could make this more official if needed.
I pushed this to try twice and compared it for stability:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=a69ef3fed85c&newProject=try&newRevision=955a564613bc01ea48214e2374fc22d85532a087&framework=1&filter=speed

overall this would be stable on all configurations.

:ted, do you know any tricks for getting the third_party/speedometer/* directory added to the tests.talos.zip file during packaging?
Flags: needinfo?(ted)
Blocks: 1404925
The code that generates test packages is a Python script nowadays, and the list of files that wind up in the Talos zip is specified here:
https://dxr.mozilla.org/mozilla-central/rev/41286177c59c74ec37961b1edea34cc90d6f6dc5/python/mozbuild/mozbuild/action/test_archive.py#367

You should be able to just add a new pattern there to include whatever you want in the zip.
Flags: needinfo?(ted)
Note that for meaningful Talos comparisons, we need to update our copy of Speedometer in tree to pick up important changes such as https://bugs.webkit.org/show_bug.cgi?id=172968.
Thanks for the note :ehsan, My attempt to generate a run in talos was independent of the score produced by the final calcuation, instead it is related to the 150 subtests that are reported.  The final score will not affect talos at all, we will be comparing build A<->B, not comparing Firefox<->Chrome or other published numbers- we are just looking for regressions in firefox- I know changing the summarization of the subtests can affect which regressions we see in some cases.  What I produced was looking at the raw 150 data points and doing a geometric mean inside of talos (default summation we use).

In this case if updating the outdated benchmark is not necessary for optimizing PGO, then I would argue that it is not necessary for comparing against itself- the tests are still the same.

Despite that, I think we should update to the latest benchmark data, is there a schedule we should use for this?  Other benchmarks we run have not been updated.

Also, who is the developer contact for this benchmark?  I have heard requests from :bkelly, :smaug in the past, but we need documentation about what this test is and what it does on our wiki: https://wiki.mozilla.org/Buildbot/Talos/Tests, this would include an owner to contact for any questions.
Flags: needinfo?(ehsan)
:ted, thanks for the link to the pattern usage in test_archive.py, is there a way to specify where the extra files get placed inside the .zip file?  Also we will need to reference the files locally when running via |./mach talos-test -a speedometer|.  Possibly we need to end up with 2 copies in tree
Flags: needinfo?(ted)
Whiteboard: [PI:September] → [PI:October]
Depends on: 1405371
(In reply to Joel Maher ( :jmaher) (UTC-5) from comment #10)
> Thanks for the note :ehsan, My attempt to generate a run in talos was
> independent of the score produced by the final calcuation, instead it is
> related to the 150 subtests that are reported.  The final score will not
> affect talos at all, we will be comparing build A<->B, not comparing
> Firefox<->Chrome or other published numbers- we are just looking for
> regressions in firefox- I know changing the summarization of the subtests
> can affect which regressions we see in some cases.  What I produced was
> looking at the raw 150 data points and doing a geometric mean inside of
> talos (default summation we use).
> 
> In this case if updating the outdated benchmark is not necessary for
> optimizing PGO, then I would argue that it is not necessary for comparing
> against itself- the tests are still the same.

Yes, this is all true.  I just meant that once developers use this facility to compare their pushes to see if they cause regressions or not, they should be using the real benchmark.  :-)

> Despite that, I think we should update to the latest benchmark data, is
> there a schedule we should use for this?  Other benchmarks we run have not
> been updated.

I just submitted patches to do this in bug 1405371.

> Also, who is the developer contact for this benchmark?  I have heard
> requests from :bkelly, :smaug in the past, but we need documentation about
> what this test is and what it does on our wiki:
> https://wiki.mozilla.org/Buildbot/Talos/Tests, this would include an owner
> to contact for any questions.

I think the SpiderMonkey team is the right owner.  I just confirmed with jandem on IRC.
Flags: needinfo?(ehsan)
one patch of a few.  Keep in mind this doesn't allow us to run locally via |mach talos-test -a speedometer|, that will require some other hacking.
Assignee: nobody → jmaher
Status: NEW → ASSIGNED
Flags: needinfo?(ted)
Attachment #8916079 - Flags: review?(rwood)
this is really hacky, ideally I will get this upstreamed to speedometer- probably in a more formal method.  If we are not comfortable with the hacks, then we can wait until it is officially in speedometer and we update speedometer in tree.
Attachment #8913719 - Attachment is obsolete: true
Attachment #8916080 - Flags: review?(rwood)
Comment on attachment 8916079 [details] [diff] [review]
copy third_party/speedometer -> talos

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

LGTM
Attachment #8916079 - Flags: review?(rwood) → review+
Comment on attachment 8916080 [details] [diff] [review]
hackery to speedometer in order to get results into talos

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

::: third_party/speedometer/resources/benchmark-report.js
@@ +73,5 @@
>              var fullNames = new Array;
>              for (var fullName in measuredValuesByFullName)
>                  fullNames.push(fullName);
>  
> +			if ( typeof tpRecordTime !== "undefined" ) {

alignment

@@ +81,5 @@
> +                }
> +                fullNames = new Array;
> +                for (var fullName in measuredValuesByFullName) {
> +                    //TODO: how to get the value '5' dynamically
> +                    var iterationCount = 5;

Not sure what you mean by the comment, and is this different than (from line 27):

        iterationCount: 5,
full try run with updated speedometer and mostly clean set of code:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d784eb3dbd46138999c0d9d5bb324e1a8410c7c7

the only quirky code would be anything I am adding to speedometer- I do summarize the speedometer subtests properly to get an official score.
Pushed by jmaher@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c730f942ce30
copy speedometer into talos package. r=rwood
landed the easy patch, will get the remaining 2 patches ready for review later today.
Keywords: leave-open
Attachment #8916658 - Flags: review?(rwood)
slightly updated
Attachment #8916658 - Attachment is obsolete: true
Attachment #8916658 - Flags: review?(rwood)
Attachment #8916751 - Flags: review?(rwood)
you can see the results here on try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9baa81a7fc538338ff88cec637af3d67e5bb9aad

that is the two patches on here (up for review), + slightly modified patch from bug 1404925.  Once this bug is resolved, we can finish up bug 1404925.
Attachment #8916080 - Attachment is obsolete: true
Attachment #8916080 - Flags: review?(rwood)
Comment on attachment 8916752 [details] [diff] [review]
hacks on speedometer benchmark to report results to talos

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

LGTM. Applied changes locally and ensured speedometer still runs fine stand-alone in browser with these upstream changes.
Attachment #8916752 - Flags: review+
Comment on attachment 8916751 [details] [diff] [review]
support in talos framework for speedometer

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

R+ with update to format_pagename as noted

::: testing/talos/talos/output.py
@@ +246,5 @@
> +        correctionFactor = 3
> +        results = [i for i, j in val_list]
> +        # speedometer has 9 subtests, and the 10th value is the test value we care about
> +        if len(results) != 160:
> +            raise Exception("Speedometer has 160 subtests, found: %s instead" % len(results))

I find this confusing, the comment above says 'speedometer has 9 subtests..' and then the code checks for / exception reports it's expecting '160 subtests'....

::: testing/talos/talos/results.py
@@ +256,5 @@
>      def format_pagename(self, page):
>          """
>          fix up the page for reporting
>          """
> +        return page

This should be removed now (Cool solution for getting around format pagename for speedometer btw)
Attachment #8916751 - Flags: review?(rwood) → review+
Pushed by jmaher@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/054abb7cff0e
add support to talos for speedometer, including no magical subtest page name formatting. r=rwood
https://hg.mozilla.org/integration/mozilla-inbound/rev/2872f2358856
changes to speedometer for reporting in talos. r=rwood
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/054abb7cff0e
https://hg.mozilla.org/mozilla-central/rev/2872f2358856
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.