<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - Add PerfAutoRun to automate browser based performance benchmarks(e.g. Speedometer and JetStream)"
href="https://bugs.webkit.org/show_bug.cgi?id=144038#c14">Comment # 14</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - Add PerfAutoRun to automate browser based performance benchmarks(e.g. Speedometer and JetStream)"
href="https://bugs.webkit.org/show_bug.cgi?id=144038">bug 144038</a>
from <span class="vcard"><a class="email" href="mailto:rniwa@webkit.org" title="Ryosuke Niwa <rniwa@webkit.org>"> <span class="fn">Ryosuke Niwa</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=251564&action=diff" name="attach_251564" title="Patch">attachment 251564</a> <a href="attachment.cgi?id=251564&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=251564&action=review">https://bugs.webkit.org/attachment.cgi?id=251564&action=review</a>
<span class="quote">> Tools/Scripts/run-benchmark:9
> +logger.setLevel(logging.DEBUG)</span >
We probably don't want to debug logging by default.
You probably want to add an option to control logging level instead, leaving default.
<span class="quote">> Tools/Scripts/run-benchmark:25
> + args = parser.parse_args()</span >
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.
<span class="quote">> Tools/Scripts/run-benchmark:26
> + logger.info('Initializing program with following parameters')</span >
I'd make these debug log since the user had just specified these options.
<span class="quote">> Tools/Scripts/webkitpy/benchmark_runner/README.md:1
> +# PerfAutoRun</span >
Please rename this.
<span class="quote">> Tools/Scripts/webkitpy/benchmark_runner/benchmark_builder/benchmark_builder_factory.py:10
> +logger = logging.getLogger('PerfAutorun')</span >
Do _log = logging.getLogger(__name__)
<span class="quote">> Tools/Scripts/webkitpy/benchmark_runner/benchmark_builder/generic_benchmark_builder.py:12
> +logger = logging.getLogger('PerfAutoRun')</span >
Do _log = logging.getLogger(__name__)
<span class="quote">> Tools/Scripts/webkitpy/benchmark_runner/benchmark_builder/generic_benchmark_builder.py:27
> + if patch:</span >
We prefer early return over nested if's. Just do:
if patch:
return self.webRoot
instead.
<span class="quote">> Tools/Scripts/webkitpy/benchmark_runner/benchmark_builder/generic_benchmark_builder.py:32
> + if errorCode != 0:</span >
In WebKit coding styling, we don't use == or != against 0.
Just use "if errorCode:" instead.
<span class="quote">> Tools/Scripts/webkitpy/benchmark_runner/benchmark_builder/jetstream_benchmark_builder.py:10
> +logger = logging.getLogger('PerfAutoRun')</span >
Do _log = logging.getLogger(__name__)
<span class="quote">> Tools/Scripts/webkitpy/benchmark_runner/benchmark_builder/jetstream_benchmark_builder.py:25
> + if errorCode != 0:</span >
Ditto about not checking against 0.
<span class="quote">> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:20
> +logger = logging.getLogger('PerfAutoRun')</span >
Do _log = logging.getLogger(__name__)
<span class="quote">> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:55
> + # Wait for http server to launch and platform handler to clean
> + # the environment.</span >
I don't think this comment adds any value. Please remove it.
<span class="quote">> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:56
> + time.sleep(2)</span >
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.
<span class="quote">> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:64
> + if len(result) == 0:</span >
No need to call len. Just do "if not result:".
<span class="quote">> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:65
> + self.platformDriver.closeBrowsers()</span >
Why don't we put this into "finally:" statment intead so that we don't have to repeat it three times.
<span class="quote">> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:73
> + time.sleep(1)</span >
Why are we waiting for 1s?
<span class="quote">> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/browser_driver_factory.py:10
> +logger = logging.getLogger('PerfAutoRun')</span >
Do _log = logging.getLogger(__name__)
<span class="quote">> 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'},
> + },
> + }</span >
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.
<span class="quote">> 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</span >
Perhaps put this inside try & catch and spit out an error message instead?
<span class="quote">> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_chrome_driver.py:13
> +logger = logging.getLogger('PerfAutoRun')</span >
Do _log = logging.getLogger(__name__)
<span class="quote">> 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</span >
It seems like this will fit into a single line. Also use "FIXME:" prefix as done elsewhere.
<span class="quote">> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_chrome_driver.py:32
> + time.sleep(2)</span >
Why 2s?
<span class="quote">> 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</span >
Ditto.
<span class="quote">> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_safari_driver.py:13
> +logger = logging.getLogger('PerfAutoRun')</span >
Ditto.
<span class="quote">> 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)</span >
Really? It's unfortuante tht we don't have any other mechanism to ensure this :(
<span class="quote">> 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"
> + }
> + ]
> +}</span >
Now that we're making Builder subclasses, why don't we put this information as static class variables instead?
<span class="quote">> Tools/Scripts/webkitpy/benchmark_runner/generic_factory.py:9
> +logger = logging.getLogger('PerfAutoRun')</span >
Ditto.
<span class="quote">> 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)</span >
Wrong indentation. Each subsequent indentation is done by exactly 4 spaces.
<span class="quote">> Tools/Scripts/webkitpy/benchmark_runner/result_wrapper/merge_result_wrapper.py:7
> +class DictionaryMerger(BaseResultWrapper):</span >
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.
<span class="quote">> Tools/Scripts/webkitpy/benchmark_runner/result_wrapper/merge_result_wrapper.py:9
> + if len(dicts) == 0:</span >
Ditto. Use if not dicts.
<span class="quote">> Tools/Scripts/webkitpy/benchmark_runner/result_wrapper/result_wrapper_factory.py:10
> +logger = logging.getLogger('PerfAutoRun')</span >
Ditto.
<span class="quote">> Tools/Scripts/webkitpy/benchmark_runner/utils.py:8
> +logger = logging.getLogger('PerfAutoRun')</span >
Ditto.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>