Closed Bug 1460741 Opened 6 years ago Closed 6 years ago

Add 'speedometer' benchmark to raptor for firefox

Categories

(Testing :: Raptor, enhancement)

Product:
Testing
Component:
Raptor
Version:
Version 3
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox62 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: rwood, Assigned: rwood)

References

Details

Attachments

(2 files)

Bug 1460741 - Taskcluster configs for raptor speedometer on firefox;
59 bytes, text/x-review-board-request
jmaher
: review+
Details
Bug 1460741 - Add 'speedometer' benchmark to raptor for firefox;
59 bytes, text/x-review-board-request
jmaher
: review+
Details 
      No description provided.
Assignee: nobody → rwood
Status: NEW → ASSIGNED
Summary: Add firefox 'speedometer' benchmark to raptor → Add speedometer' benchmark to raptor for firefox
Summary: Add speedometer' benchmark to raptor for firefox → Add 'speedometer' benchmark to raptor for firefox
Comment on attachment 8975899 [details]
Bug 1460741 - Taskcluster configs for raptor speedometer on firefox;

https://reviewboard.mozilla.org/r/244096/#review250046

I didn't test this- the configs look great; I do look forward to this running everywhere
Attachment #8975899 - Flags: review?(jmaher) → review+
Comment on attachment 8975901 [details]
Bug 1460741 - Add 'speedometer' benchmark to raptor for firefox;

https://reviewboard.mozilla.org/r/244098/#review250138


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: testing/raptor/raptor/benchmark.py:100
(Diff revision 3)
> +    def setup_webserver(self, webserver):
> +        LOG.info("starting webserver on %r" % webserver)
> +        self.host, self.port = webserver.split(':')
> +
> +        return server.WebTestHttpd(port=int(self.port), doc_root=self.bindir,
> +                            routes=[("GET", "*", handlers.file_handler)])

Error: Continuation line under-indented for visual indent [flake8: E128]
(In reply to Robert Wood [:rwood] (PTO back 23-May) from comment #35)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=c66fc226a584a5a972f2bf8552da201fd4a05d80

Finally a green speedometer run on try! Only one issue left - calculating the final score isn't working; value is zero for some reason.
Depends on: 1462776
Comment on attachment 8975901 [details]
Bug 1460741 - Add 'speedometer' benchmark to raptor for firefox;

https://reviewboard.mozilla.org/r/244098/#review254506

too many nits/questions/concerns to r+;  I think this is not far from a r+ though, I look forward to the final state of this

::: testing/raptor/raptor/benchmark.py:39
(Diff revision 22)
> +            # back to:
> +            # mozilla-unified/obj-x86_64-apple-darwin17.4.0/
> +            # note, this may need to be updated per platform
> +            self.bindir = os.path.normpath(os.path.join(self.config['binary'],
> +                                                        '..', '..', '..', '..',
> +                                                        '..'))

I really don't like seeing 5x '..' - and it is OSX specific.

::: testing/raptor/raptor/benchmark.py:46
(Diff revision 22)
> +        # now add path for benchmark source; locally we put it in a bindir benchmarks
> +        # folder; in production the files are automatically copied to a different dir
> +        if self.config.get('run_local', False):
> +            self.bindir = os.path.join(self.bindir, 'testing', 'raptor', 'benchmarks')
> +        else:
> +            self.bindir = os.path.join(self.bindir, 'tests', 'raptor', 'raptor',

we have bindir, but isn't this really home/root/repo_path ?  maybe we call it something else?  I expect bindir to be where the binaries (tools or program we are testing) to live

::: testing/raptor/raptor/benchmark.py:68
(Diff revision 22)
> +    def get_webkit_source(self):
> +        # in production the build system auto copies webkit source into place;
> +        # but when run locally we need to do this manually, so that talos can find it
> +
> +        # source is repo/third_party...
> +        src = os.path.join(here, '..', '..', '..', 'third_party', 'webkit', 'PerformanceTests')

isn't src the same as bindir?

::: testing/raptor/raptor/benchmark.py:69
(Diff revision 22)
> +        # in production the build system auto copies webkit source into place;
> +        # but when run locally we need to do this manually, so that talos can find it
> +
> +        # source is repo/third_party...
> +        src = os.path.join(here, '..', '..', '..', 'third_party', 'webkit', 'PerformanceTests')
> +        dest = os.path.join(self.bindir)

what are you joining here?

::: testing/raptor/raptor/benchmark.py:78
(Diff revision 22)
> +
> +        LOG.info("copying webkit benchmarks from %s to %s" % (src, dest))
> +        try:
> +            shutil.copytree(src, dest)
> +        except Exception:
> +            LOG.critical("error copying webkit benchmarks from %s to %s" % (src, dest))

so this will delete and copy the benchmark everytime?  Some benchmarks are very large and benchmarks rarely change; could we cache them in a local spot and ignore them if they exist?

::: testing/raptor/raptor/output.py:238
(Diff revision 22)
>      def construct_results(self, vals, testname):
> -        if testname.startswith('v8_7'):
> +        if testname.startswith('raptor-v8_7'):
>              return self.v8_Metric(vals)
> -        elif testname.startswith('kraken'):
> +        elif testname.startswith('raptor-kraken'):
>              return self.JS_Metric(vals)
> -        elif testname.startswith('ares6'):
> +        elif testname.startswith('raptor-ares6'):

ares6 is a shell only benchmark; we can avoid it

::: testing/raptor/raptor/playback/mitmproxy.py:88
(Diff revision 22)
>              # back to:
>              # mozilla-unified/obj-x86_64-apple-darwin17.4.0/
>              # note, this may need to be updated per platform
>              self.bindir = os.path.normpath(os.path.join(self.config['binary'],
>                                                          '..', '..', '..', '..',
> -                                                        '..', 'testing', 'raptor'))
> +                                                        '..'))

