Bug 192905 - Add support to run-benchmark to use non-default copies of the browser apps
Summary: Add support to run-benchmark to use non-default copies of the browser apps
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-12-19 17:14 PST by Simon Fraser (smfr)
Modified: 2018-12-21 15:53 PST (History)
7 users (show)

See Also:


Attachments
Patch (14.95 KB, patch)
2018-12-19 17:16 PST, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (23.72 KB, patch)
2018-12-20 15:35 PST, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (24.34 KB, patch)
2018-12-20 15:47 PST, Simon Fraser (smfr)
dewei_zhu: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2018-12-19 17:14:31 PST
Add support to run-benchmark to use non-default copies of the browser apps
Comment 1 Simon Fraser (smfr) 2018-12-19 17:16:04 PST
Created attachment 357763 [details]
Patch
Comment 2 Simon Fraser (smfr) 2018-12-19 17:16:06 PST
<rdar://problem/46845840>
Comment 3 dewei_zhu 2018-12-19 17:32:50 PST
Comment on attachment 357763 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=357763&action=review

> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_safari_driver.py:40
> +        elif self._browser_path:

It's strange that launch_url takes browser_build_path as an argument but keep '_browser_path' as an instance variable.
It seems cleaner to me to change the signature of launch_url to also take browser_path as an  extra argument instead of changing the contractor of browser drivers.
What do you think?

> Tools/Scripts/webkitpy/benchmark_runner/run_benchmark.py:47
> +    parser.add_argument('--browser-path', help='Specify the path to a non-default copy of the target browser as a path to the .app.')

This may need to be mutual exclusive with '--build-directory'.
Refer: https://docs.python.org/2.7/library/argparse.html#argparse.ArgumentParser.add_mutually_exclusive_group
Comment 4 Stephanie Lewis 2018-12-19 17:46:25 PST
I agree with Dewei's comment about being consistent with browser_path and build_dir.  I still think it's a bad idea to add this as an option.  Using a bundle for performance testing can bite you in weird ways.
Comment 5 Ryosuke Niwa 2018-12-19 20:03:40 PST
Comment on attachment 357763 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=357763&action=review

> Tools/ChangeLog:10
> +        Add support for a --browser-path argument to run-benchmark, which allows you to use
> +        a custom app bundle for a given browser (only implemented for Safari at present).

We already have --build-directory? How is this option different from that?
Comment 6 Simon Fraser (smfr) 2018-12-20 11:47:35 PST
--build-directory doesn't let me point at a custom Safari.app (e.g. an STP).
Comment 7 dewei_zhu 2018-12-20 13:59:45 PST
If you build the binary locally, --build-directory is expecting a folder than contains Safari.app. Also this folder should contain all related frameworks.
Comment 8 Simon Fraser (smfr) 2018-12-20 14:16:12 PST
I want to run against pre-built app bundles (spades). Yes, I'm aware of the perf implications.
Comment 9 Simon Fraser (smfr) 2018-12-20 15:35:06 PST
Created attachment 357879 [details]
Patch
Comment 10 Simon Fraser (smfr) 2018-12-20 15:47:40 PST
Created attachment 357883 [details]
Patch
Comment 11 dewei_zhu 2018-12-20 17:11:08 PST
Comment on attachment 357883 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=357883&action=review

> Tools/Scripts/webkitpy/benchmark_runner/run_benchmark.py:49
> +
> +    group = parser.add_mutually_exclusive_group()
> +    group.add_argument('--browser-path', help='Specify the path to a non-default copy of the target browser as a path to the .app.')
> +    group.add_argument('--build-directory', dest='build_dir', help='Path to the browser executable (e.g. WebKitBuild/Release/).')
> +

Nit: It seems cleaner if we move this section below the last time we invoke parser.add_argument.
Comment 12 Simon Fraser (smfr) 2018-12-21 15:53:43 PST
https://trac.webkit.org/changeset/239522/webkit