[Webkit-unassigned] [Bug 186414] Add benchmark for WebKit process launch times

Sat Jun 9 18:10:27 PDT 2018

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

Geoffrey Garen <ggaren at apple.com> changed:

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

--- 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/SocketServer.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/SocketServer.py", line 321, in process_request
    self.finish_request(request, client_address)
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/SocketServer.py", line 334, in finish_request
    self.RequestHandlerClass(request, client_address, self)
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/SocketServer.py", line 657, in __init__
    self.finish()
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/SocketServer.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?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180610/ef0c8980/attachment.html>