more magic 5x '..' stuff that is OSX specific.  can we find a CWD that is set and used throughout- then we can reference things like firefox binary, test source, raptor tools, etc. easier.

::: testing/raptor/raptor/raptor.py:105
(Diff revision 22)
>          self.config['playback_recordings'] = test.get('playback_recordings', None)
>  
>      def run_test(self, test, timeout=None):
>          self.log.info("starting raptor test: %s" % test['name'])
> +        self.log.info(str(test))
> +        self.log.info(str(self.config))

are these for debugging?  if they are useful, add some context so we know what they represent.

::: testing/raptor/raptor/raptor.py:160
(Diff revision 22)
>  
>      def process_results(self):
> +        # when running locally output results in build/raptor.json; when running
> +        # in production output to a local.json to be turned into tc job artifact
> +        if self.config.get('run_local', False):
> +            if 'MOZ_DEVELOPER_REPO_DIR' in os.environ:

could we force MOZ_DEVELOPER_REPO_DIR to be set all the time?  would that be bindir in other cases or bindir/../../../../.. ?

::: testing/raptor/raptor/tests/raptor-speedometer.ini:15
(Diff revision 22)
> +test_url = /Speedometer/index.html?raptor
> +page_cycles = 5
> +page_timeout = 120000
> +unit = score
> +lower_is_better = false
> +alert_threshold = 2.0

I like the look of this format :)

