[webkit-reviews] review denied: [Bug 186414] Add benchmark for WebKit process launch times : [Attachment 342371] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Ben Richards
<benton_richards at apple.com>'s request for review:
Bug 186414: Add benchmark for WebKit process launch times
https://bugs.webkit.org/show_bug.cgi?id=186414

Attachment 342371: Patch

https://bugs.webkit.org/attachment.cgi?id=342371&action=review




--- Comment #5 from Geoffrey Garen <ggaren at apple.com> ---
Comment on attachment 342371
  --> https://bugs.webkit.org/attachment.cgi?id=342371
Patch

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

On the right track but needs some refinements.

> PerformanceTests/ChangeLog:9
> +	   * LaunchBench/new_tab.py: Added.
> +	   * LaunchBench/startup.py: Added.

Let's call this folder "LaunchTime" since these are launch time tests.

> PerformanceTests/LaunchBench/new_tab.py:1
> +#! /usr/bin/env python

I think you need to chmod +x this file in order to take advantage of the #!
notation. When I applied this patch locally, the script was not marked
executable.

> PerformanceTests/LaunchBench/new_tab.py:13
> +NUM_TESTS = 5

Let's call this NUM_ITERATIONS. "Iteration" is our term of art for running the
same test more than once. "NUM_TESTS" sounds like you're going to run more than
one kind of test.

> PerformanceTests/LaunchBench/new_tab.py:17
> +	     set startTime to (do JavaScript "+new Date()" in document 1)

I noticed that Safari doesn't allow "do JavaScript" by default. You have to
enable the Develop menu and then Enable Develop->Allow JavaScript From Apple
Events.

I think that means we should avoid "do JavaScript" if we can.

We want to make sure that this benchmark is as easy as possible for any
developer or bot to run. The less setup the better. Ideally, zero setup.

One option is to compute startTime locally in our script -- either by
converting our script to JavaScript and using "new Date" locally, or by using
AppleScript's notion of time, or by calling out to a system time service.
Another option is to ask the target browser to load a data URL that includes a
script to perform "new Date" and then ping our HTTP server with the result.
Another option is to stuff a time value in window.name, which AppleScript can
read out using "name of front document". There are probably more options too.

> PerformanceTests/LaunchBench/new_tab.py:19
> +	     set stopTime to (do JavaScript
"performance.timing.navigationStart" in document 1)

Same idea here.

One option is to load a data URL that pings us back with
performance.timing.navigationStart or stuffs that value into "name of front
document".

> PerformanceTests/LaunchBench/new_tab.py:21
> +	     do shell script "echo " & startTime & "-" & stopTime

I think this line belongs outside the tell block, right?

> PerformanceTests/LaunchBench/new_tab.py:36
> +    while True:
> +	   if i >= len(sys.argv):
> +	       break
> +	   arg = sys.argv[i]

You should use getopt or argparse rather than parsing options by hand. When I
was trying to figure out the argument format, I got some surprising results
that probably would have gone better with more robust options parsing. These
libraries also make it trivial to have short and long form arguments (like "-p"
or "--path").

Also, you should add a -h/--help option that lists the command line arguments
and demonstrates an example usage.

> PerformanceTests/LaunchBench/new_tab.py:47
> +	       if 'Chrome' in BROWSER_BUNDLE_PATH:

This logic should move outside of options parsing.

> PerformanceTests/LaunchBench/new_tab.py:80
> +    call(['open', '-a', BROWSER_BUNDLE_PATH, 'http://mysneakywebsite.com'])

We should probably lay off of Sam's hosting bill here :P. You can just open the
app without a URL argument.

Also, I think you need -F to discard past state.

> PerformanceTests/LaunchBench/new_tab.py:81
> +    time.sleep(1)

Is this needed for correctness, or is it just an attempt to wait for the system
to quiet down?

> PerformanceTests/LaunchBench/new_tab.py:103
> +def quit_browser():
> +    call(['osascript', '-e', 'quit app "%s"' % BROWSER_BUNDLE_PATH])

I think you might want to call quit at the start as well -- just in case the
app is already running.

> PerformanceTests/LaunchBench/new_tab.py:112
> +    launch_browser()
> +    time.sleep(1)

launch_browser() also calls sleep. How much sleeping do we want to do?

> PerformanceTests/LaunchBench/new_tab.py:123
> +	       # time.sleep(0.5)

Looks like you decided to remove this.

> PerformanceTests/LaunchBench/new_tab.py:140
> +    if len(tests) >= 5:
> +	   tests = tests[1:]
> +    elif len(tests) >= 10:
> +	   tests = tests[3:]
> +    elif len(tests) >= 20:
> +	   tests = tests[10:]

I think it's better to discard a constant amount. That way, adding more
iterations has a more predictable result. In this configuration, it's not valid
to compare n = 3 to n = 6 because you'll end up with different warmup amounts,
and you might confuse yourself into thinking that you added 3 extra runs when
in reality you only added 2 (with one discarded). It's also not valid to use n
= 3 at all, since you won't do warmup at all.

> PerformanceTests/LaunchBench/startup.py:1
> +#! /usr/bin/env python

Ditto about +x.

> PerformanceTests/LaunchBench/startup.py:53
> +    used = {}
> +    i = 1
> +    while True:

Same comment about args.

> PerformanceTests/LaunchBench/startup.py:143
> +    while True:
> +	   try:
> +	       server = SocketServer.TCPServer(('0.0.0.0', PORT), Handler)
> +	       break
> +	   except:
> +	       continue

I've seen this script hang, and I think it hangs here. Needs some debugging.

What's the reason for this idiom?

Here's one traceback I caught:

Exception happened during processing of request from ('127.0.0.1', 51182)
Traceback (most recent call last):
  File
"/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/SocketS
erver.py", line 295, in _handle_request_noblock
    self.process_request(request, client_address)
  File
"/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/SocketS
erver.py", line 321, in process_request
    self.finish_request(request, client_address)
  File
"/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/SocketS
erver.py", line 334, in finish_request
    self.RequestHandlerClass(request, client_address, self)
  File
"/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/SocketS
erver.py", line 657, in __init__
    self.finish()
  File
"/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/SocketS
erver.py", line 716, in finish
    self.wfile.close()
  File
"/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/socket.
py", line 283, in close
    self.flush()
  File
"/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/socket.
py", line 307, in flush
    self._sock.sendall(view[write_offset:write_offset+buffer_size])
error: [Errno 32] Broken pipe

> PerformanceTests/LaunchBench/startup.py:147
> +    server_thread.start()
> +    time.sleep(1)

Can we do something more reliable to wait for the server thread to start? Maybe
wait on a condition variable?

> PerformanceTests/LaunchBench/startup.py:164
> +	       for i in range(1000000):
> +		   sys.stdout.write('')  # spin to let browser finish closing

Is this better than time.sleep() in some way?