[Webkit-unassigned] [Bug 144038] Add PerfAutoRun to automate browser based performance benchmarks(e.g. Speedometer and JetStream)

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=144038

--- Comment #16 from dewei_zhu at apple.com ---
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/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.

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.

>> 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?

Doing try-catch for this import makes no difference. We do throw exception in BrowserDriverFactory not BrowserDriver.

>> 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 :(

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.

>> 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?

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.

>> 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.

My bad. I should put them in one line and this should have been fix since you've pointed it out last review.

>> 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.

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.

-- 
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/20150425/d7e94b6d/attachment-0001.html>