Closed Bug 1472803 Opened 2 years ago Closed 2 years ago

port sunspider to raptor to run on windows

Categories

(Testing :: Raptor, enhancement)

enhancement
Not set

Tracking

(firefox63 fixed)

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: jmaher, Assigned: igoldan)

References

Details

(Whiteboard: [PI:July])

Attachments

(5 files)

sunspider currently runs on AWFY as a shell benchmark and a browser benchmark.  We want to support both in the move from AWFY -> raptor/jsshell.

We have a copy of sunspider in awfy:
https://github.com/mozilla/arewefastyet/tree/2696cbea7fbbe50c3a8cbf70a0ac3c70f135852e/benchmarks/SunSpider

it is also in the webkit repo:
https://github.com/WebKit/webkit/tree/master/PerformanceTests/SunSpider

I believe we want 1.0.1 for browser and 0.91 for shell; i.e. what we have in AWFY should be added to the tree (the size is <3MB) in the directory:
third_party/webkit/PerformanceTests/

To run the test, we will do something different for jsshell-bench; for the browser tests, I believe we just launch a webbrowser at the root of the directory and browser to it.  We should add a ?raptor option where we autorun what we want (all) and report (to the control server via window.post()).

For scoring, we might have to do some followup work to make this work smoother, lets get the test running on try and turned on for tier-3.  We can make the scoring more complete in a second bug/patch.
Whiteboard: [PI:July]
Assignee: nobody → igoldan
Comment on attachment 8991913 [details]
Bug 1472803 - Run SunSpider in browser mode

I've just started to integrate the test. Currently, Raptor only starts it and that's all. I've yet to figure out how the test talks back to the webext.
Attachment #8991913 - Flags: review?(rwood) → feedback?(rwood)
Attachment #8991913 - Flags: review?(rwood) → feedback?(rwood)
Comment on attachment 8991913 [details]
Bug 1472803 - Run SunSpider in browser mode

https://reviewboard.mozilla.org/r/256826/#review264112

Great progress - the benchmark does run via raptor on both Firefox and Google Chrome.

The results are making from the benchmark to the control server - in the terminal output look for this line to see the actual results:

raptor-control-server received webext_results: {u'name': u'raptor-sunspider-browser-firefox', u'type': u'benchmark', u'measurements': {u'sunspider': [[{u'math-spectral-norm': [2, 2, 2, 2, 2, 2, 2, 3, 2, 2],...

However the results are not processed for final output for perfherder, they are blank:

'raptor-output PERFHERDER_DATA: {"framework": {"name": "raptor"}, "suites": [{"extraOptions": [], "name": "raptor-sunspider-browser-firefox", "lowerIsBetter": false, "alertThreshold": 2.0, "subtests": [], "type": "benchmark"}]}'

So the next steps are:

- make sure the results being received by the control server are correct;
- add code to process those results and format for perfherder output (Hint:  /testing/raptor/raptor/output.py)

Please see other bugs i.e. Bug 1460741 where benchmarks were added to raptor, for more help on adding the code that processes the results.

Also please see the other issues I added in the review. Thanks!

::: testing/raptor/raptor/tests/raptor-sunspider.ini:15
(Diff revision 2)
> +page_timeout = 35000
> +unit = score
> +lower_is_better = false
> +alert_threshold = 2.0
> +
> +[raptor-sunspider-browser-firefox]

remove '-browser-'

::: testing/raptor/raptor/tests/raptor-sunspider.ini:16
(Diff revision 2)
> +unit = score
> +lower_is_better = false
> +alert_threshold = 2.0
> +
> +[raptor-sunspider-browser-firefox]
> +test_url = http://localhost:<port>/SunSpider/sunspider-1.0.1/sunspider-1.0.1/driver.html?raptor

Since the test_url is the same for both Firefox and Chrome, please move it to the [DEFAULT] section.

::: testing/raptor/raptor/tests/raptor-sunspider.ini:19
(Diff revision 2)
> +
> +[raptor-sunspider-browser-firefox]
> +test_url = http://localhost:<port>/SunSpider/sunspider-1.0.1/sunspider-1.0.1/driver.html?raptor
> +apps = firefox
> +
> +[raptor-sunspider-browser-chrome]

remove '-browser-'

::: testing/raptor/raptor/tests/raptor-sunspider.ini:20
(Diff revision 2)
> +[raptor-sunspider-browser-firefox]
> +test_url = http://localhost:<port>/SunSpider/sunspider-1.0.1/sunspider-1.0.1/driver.html?raptor
> +apps = firefox
> +
> +[raptor-sunspider-browser-chrome]
> +test_url = http://localhost:<port>/SunSpider/sunspider-1.0.1/sunspider-1.0.1/driver.html?raptor

Since the test_url is the same for both Firefox and Chrome, please move it to the [DEFAULT] section.

::: third_party/webkit/PerformanceTests/SunSpider/sunspider-0.9.1/json2.js:1
(Diff revision 2)
> +/*

The test runs sunspider-1.0.1. This adds sunspider-0.9.1 also. I wouldn't add version 0.9.1 if we aren't running it.

::: third_party/webkit/PerformanceTests/SunSpider/sunspider-1.0.1/sunspider-analyze-results.js:189
(Diff revision 2)
> +            result += "ERROR: Invalid test run.";
> +        else
> +            result += "ERROR: Some tests failed.";
> +        return result;
> +    }
> +    

There are alot of lines with extra spaces. I'm not sure if we run lint on js in the tree - but if we do, adding the sunspider source is going to trigger a whole bunch of lint errors (blank spaces, lines > 100, etc).

When you push to try be sure to run all the lint jobs - and see if any run on the third_party dir / sources as I'm not sure if we lint those or not.
Attachment #8991913 - Flags: feedback?(rwood) → feedback+
p.s. You'll also need to add to the taskcluster configs to add raptor-sunspider:

https://searchfox.org/mozilla-central/source/taskcluster/ci/test/raptor.yml

https://searchfox.org/mozilla-central/rev/b0275bc977ad7fda615ef34b822bba938f2b16fd/taskcluster/ci/test/test-sets.yml#83

You'll need that to run it on try, when you're ready (then you do ./mach try-fuzzy --full and you can search for the raptor jobs).
Depends on: 1476290
Comment on attachment 8991913 [details]
Bug 1472803 - Run SunSpider in browser mode

https://reviewboard.mozilla.org/r/256828/#review265054

this doesn't do output parsing in testing/raptor/raptor/output.py, this doesn't do the taskcluster configurations either; otherwise this is good (and if this is already done in the rwood patch, then this is really good)
Attachment #8991913 - Flags: review?(jmaher) → review+
Attachment #8993320 - Flags: review?(jmaher)
Attachment #8993321 - Flags: review?(jmaher)
Attachment #8993322 - Flags: review?(jmaher)
Comment on attachment 8993320 [details]
Bug 1472803 - Add TaskCluster configs for Raptor's SunSpider; tweak new raptor ini

https://reviewboard.mozilla.org/r/258098/#review265132

looks good
Attachment #8993320 - Flags: review?(jmaher) → review+
Comment on attachment 8993322 [details]
Bug 1472803 - Remove sunspider-0.9.1

https://reviewboard.mozilla.org/r/258102/#review265136

we might want to consider this as :rwood will add it for the jsshell-bench version; but this is better and I r+
Attachment #8993322 - Flags: review?(jmaher) → review+
Comment on attachment 8993321 [details]
Bug 1472803 - Add dedicated parse method

https://reviewboard.mozilla.org/r/258100/#review265140

this summarization code looks good, we need to add one thing, the final result to report which is missing.

The algorithm used on AWFY is:
sum([average(x) for x in results['replicates']])

basically we will have 20+ replicates for each of the 26 subtests, we need to take the average for the replicates resulting in a single value; given 26 single values, sum them up.

we would need to create something like benchmark_score:

    @classmethod
    def sunspider_score(cls, val_list):
        """
        sunspider_score: sum of the average value of each subtest replicates
        """
        results = [filter.average(i) for i, j in val_list]
        return filter.sum(results)
        
        
        
I don't know if we have .average or .sum, but those are easy to implement.
Attachment #8993321 - Flags: review?(jmaher) → review-
Comment on attachment 8993320 [details]
Bug 1472803 - Add TaskCluster configs for Raptor's SunSpider; tweak new raptor ini

https://reviewboard.mozilla.org/r/258098/#review265224

Awesome thanks
Attachment #8993320 - Flags: review?(rwood) → review+
Attachment #8993322 - Flags: review?(rwood)
Hey, thanks for the update, I'm having a look now and trying it out locally. In the mean time, you'll have to rebase, as we landed raptor webaudio yesterday so there are a few conflicts when applying your patches.

So please rebase, update mozreview, and push to try - please run this new raptor sunspider on both firefox and chrome, on win7/win10/linux64/osx. Thanks!
Flags: needinfo?(igoldan)
Comment on attachment 8993321 [details]
Bug 1472803 - Add dedicated parse method

https://reviewboard.mozilla.org/r/258100/#review265412

Thanks for the update - you are defintely on the right track here! A couple of updates required - please see the issues. Thanks! :)

::: testing/raptor/raptor/output.py:103
(Diff revision 2)
>                  LOG.error("output.summarize received unsupported test results type")
>                  return
>  
>          # if there is more than one subtest, calculate a summary result
>          if len(subtests) > 1:
>              suite['value'] = self.construct_results(vals, testname=test.name)

Just note here how self.construct_results is called. You'll need to add to the construct_results method (see note below).

::: testing/raptor/raptor/output.py:219
(Diff revision 2)
> +                                      'lowerIsBetter': test.lower_is_better,
> +                                      'name': sub,
> +                                      'replicates': []}
> +                _subtests[sub]['replicates'].extend([round(x, 3) for x in replicates])
> +
> +        # TODO: DRY object literal

nit: what does this comment mean?

::: testing/raptor/raptor/output.py:234
(Diff revision 2)
> +        vals = []
> +
> +        names = _subtests.keys()
> +        names.sort(reverse=True)
> +        for name in names:
> +            _subtests[name]['value'] = average = self._average(_subtests[name]['replicates'])

instead of self._average

Please use a filter from filter.py, like the other benchmark parsing methods do.

i.e. 

_subtests[name]['value'] = filter.mean(_subtests[name]['replicates'])

::: testing/raptor/raptor/output.py:239
(Diff revision 2)
> +            _subtests[name]['value'] = average = self._average(_subtests[name]['replicates'])
> +            subtests.append(_subtests[name])
> +            total_subtest['value'] += average
> +
> +            vals.append([_subtests[name]['value'], name])
> +        vals.append([total_subtest['value'], total_subtest['name']])

please remove this and the 'total_subtest['value'] += avreage line above - this should be a  part of calculating the final score.

i.e. you will add a 

@classmethod
def sunspider_score(cls, val_list):
    results = [i for i, j in val_list]
    return filter.sum(results)

Then you'll add to construct_results... and construct_results is called in summarize above

::: testing/raptor/raptor/output.py:243
(Diff revision 2)
> +            vals.append([_subtests[name]['value'], name])
> +        vals.append([total_subtest['value'], total_subtest['name']])
> +
> +        return subtests, vals
> +
> +    @classmethod

please remove this method and use the mean filter (filter.py) as noted above

::: testing/raptor/raptor/output.py:342
(Diff revision 2)
>              return self.benchmark_score(vals)
>          elif testname.startswith('raptor-speedometer'):
>              return self.speedometer_score(vals)
>          elif testname.startswith('raptor-stylebench'):
>              return self.stylebench_score(vals)
>          elif len(vals) > 1:

You'll add an elif for sunspider, calling your new sunspider_score method that you'll add
Attachment #8993321 - Flags: review?(rwood) → review-
(In reply to Robert Wood [:rwood] from comment #17)
> Hey, thanks for the update, I'm having a look now and trying it out locally.
> In the mean time, you'll have to rebase, as we landed raptor webaudio
> yesterday so there are a few conflicts when applying your patches.
> 
> So please rebase, update mozreview, and push to try - please run this new
> raptor sunspider on both firefox and chrome, on win7/win10/linux64/osx.
> Thanks!

Sure thing!
Flags: needinfo?(igoldan)
Comment on attachment 8993321 [details]
Bug 1472803 - Add dedicated parse method

https://reviewboard.mozilla.org/r/258100/#review265412

> nit: what does this comment mean?

We repeat that upper object literal 4 times. We should put it at class level or something.
I'll file a separate bug mentioning some small code refactors which are needed.
Comment on attachment 8993321 [details]
Bug 1472803 - Add dedicated parse method

I've addressed this in the last commit (the 5th one). But I still need r+ on it to land my patch.
Attachment #8993321 - Flags: review?(rwood)
Comment on attachment 8993321 [details]
Bug 1472803 - Add dedicated parse method

https://reviewboard.mozilla.org/r/258100/#review266086

r+ with the other commit in this patch which addresses these issues. In the future please use hg commit --amend.
Attachment #8993321 - Flags: review?(rwood) → review+
Comment on attachment 8994453 [details]
Bug 1472803 - Rebase on latest m-i, resolve conflicts, address PR requests

https://reviewboard.mozilla.org/r/259002/#review266090

Awesone, thanks for the udpate - great to land sunpsider in Raptor! I applied your latest patches and ran sunspider locally on OSX on firefox and google chrome, worked great.

Just one nit in this re: variable name (see below). Please update that nit and then please push your final patches to try again before landing.

Note: Several of these commits are just updates to the first commit. In the future please use 'hg commit --amend' so that you don't add a new commit just when doing post-review updates.

You can use multiple commits if it makes logical sense i.e. splitting up logical functionality/features, but when updating existing commits with fixes etc. It's best to use 'hg commit --amend' to just update the exiting commit with fixes. For example, in order to enable you to land this as/is, I had to r+ a previous commit that I had r- even though that commit itself wasn't updated; so it looks like I gave an r+ to a commit that wasn't ready to land (since the items to address it were in a new commit not the same one).

You can use hg evolve if you have multiple commits and need to update one of them - you can pull to whichever commit you are updating (i.e. hg update bookmark~1 to go to one commit below the top level one), make your changes, commit --ammend it, then do 'hg evolve --all', and push to mozreview. Then the existing commit will be updated instead of making a new one.

Or, if you just have one commit to start, just keep doing 'hg commit --amend' each time then you'll stay with the single conmit in the patch.

Please consider that for future patches, thanks :)

::: testing/raptor/raptor/output.py:384
(Diff revision 1)
>          score = 60 * 1000 / filter.geometric_mean(results) / correctionFactor
>          return score
>  
> +    @classmethod
> +    def sunspider_score(cls, val_list):
> +        results = [i for i, _ in val_list]

nit: please use a variable name other than "_"
Attachment #8994453 - Flags: review?(rwood) → review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 044d9fb394728b5b851b7df7368f1555de621f6d -d 4b6f4651d4e2: rebasing 474268:044d9fb39472 "Bug 1472803 - Run SunSpider in browser mode r=jmaher"
merging testing/raptor/webext/raptor/manifest.json
rebasing 474269:288fcb8e0ee6 "Bug 1472803 - Add TaskCluster configs for Raptor's SunSpider; tweak new raptor ini r=jmaher,rwood"
rebasing 474270:553dac290acc "Bug 1472803 - Add dedicated parse method r=rwood"
merging testing/raptor/raptor/output.py
rebasing 474271:8d00cc51dace "Bug 1472803 - Remove sunspider-0.9.1 r=jmaher"
rebasing 474272:2a8a13fa6f09 "Bug 1472803 - Rebase on latest m-i, resolve conflicts, address PR requests r=rwood" (tip)
merging testing/raptor/raptor/output.py
warning: conflicts while merging testing/raptor/raptor/output.py! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
(In reply to Robert Wood [:rwood] from comment #29)
> Note: Several of these commits are just updates to the first commit. In the
> future please use 'hg commit --amend' so that you don't add a new commit
> just when doing post-review updates.
> 
> You can use multiple commits if it makes logical sense i.e. splitting up
> logical functionality/features, but when updating existing commits with
> fixes etc. It's best to use 'hg commit --amend' to just update the exiting
> commit with fixes. For example, in order to enable you to land this as/is, I
> had to r+ a previous commit that I had r- even though that commit itself
> wasn't updated; so it looks like I gave an r+ to a commit that wasn't ready
> to land (since the items to address it were in a new commit not the same
> one).
> 
> You can use hg evolve if you have multiple commits and need to update one of
> them - you can pull to whichever commit you are updating (i.e. hg update
> bookmark~1 to go to one commit below the top level one), make your changes,
> commit --ammend it, then do 'hg evolve --all', and push to mozreview. Then
> the existing commit will be updated instead of making a new one.
> 
> Or, if you just have one commit to start, just keep doing 'hg commit
> --amend' each time then you'll stay with the single conmit in the patch.
> 
> Please consider that for future patches, thanks :)

Yes, will follow your advice from now on.
All tests pass: https://treeherder.mozilla.org/#/jobs?repo=try&author=igoldan@mozilla.com&fromchange=a47086407983a4bd8337ced931f8820ba0b976a5&filter-tier=1&filter-tier=2&filter-tier=3

Will file a separate bug for the jsshell version of the benchmark unless it has already been filed.
Pushed by igoldan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/da8bf37b6cdd
Run SunSpider in browser mode r=jmaher
https://hg.mozilla.org/integration/autoland/rev/f2b9b06acc7f
Add TaskCluster configs for Raptor's SunSpider; tweak new raptor ini r=jmaher,rwood
https://hg.mozilla.org/integration/autoland/rev/d64540743541
Add dedicated parse method r=rwood
https://hg.mozilla.org/integration/autoland/rev/e616f5a70b50
Remove sunspider-0.9.1 r=jmaher
https://hg.mozilla.org/integration/autoland/rev/b3b7f2d50c16
Rebase on latest m-i, resolve conflicts, address PR requests r=rwood
Depends on: 1478352
No longer depends on: 1478352
You need to log in before you can comment on or make changes to this bug.