Bug 147800 - Make run-benchmark cleanup more robust and minor code cleaning
Summary: Make run-benchmark cleanup more robust and minor code cleaning
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-08-07 16:18 PDT by dewei_zhu
Modified: 2015-08-10 16:44 PDT (History)
4 users (show)

See Also:


Attachments
Patch (10.16 KB, patch)
2015-08-07 16:35 PDT, dewei_zhu
no flags Details | Formatted Diff | Diff
Patch (9.67 KB, patch)
2015-08-10 13:22 PDT, dewei_zhu
no flags Details | Formatted Diff | Diff
Patch (8.98 KB, patch)
2015-08-10 15:01 PDT, dewei_zhu
no flags Details | Formatted Diff | Diff
Patch (11.31 KB, patch)
2015-08-10 15:31 PDT, dewei_zhu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description dewei_zhu 2015-08-07 16:18:20 PDT
Use python 'with' statement to make cleanup more robust and minor code cleaning
Comment 1 dewei_zhu 2015-08-07 16:35:35 PDT
Created attachment 258543 [details]
Patch
Comment 2 dewei_zhu 2015-08-07 16:39:11 PDT
Comment on attachment 258543 [details]
Patch

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

> Tools/Scripts/webkitpy/benchmark_runner/http_server_driver/simple_http_server_driver.py:27
> +        self._ip = '127.0.0.1'

We do not really need the external ip address since the http server is hosted locally or equivalent to locally.
Comment 3 Ryosuke Niwa 2015-08-07 16:52:22 PDT
Comment on attachment 258543 [details]
Patch

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

> Tools/ChangeLog:7
> +

Need some change description here.

> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:25
> +class built_benchmark(object):

Class names should be CamelCase.

> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:36
> +
> +    def __enter__(self):
> +        self._runner._web_root = self._runner._benchmark_builder.prepare(self._runner._plan_name, self._runner._plan)
> +
> +    def __exit__(self, exc_type, exc_value, traceback):
> +        if self._runner._benchmark_builder:
> +            self._runner._benchmark_builder.clean()

I don't think it makes much sense to have a separate class from BenchmarkBuilder just to implement __enter__ and __exit__.
Why can't we just add them to BenchmarkBuilder?

It's also strange for these two classes to rely on runner object.

> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:39
> +class test_environment(object):

Ditto. This should be named TestEnvironment.

> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:48
> +        self._runner._http_server_driver.serve(self._runner._web_root)
> +        self._runner._browser_driver.prepare_env(self._runner._device_id)
> +        url = urlparse.urljoin(self._runner._http_server_driver.base_url(), self._runner._plan_name + '/' + self._runner._plan['entry_point'])
> +        self._runner._browser_driver.launch_url(url, self._runner._device_id)

I don't think it's a good design to modify runner's property directly like this

> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:108
> +                with test_environment(self), timeout(self._plan['timeout']):
>                      result = self._http_server_driver.fetch_result()

It looks like _browser_driver is not accessed anywhere else so I don't think it needs to be be on _runner at all.
Ideally, I'd like see the code like this here:

with TestEnvironment(benchmark) as env, timeout(self._plan['timeout'])
    result = env.fetch_results()
Comment 4 dewei_zhu 2015-08-07 17:56:03 PDT
Comment on attachment 258543 [details]
Patch

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

>> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:108
>>                      result = self._http_server_driver.fetch_result()
> 
> It looks like _browser_driver is not accessed anywhere else so I don't think it needs to be be on _runner at all.
> Ideally, I'd like see the code like this here:
> 
> with TestEnvironment(benchmark) as env, timeout(self._plan['timeout'])
>     result = env.fetch_results()

Browser driver is specified by platform and browser which are provided by the parameter of BenchmarkRunner.__init__. The solution I can think of currently is storing those values in a structure, and supplying this structure as  parameter of TextEnvironment.__init__ . Does that sounds good?
Comment 5 Ryosuke Niwa 2015-08-07 20:47:04 PDT
Comment on attachment 258543 [details]
Patch

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

>>> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:108
>>>                      result = self._http_server_driver.fetch_result()
>> 
>> It looks like _browser_driver is not accessed anywhere else so I don't think it needs to be be on _runner at all.
>> Ideally, I'd like see the code like this here:
>> 
>> with TestEnvironment(benchmark) as env, timeout(self._plan['timeout'])
>>     result = env.fetch_results()
> 
> Browser driver is specified by platform and browser which are provided by the parameter of BenchmarkRunner.__init__. The solution I can think of currently is storing those values in a structure, and supplying this structure as  parameter of TextEnvironment.__init__ . Does that sounds good?

In that case, I don't think we want test_environment object all.
It's better for both browser_driver and http_server_driver to independently implement __enter__ and __exit__ (without a wrapper class)
so that we can directly instantiate those classes in this function.
Otherwise, the extra data structures and abstractions will only increase the code complexity.
Comment 6 dewei_zhu 2015-08-09 17:54:38 PDT
Comment on attachment 258543 [details]
Patch

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

