Bug 147800 – Make run-benchmark cleanup more robust and minor code cleaning

• Component: New Bugs
• Status: RESOLVED FIXED

Use python 'with' statement to make cleanup more robust and minor code cleaning

Created attachment 258543 [details]
Patch

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.

[Ryosuke Niwa]

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

[Ryosuke Niwa]

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

Created attachment 258645 [details]
Patch

[Ryosuke Niwa]

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.

Created attachment 258655 [details]
Patch

[Ryosuke Niwa]

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

Created attachment 258658 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 258658 [details]
Patch

Clearing flags on attachment: 258658

Committed r188237: <http://trac.webkit.org/changeset/188237>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.