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)
Testing
Talos
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)
932 bytes,
patch
|
rwood
:
review+
|
Details | Diff | Splinter Review |
10.96 KB,
patch
|
rwood
:
review+
|
Details | Diff | Splinter Review |
3.56 KB,
patch
|
rwood
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•7 years ago
|
||
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 :)
Comment 3•7 years ago
|
||
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?
Assignee | ||
Comment 4•7 years ago
|
||
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.
Assignee | ||
Comment 5•7 years ago
|
||
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
Assignee | ||
Comment 6•7 years ago
|
||
a diff of changes needed for speedometer to report numbers that we need- I could make this more official if needed.
Assignee | ||
Comment 7•7 years ago
|
||
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)
Comment 8•7 years ago
|
||
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)
Comment 9•7 years ago
|
||
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.
Assignee | ||
Comment 10•7 years ago
|
||
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)
Assignee | ||
Comment 11•7 years ago
|
||
: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)
Updated•7 years ago
|
Whiteboard: [PI:September] → [PI:October]
Comment 12•7 years ago
|
||
(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)
Assignee | ||
Comment 13•7 years ago
|
||
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)
Assignee | ||
Comment 14•7 years ago
|
||
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 15•7 years ago
|
||
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 16•7 years ago
|
||
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,
Assignee | ||
Comment 17•7 years ago
|
||
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.
Comment 18•7 years ago
|
||
Pushed by jmaher@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/c730f942ce30 copy speedometer into talos package. r=rwood
Assignee | ||
Comment 19•7 years ago
|
||
landed the easy patch, will get the remaining 2 patches ready for review later today.
Keywords: leave-open
Assignee | ||
Comment 20•7 years ago
|
||
Attachment #8916658 -
Flags: review?(rwood)
Assignee | ||
Comment 21•7 years ago
|
||
slightly updated
Attachment #8916658 -
Attachment is obsolete: true
Attachment #8916658 -
Flags: review?(rwood)
Attachment #8916751 -
Flags: review?(rwood)
Assignee | ||
Comment 22•7 years ago
|
||
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 23•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c730f942ce30
Comment 24•7 years ago
|
||
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 25•7 years ago
|
||
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+
Comment 26•7 years ago
|
||
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
Assignee | ||
Updated•7 years ago
|
Keywords: leave-open
Comment 27•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/054abb7cff0e https://hg.mozilla.org/mozilla-central/rev/2872f2358856
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in
before you can comment on or make changes to this bug.
Description
•