>>>> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:108
>>>>                      result = self._http_server_driver.fetch_result()
>>> 
>>> It looks like _browser_driver is not accessed anywhere else so I don't think it needs to be be on _runner at all.
>>> Ideally, I'd like see the code like this here:
>>> 
>>> with TestEnvironment(benchmark) as env, timeout(self._plan['timeout'])
>>>     result = env.fetch_results()
>> 
>> Browser driver is specified by platform and browser which are provided by the parameter of BenchmarkRunner.__init__. The solution I can think of currently is storing those values in a structure, and supplying this structure as  parameter of TextEnvironment.__init__ . Does that sounds good?
> 
> In that case, I don't think we want test_environment object all.
> It's better for both browser_driver and http_server_driver to independently implement __enter__ and __exit__ (without a wrapper class)
> so that we can directly instantiate those classes in this function.
> Otherwise, the extra data structures and abstractions will only increase the code complexity.

Yes, we should avoid introducing unnecessary abstractions. The problem with both browser_driver and http_server is that they are instantiated by the factory function, are we expecting something like "with BrowserDriverFactory.create(....), HttpServerDriverFactory.create(....): "?
Comment 7 dewei_zhu 2015-08-09 18:07:57 PDT
Comment on attachment 258543 [details]
Patch

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

>>>>> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:108
>>>>>                      result = self._http_server_driver.fetch_result()
>>>> 
>>>> It looks like _browser_driver is not accessed anywhere else so I don't think it needs to be be on _runner at all.
>>>> Ideally, I'd like see the code like this here:
>>>> 
>>>> with TestEnvironment(benchmark) as env, timeout(self._plan['timeout'])
>>>>     result = env.fetch_results()
>>> 
>>> Browser driver is specified by platform and browser which are provided by the parameter of BenchmarkRunner.__init__. The solution I can think of currently is storing those values in a structure, and supplying this structure as  parameter of TextEnvironment.__init__ . Does that sounds good?
>> 
>> In that case, I don't think we want test_environment object all.
>> It's better for both browser_driver and http_server_driver to independently implement __enter__ and __exit__ (without a wrapper class)
>> so that we can directly instantiate those classes in this function.
>> Otherwise, the extra data structures and abstractions will only increase the code complexity.
> 
> Yes, we should avoid introducing unnecessary abstractions. The problem with both browser_driver and http_server is that they are instantiated by the factory function, are we expecting something like "with BrowserDriverFactory.create(....), HttpServerDriverFactory.create(....): "?

The purpose of having a 'with' statement is to reuse the code else where. Apparently, we do not reuse this code else where. Since 'with' statement is equivalent to 'try... finally...' block(http://effbot.org/zone/python-with-statement.htm), we may just use that instead.
Comment 8 dewei_zhu 2015-08-10 13:22:02 PDT
Created attachment 258645 [details]
Patch
Comment 9 Ryosuke Niwa 2015-08-10 14:24:08 PDT
Comment on attachment 258645 [details]
Patch

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

> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:72
> +                    result = None

Can we extract the code inside the inner try into a separate helper function?
Six levels of indentation below is just too hard to read.

> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:83
> +                    _log.error('No result or the server crashed. Something went wrong. Will skip current benchmark.\nError: {error}, Server return code: {return_code}, result: {result}'

This comment is inaccurate.  We're not skipping the current benchmark, we're exiting early.
I don't think we need to say anything about that at all.
In fact, I don't think we want to catch exceptions here at all since that'll allow the exception to propagate all the way up and exit the program anyway.

> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:87
> +                    if self._browser_driver:

How can self._browser_driver be ever False'y?  Since this is initialized in __init__, this should never be falsey.
If it could, e.g. factory.create could return None, then we should check that condition upfront and bail out
instead of trying to run the test and blowing up later.

> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:90
> +                    if self._http_server_driver:

Ditto.
Comment 10 dewei_zhu 2015-08-10 15:01:21 PDT
Created attachment 258655 [details]
Patch
Comment 11 Ryosuke Niwa 2015-08-10 15:10:43 PDT
Comment on attachment 258655 [details]
Patch

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

> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:63
> +        for iteration in xrange(1, int(self._plan['count']) + 1):

I think it's better for the caller of _run_benchmark to take care of reading the values out of plan

> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:87
> +        _log.info('Start to execute the plan')
> +        _log.info('Start a new benchmark')

These logging seems useless.  Please remove them.

> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:90
> +        benchmark_builder = BenchmarkBuilder()
> +        self._web_root = benchmark_builder.prepare(self._plan_name, self._plan)

This shouldn't be an instance variable since it's only used within _run_benchmark.
It should be an argument to _run_benchmark instead.

It's also strange that BenchmarkBuilder constructor takes no value and we have to call prepare separately.
We should combine these two and use "with" statement instead of manually calling clean() below.
Comment 12 dewei_zhu 2015-08-10 15:23:12 PDT
Comment on attachment 258655 [details]
Patch

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

>> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:90
>> +        self._web_root = benchmark_builder.prepare(self._plan_name, self._plan)
> 
> This shouldn't be an instance variable since it's only used within _run_benchmark.
> It should be an argument to _run_benchmark instead.
> 
> It's also strange that BenchmarkBuilder constructor takes no value and we have to call prepare separately.
> We should combine these two and use "with" statement instead of manually calling clean() below.

Good idea!
Comment 13 dewei_zhu 2015-08-10 15:31:47 PDT
Created attachment 258658 [details]
Patch
Comment 14 WebKit Commit Bot 2015-08-10 16:44:29 PDT
Comment on attachment 258658 [details]
Patch

Clearing flags on attachment: 258658

Committed r188237: <http://trac.webkit.org/changeset/188237>
Comment 15 WebKit Commit Bot 2015-08-10 16:44:33 PDT
All reviewed patches have been landed.  Closing bug.