[Webkit-unassigned] [Bug 144038] Add PerfAutoRun to automate browser based performance benchmarks(e.g. Speedometer and JetStream)
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Apr 24 14:08:33 PDT 2015
https://bugs.webkit.org/show_bug.cgi?id=144038
--- Comment #14 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 251564
--> https://bugs.webkit.org/attachment.cgi?id=251564
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=251564&action=review
> Tools/Scripts/run-benchmark:9
> +logger.setLevel(logging.DEBUG)
We probably don't want to debug logging by default.
You probably want to add an option to control logging level instead, leaving default.
> Tools/Scripts/run-benchmark:25
> + args = parser.parse_args()
Please add a blink line after this so that we can separate the list of arguments from the code below.
Probably another blank line is needed before BenchmarkRunner is instantiated too.
> Tools/Scripts/run-benchmark:26
> + logger.info('Initializing program with following parameters')
I'd make these debug log since the user had just specified these options.
> Tools/Scripts/webkitpy/benchmark_runner/README.md:1
> +# PerfAutoRun
Please rename this.
> Tools/Scripts/webkitpy/benchmark_runner/benchmark_builder/benchmark_builder_factory.py:10
> +logger = logging.getLogger('PerfAutorun')
Do _log = logging.getLogger(__name__)
> Tools/Scripts/webkitpy/benchmark_runner/benchmark_builder/generic_benchmark_builder.py:12
> +logger = logging.getLogger('PerfAutoRun')
Do _log = logging.getLogger(__name__)
> Tools/Scripts/webkitpy/benchmark_runner/benchmark_builder/generic_benchmark_builder.py:27
> + if patch:
We prefer early return over nested if's. Just do:
if patch:
return self.webRoot
instead.
> Tools/Scripts/webkitpy/benchmark_runner/benchmark_builder/generic_benchmark_builder.py:32
> + if errorCode != 0:
In WebKit coding styling, we don't use == or != against 0.
Just use "if errorCode:" instead.
> Tools/Scripts/webkitpy/benchmark_runner/benchmark_builder/jetstream_benchmark_builder.py:10
> +logger = logging.getLogger('PerfAutoRun')
Do _log = logging.getLogger(__name__)
> Tools/Scripts/webkitpy/benchmark_runner/benchmark_builder/jetstream_benchmark_builder.py:25
> + if errorCode != 0:
Ditto about not checking against 0.
> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:20
> +logger = logging.getLogger('PerfAutoRun')
Do _log = logging.getLogger(__name__)
> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:55
> + # Wait for http server to launch and platform handler to clean
> + # the environment.
I don't think this comment adds any value. Please remove it.
> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:56
> + time.sleep(2)
Why are we sleeping for 2s? Are we waiting for httpd to launch?
If so, we should instead check for pid file to be generated.
Waiting for 2s is neither sufficient nor necesary condition for httpd to have launched.
See how apache_http_server.py (somewhere in webkitpy) does this.
In fact, you might wanna just use that class with a new httpd.conf.
Also, we should probably be doing it in httpServerDriver.serve.
> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:64
> + if len(result) == 0:
No need to call len. Just do "if not result:".
> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:65
> + self.platformDriver.closeBrowsers()
Why don't we put this into "finally:" statment intead so that we don't have to repeat it three times.
> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:73
> + time.sleep(1)
Why are we waiting for 1s?
> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/browser_driver_factory.py:10
> +logger = logging.getLogger('PerfAutoRun')
Do _log = logging.getLogger(__name__)
> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/browser_driver_factory.py:21
> + logger.warn('Loading %s failed, start using pre-defined drivers' % (driverFileName))
> + drivers = {
> + 'osx': {
> + 'safari': {'moduleName': 'OSXSafariDriver', 'filePath': 'browser_driver.osx_safari_driver'},
> + 'chrome': {'moduleName': 'OSXChromeDriver', 'filePath': 'browser_driver.osx_chrome_driver'},
> + },
> + }
I don't think we need a fallback code here. It's just duplicating the code.
In fact, it's probably a good idea to do a full stop if the configuration file specified by the user doesn't exist.
> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_chrome_driver.py:10
> +# We assume that this handle can only be used when the platform is OSX
> +from AppKit import NSRunningApplication
> +from browser_driver import BrowserDriver
Perhaps put this inside try & catch and spit out an error message instead?
> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_chrome_driver.py:13
> +logger = logging.getLogger('PerfAutoRun')
Do _log = logging.getLogger(__name__)
> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_chrome_driver.py:24
> + # may need to be modified for develop build, such as setting up
> + # libraries
It seems like this will fit into a single line. Also use "FIXME:" prefix as done elsewhere.
> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_chrome_driver.py:32
> + time.sleep(2)
Why 2s?
> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_safari_driver.py:10
> +# We assume that this handle can only be used when the platform is OSX.
> +from AppKit import NSRunningApplication
> +from browser_driver import BrowserDriver
Ditto.
> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_safari_driver.py:13
> +logger = logging.getLogger('PerfAutoRun')
Ditto.
> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_safari_driver.py:29
> + # Stop for initialization of the safari process, otherwise, open
> + # command may use the system safari.
> + time.sleep(3)
Really? It's unfortuante tht we don't have any other mechanism to ensure this :(
> Tools/Scripts/webkitpy/benchmark_runner/data/plans/jetstream.plan:15
> +{
> + "http_server_driver": "SimpleHTTPServerDriver",
> + "benchmarks": [
> + {
> + "timeout" : 600,
> + "count": 5,
> + "benchmark_builder": "JetStreamBenchmarkBuilder",
> + "original_benchmark": "../../../../PerformanceTests/JetStream",
> + "benchmark_patch": "data/patches/JetStream.patch",
> + "entry_point": "JetStream/JetStream-1.0.1/index.html",
> + "result_wrapper": "MergeResultWrapper",
> + "output_file": "jetstream.result"
> + }
> + ]
> +}
Now that we're making Builder subclasses, why don't we put this information as static class variables instead?
> Tools/Scripts/webkitpy/benchmark_runner/generic_factory.py:9
> +logger = logging.getLogger('PerfAutoRun')
Ditto.
> Tools/Scripts/webkitpy/benchmark_runner/http_server_driver/simple_http_server_driver.py:81
> + p = subprocess.Popen(' '.join(['/usr/sbin/lsof',
> + '-a',
> + '-iTCP',
> + '-sTCP:LISTEN',
> + '-p',
> + str(self.serverProcess.pid)]),
> + shell=True,
> + stdout=subprocess.PIPE,
> + stderr=subprocess.PIPE)
Wrong indentation. Each subsequent indentation is done by exactly 4 spaces.
> Tools/Scripts/webkitpy/benchmark_runner/result_wrapper/merge_result_wrapper.py:7
> +class DictionaryMerger(BaseResultWrapper):
I think this is unwarrent generization since this is the only class that inherits from BaseResultWrapper.
Just get rid of BaseResultWrapper and use this class directly.
In fact, this could just be a methods in benchmark_runner instead.
> Tools/Scripts/webkitpy/benchmark_runner/result_wrapper/merge_result_wrapper.py:9
> + if len(dicts) == 0:
Ditto. Use if not dicts.
> Tools/Scripts/webkitpy/benchmark_runner/result_wrapper/result_wrapper_factory.py:10
> +logger = logging.getLogger('PerfAutoRun')
Ditto.
> Tools/Scripts/webkitpy/benchmark_runner/utils.py:8
> +logger = logging.getLogger('PerfAutoRun')
Ditto.
--
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150424/99139604/attachment-0001.html>
More information about the webkit-unassigned
mailing list