<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#c16">Comment # 16</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:dewei_zhu@apple.com" title="dewei_zhu@apple.com">dewei_zhu@apple.com</a>
</span></b>
<pre>Comment on <span class="bz_obsolete"><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/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.</span >
It have been proved that we do not have to sleep 2 seconds since the httpServerDriver will try to fetch the port number it is serving. Once it get the port number, this also indicates that the server is running.
I didn't find an easy way to use apache_http_server class as we want to have a '/shutdown' page which will terminate http server once it get hit.
<span class="quote">>> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_chrome_driver.py:10
>> +from browser_driver import BrowserDriver
>
> Perhaps put this inside try & catch and spit out an error message instead?</span >
Doing try-catch for this import makes no difference. We do throw exception in BrowserDriverFactory not BrowserDriver.
<span class="quote">>> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_safari_driver.py:29
>> + time.sleep(3)
>
> Really? It's unfortuante tht we don't have any other mechanism to ensure this :(</span >
It's kind of a race condition. It would be great that we can find a way to merge two subprocess.Popen (line 26 and 30) in to one command.
<span class="quote">>> Tools/Scripts/webkitpy/benchmark_runner/data/plans/jetstream.plan:15
>> +}
>
> Now that we're making Builder subclasses, why don't we put this information as static class variables instead?</span >
I think it would be better to follow the pattern of generic benchmark(e.g. speedometer). If we remove keys like 'original_benchmark' we need to add extra if conditions to check whether it is provided in '.plan' file.
<span class="quote">>> Tools/Scripts/webkitpy/benchmark_runner/http_server_driver/simple_http_server_driver.py:81
>> + stderr=subprocess.PIPE)
>
> Wrong indentation. Each subsequent indentation is done by exactly 4 spaces.</span >
My bad. I should put them in one line and this should have been fix since you've pointed it out last review.
<span class="quote">>> 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.</span >
I think I should call it ResultWrapperInterface rather than BaseResultWrapper. And since we provide the ability for others to import benchmarks, I think it would be better to allow them to define other wrappers.</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>