::: testing/raptor/webext/raptor/measure.js:43
(Diff revision 22)
>      });
>    }
>  }
>  
>  function setup(settings) {
> +  if (settings.type == "pageload") {

I would prefer:
if settings.type != pageload:
    return

::: testing/raptor/webext/raptor/runner.js:69
(Diff revision 22)
>          pageCycles = settings.page_cycles;
>          testURL = settings.test_url;
> +
> +        // for benchmark tests, add the port prefix to the test url
> +        if (testType == "benchmark") {
> +          testURL = "http://localhost:" + benchmarkPort + testURL;

I would like to make this not hardcoded to localhost if possible.

::: third_party/webkit/PerformanceTests/Speedometer/resources/benchmark-report.js:5
(Diff revision 22)
>  // This file can be customized to report results as needed.
>  
>  (function () {
> -    if ((!window.testRunner && location.search != '?webkit' && location.hash != '#webkit') && location.search != '?gecko')
> +    if ((!window.testRunner && location.search != '?webkit' && location.hash != '#webkit')
> +         && location.search != '?gecko' && location.search != '?raptor')

please file a bug to remove the ?gecko and tpRecordTime and related talos bits so we can resolve that after turning off the talos version in the near future.
Attachment #8975901 - Flags: review?(jmaher) → review-
Comment on attachment 8975901 [details]
Bug 1460741 - Add 'speedometer' benchmark to raptor for firefox;

https://reviewboard.mozilla.org/r/244098/#review254506

> please file a bug to remove the ?gecko and tpRecordTime and related talos bits so we can resolve that after turning off the talos version in the near future.

Filed Bug 1466200.

Thanks for the review! I am currently working on addressing the other issues as well.
Comment on attachment 8975901 [details]
Bug 1460741 - Add 'speedometer' benchmark to raptor for firefox;

https://reviewboard.mozilla.org/r/244098/#review254506

Thanks for the thorough review, issues all addressed with update coming soon...

> so this will delete and copy the benchmark everytime?  Some benchmarks are very large and benchmarks rarely change; could we cache them in a local spot and ignore them if they exist?

Yes good point, updated so it only copies if it doesn't already exist, and also changed it so it only copies that specific benchmark about to run (not all of the them).

> could we force MOZ_DEVELOPER_REPO_DIR to be set all the time?  would that be bindir in other cases or bindir/../../../../.. ?

Actually this code only executes in production and MOZ_DEVELOPER_REPO_DIR is always set in production so this line isn't really needed to be checked, I guess I just put in for 'safety'.

> I would like to make this not hardcoded to localhost if possible.

Yes good point - thanks I moved this to the test INI - and I have the code just sub/add in the actual webserver port.
Comment on attachment 8975901 [details]
Bug 1460741 - Add 'speedometer' benchmark to raptor for firefox;

https://reviewboard.mozilla.org/r/244098/#review255192

::: testing/raptor/raptor/benchmark.py:63
(Diff revisions 22 - 23)
> -        # but when run locally we need to do this manually, so that talos can find it
> -
> -        # source is repo/third_party...
> -        src = os.path.join(here, '..', '..', '..', 'third_party', 'webkit', 'PerformanceTests')
> -        dest = os.path.join(self.bindir)
> +        # but when run locally we need to do this manually, so that raptor can find it
> +        if 'speedometer' in self.test['name']:
> +            # we only want to copy over the source for the benchmark that is about to run
> +            dest = os.path.join(self.bench_dir, 'Speedometer')
> +            src = os.path.join(os.environ['MOZ_DEVELOPER_REPO_DIR'], 'third_party',
> +                               'webkit', 'PerformanceTests', 'Speedometer')

hardcoding speedometer seems like a candidate for a quick refactor.

::: testing/raptor/webext/raptor/manifest.json:20
(Diff revisions 22 - 23)
>      {
> -      "matches": ["*://*.amazon.com/*"],
> +      "matches": ["*://*.amazon.com/*",
> +                  "*://*.facebook.com/*",
> +                  "*://*.google.com/*",
> +                  "*://*.youtube.com/*"],
>        "js": ["measure.js"]

please file a bug to find a more scalable way to manage this.  adding new pages, or new benchmarks will require changes here and that is highly error prone.

::: testing/raptor/webext/raptor/runner.js:73
(Diff revisions 22 - 23)
> +        // to add a port as it's accessed via proxy and the playback tool
> +        // however for benchmark tests, their source is served out on a local
> +        // webserver, so we need to swap in the webserver port into the testURL
>          if (testType == "benchmark") {
> -          testURL = "http://localhost:" + benchmarkPort + testURL;
> +          // just replace the '<port>' keyword in the URL with actual benchmarkPort
> +          testURL = testURL.replace("<port>", benchmarkPort);

for android we will need to get rid of localhost; I am fine going forward with this in the short term.
Attachment #8975901 - Flags: review?(jmaher) → review+
Blocks: 1460743
Thanks! Filed Bug 1466652
Pushed by rwood@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/574f89a09334
Taskcluster configs for raptor speedometer on firefox; r=jmaher
https://hg.mozilla.org/integration/autoland/rev/225b70969911
Add 'speedometer' benchmark to raptor for firefox; r=jmaher
https://hg.mozilla.org/mozilla-central/rev/574f89a09334
https://hg.mozilla.org/mozilla-central/rev/225b70969911
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Blocks: 1467823
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: