[Webkit-unassigned] [Bug 144038] Add a script 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

Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #251599|review?                     |review-
              Flags|                            |

--- Comment #18 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 251599
  --> https://bugs.webkit.org/attachment.cgi?id=251599
Patch

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

> Tools/ChangeLog:7
> +

You should add a description as to what you're adding.

> Tools/Scripts/webkitpy/benchmark_runner/README.md:87
> +    * **result_wrapper**: the wrapper module you want to wrap the results. Current availble option is "MergeResultWrapper". To customize your own result wrapper, extends Utilities/ResultWrapper/BaseResultWrapper.py and register your module in Utilities/ResultWrapper/ResultWrapper.json

I really don't like that this is an option.  It's a code smell to have generalization we don't need right now.
It's always better to implement the bare minimum we need and generalize things as needed.
Otherwise, we'll end up extra layers of abstractions and configurations we don't need and cause a maintenance nightmare down the road.

> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:54
> +                self.browserDriver.launchUrl(self.composeUrl(self.httpServerDriver.baseUrl(), benchmark['entry_point']), self.buildDir)

Why don't we just use urlparse.urljoin?
https://docs.python.org/2/library/urlparse.html

> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:66
> +                        self.browserDriver.closeBrowsers()
> +                        _log.error('No result. Something went wrong')
> +                    self.browserDriver.closeBrowsers()

Why do we need to call closerBrowsers twice when result is None?
It seems like we just want to call it once.
Also, it seems like checking None-ness of result here seems strange.
Why don't we just put "_log.error('No result. Something went wrong')" in "except:" clause instead?

> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:74
> +    @classmethod
> +    def composeUrl(cls, baseUrl, relativePath):
> +        return '/'.join([baseUrl, relativePath])

We don't this function if we used urlparse.urljoin above.

> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:81
> +                fp.write(json.dumps(results))

json.dump(results, fp) will be more efficient.

> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:84
> +            _log.info('Cannot open output file: %s' % outputFile)
> +            _log.info('Results are:\n %s', json.dumps(results))

These should be errors.

> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/browser_driver_factory.py:16
> +    if not drivers:
> +        raise Exception("No driver was found in %s" % (driverFileName))

Why don't we just do "assert drivers" instead of repeating the exception message twice?

> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/browser_drivers.json:17
> +    "ios": {
> +         "safari": {
> +            "moduleName": "iOSSafariDriver", 
> +            "filePath": "browser_driver.ios_safari_driver"
> +        }
> +    } 

Does this thing exist? It seems that shouldn't have this entry if this patch doesn't include iOS implementation.

> Tools/Scripts/webkitpy/benchmark_runner/http_server_driver/http_server/twisted_http_server.py:24
> +    parser = argparse.ArgumentParser(
> +        description='python TwistedHTTPServer.py webroot')

Why don't we put this into a single line?

> Tools/Scripts/webkitpy/benchmark_runner/http_server_driver/simple_http_server_driver.py:29
> +            self.ip = [
> +                ip for ip in socket.gethostbyname_ex(
> +                    socket.gethostname())[2] if not ip.startswith("127.")][0]

This hsould all be in a single line.

> Tools/Scripts/webkitpy/benchmark_runner/http_server_driver/simple_http_server_driver.py:33
> +        except:
> +            _log.error('Cannot get the ip address of current machine')
> +            raise

Please pass the original exception object to raise as in:
except Exception:
…
    raise Exception
so that we get to see the original error.

> Tools/Scripts/webkitpy/benchmark_runner/http_server_driver/simple_http_server_driver.py:49
> +                    self.serverPort = psutil.Process(
> +                        self.serverProcess.pid).connections()[0][3][1]

Please put this in single line.

> Tools/Scripts/webkitpy/benchmark_runner/http_server_driver/simple_http_server_driver.py:70
> +                    except ValueError:
> +                        pass
> +                    except IndexError:
> +                        pass

Perhaps we should add comments about why we're ignoring these errors.

> Tools/Scripts/webkitpy/benchmark_runner/http_server_driver/simple_http_server_driver.py:75
> +            except:
> +                raise

Pleaes don't do this.  This will prevent us from seeing the actual error.

> Tools/Scripts/webkitpy/benchmark_runner/result_wrapper/base_result_wrapper.py:9
> +from abc import abstractmethod
> +
> +
> +class BaseResultWrapper(object):
> +    @abstractmethod
> +    def wrap(self, results):
> +        pass

I really don't think we should have this abstract class.
I can't think of any use case in which another implementation of merging results is needed.
The fact the type of results "wrapper" needs to be specified in the plan is an extra annoynace that's unwarrented.
r- because of this.

-- 
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/82d6a532/attachment.html>