Bug 186414 - Add benchmark for WebKit process launch times
Summary: Add benchmark for WebKit process launch times
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ben Richards
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-06-07 15:49 PDT by Ben Richards
Modified: 2018-07-20 09:02 PDT (History)
7 users (show)

See Also:


Attachments
Patch (10.30 KB, patch)
2018-06-09 15:10 PDT, Ben Richards
no flags Details | Formatted Diff | Diff
Patch (13.27 KB, patch)
2018-06-11 16:29 PDT, Ben Richards
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews206 for win-future (12.75 MB, application/zip)
2018-06-12 18:44 PDT, Build Bot
no flags Details
benchmark startup and new tab time (10.74 KB, patch)
2018-06-22 12:56 PDT, Ben Richards
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews204 for win-future (12.74 MB, application/zip)
2018-06-22 21:58 PDT, Build Bot
no flags Details
Patch (11.19 KB, patch)
2018-06-26 16:29 PDT, Ben Richards
no flags Details | Formatted Diff | Diff
Patch (11.03 KB, patch)
2018-06-29 16:42 PDT, Ben Richards
no flags Details | Formatted Diff | Diff
Patch (11.22 KB, patch)
2018-06-29 20:14 PDT, Ben Richards
no flags Details | Formatted Diff | Diff
Patch (13.65 KB, patch)
2018-07-03 17:51 PDT, Ben Richards
no flags Details | Formatted Diff | Diff
Patch (19.04 KB, patch)
2018-07-11 18:01 PDT, Ben Richards
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews204 for win-future (12.76 MB, application/zip)
2018-07-11 19:37 PDT, Build Bot
no flags Details
Patch (19.21 KB, patch)
2018-07-14 20:53 PDT, Ben Richards
no flags Details | Formatted Diff | Diff
Patch (20.54 KB, patch)
2018-07-16 14:53 PDT, Ben Richards
no flags Details | Formatted Diff | Diff
Patch (33.97 KB, patch)
2018-07-17 15:43 PDT, Ben Richards
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews206 for win-future (13.05 MB, application/zip)
2018-07-17 18:49 PDT, Build Bot
no flags Details
Patch (39.18 KB, patch)
2018-07-18 17:03 PDT, Ben Richards
no flags Details | Formatted Diff | Diff
Patch (35.73 KB, patch)
2018-07-18 20:52 PDT, Ben Richards
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews205 for win-future (13.01 MB, application/zip)
2018-07-19 00:09 PDT, Build Bot
no flags Details
Patch for landing (35.65 KB, patch)
2018-07-19 13:55 PDT, Ben Richards
no flags Details | Formatted Diff | Diff
Patch for landing (35.67 KB, patch)
2018-07-19 14:13 PDT, Ben Richards
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ben Richards 2018-06-07 15:49:39 PDT
...
Comment 1 Alexey Proskuryakov 2018-06-07 20:17:48 PDT
Seeing Safari tests in WebKit would be quite surprising. What is the benefit of having these in WebKit?
Comment 2 Ben Richards 2018-06-09 15:10:25 PDT
Created attachment 342371 [details]
Patch
Comment 3 Build Bot 2018-06-09 15:12:09 PDT
Attachment 342371 [details] did not pass style-queue:


ERROR: PerformanceTests/LaunchBench/startup.py:184:  [main] Module 'numpy' has no 'array' member  [pylint/E1101] [5]
ERROR: PerformanceTests/LaunchBench/startup.py:187:  [main] Module 'numpy' has no 'mean' member  [pylint/E1101] [5]
ERROR: PerformanceTests/LaunchBench/startup.py:187:  [main] Module 'numpy' has no 'float64' member  [pylint/E1101] [5]
ERROR: PerformanceTests/LaunchBench/startup.py:188:  [main] Module 'numpy' has no 'std' member  [pylint/E1101] [5]
ERROR: PerformanceTests/LaunchBench/startup.py:188:  [main] Module 'numpy' has no 'float64' member  [pylint/E1101] [5]
ERROR: PerformanceTests/LaunchBench/new_tab.py:141:  [main] Module 'numpy' has no 'array' member  [pylint/E1101] [5]
ERROR: PerformanceTests/LaunchBench/new_tab.py:144:  [main] Module 'numpy' has no 'mean' member  [pylint/E1101] [5]
ERROR: PerformanceTests/LaunchBench/new_tab.py:144:  [main] Module 'numpy' has no 'float64' member  [pylint/E1101] [5]
ERROR: PerformanceTests/LaunchBench/new_tab.py:145:  [main] Module 'numpy' has no 'std' member  [pylint/E1101] [5]
ERROR: PerformanceTests/LaunchBench/new_tab.py:145:  [main] Module 'numpy' has no 'float64' member  [pylint/E1101] [5]
Total errors found: 10 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Geoffrey Garen 2018-06-09 17:13:50 PDT
(In reply to Alexey Proskuryakov from comment #1)
> Seeing Safari tests in WebKit would be quite surprising. What is the benefit
> of having these in WebKit?

We're going to use these tests to optimize WebKit startup time.

There's nothing particularly Safari-specific about these tests. They can run against any target that supports a little scripting.
Comment 5 Geoffrey Garen 2018-06-09 18:10:27 PDT
Comment on attachment 342371 [details]
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?
Comment 6 Ben Richards 2018-06-11 16:29:56 PDT
Created attachment 342479 [details]
Patch
Comment 7 Build Bot 2018-06-11 16:31:11 PDT
Attachment 342479 [details] did not pass style-queue:


ERROR: PerformanceTests/LaunchTime/startup.py:167:  [main] Module 'numpy' has no 'array' member  [pylint/E1101] [5]
ERROR: PerformanceTests/LaunchTime/startup.py:170:  [main] Module 'numpy' has no 'mean' member  [pylint/E1101] [5]
ERROR: PerformanceTests/LaunchTime/startup.py:170:  [main] Module 'numpy' has no 'float64' member  [pylint/E1101] [5]
ERROR: PerformanceTests/LaunchTime/startup.py:171:  [main] Module 'numpy' has no 'std' member  [pylint/E1101] [5]
ERROR: PerformanceTests/LaunchTime/startup.py:171:  [main] Module 'numpy' has no 'float64' member  [pylint/E1101] [5]
ERROR: PerformanceTests/LaunchTime/new_tab.py:240:  [main] Module 'numpy' has no 'array' member  [pylint/E1101] [5]
ERROR: PerformanceTests/LaunchTime/new_tab.py:243:  [main] Module 'numpy' has no 'mean' member  [pylint/E1101] [5]
ERROR: PerformanceTests/LaunchTime/new_tab.py:243:  [main] Module 'numpy' has no 'float64' member  [pylint/E1101] [5]
ERROR: PerformanceTests/LaunchTime/new_tab.py:244:  [main] Module 'numpy' has no 'std' member  [pylint/E1101] [5]
ERROR: PerformanceTests/LaunchTime/new_tab.py:244:  [main] Module 'numpy' has no 'float64' member  [pylint/E1101] [5]
Total errors found: 10 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Build Bot 2018-06-12 18:44:20 PDT
Comment on attachment 342479 [details]
Patch

Attachment 342479 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/8154271

New failing tests:
http/tests/security/canvas-remote-read-remote-video-blocked-no-crossorigin.html
Comment 9 Build Bot 2018-06-12 18:44:31 PDT
Created attachment 342617 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 10 Ben Richards 2018-06-22 12:56:54 PDT
Created attachment 343358 [details]
benchmark startup and new tab time
Comment 11 Build Bot 2018-06-22 13:00:02 PDT
Attachment 343358 [details] did not pass style-queue:


ERROR: PerformanceTests/LaunchTime/startup.py:170:  [main] Module 'numpy' has no 'array' member  [pylint/E1101] [5]
ERROR: PerformanceTests/LaunchTime/startup.py:174:  [main] Module 'numpy' has no 'mean' member  [pylint/E1101] [5]
ERROR: PerformanceTests/LaunchTime/startup.py:174:  [main] Module 'numpy' has no 'float64' member  [pylint/E1101] [5]
ERROR: PerformanceTests/LaunchTime/startup.py:175:  [main] Module 'numpy' has no 'std' member  [pylint/E1101] [5]
ERROR: PerformanceTests/LaunchTime/startup.py:175:  [main] Module 'numpy' has no 'float64' member  [pylint/E1101] [5]
ERROR: PerformanceTests/LaunchTime/new_tab.py:176:  [main] Module 'numpy' has no 'array' member  [pylint/E1101] [5]
ERROR: PerformanceTests/LaunchTime/new_tab.py:180:  [main] Module 'numpy' has no 'mean' member  [pylint/E1101] [5]
ERROR: PerformanceTests/LaunchTime/new_tab.py:180:  [main] Module 'numpy' has no 'float64' member  [pylint/E1101] [5]
ERROR: PerformanceTests/LaunchTime/new_tab.py:181:  [main] Module 'numpy' has no 'std' member  [pylint/E1101] [5]
ERROR: PerformanceTests/LaunchTime/new_tab.py:181:  [main] Module 'numpy' has no 'float64' member  [pylint/E1101] [5]
Total errors found: 10 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 Build Bot 2018-06-22 21:58:27 PDT
Comment on attachment 343358 [details]
benchmark startup and new tab time

Attachment 343358 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8300738

New failing tests:
http/tests/security/local-video-source-from-remote.html
Comment 13 Build Bot 2018-06-22 21:58:38 PDT
Created attachment 343420 [details]
Archive of layout-test-results from ews204 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 14 Ben Richards 2018-06-26 16:29:59 PDT
Created attachment 343658 [details]
Patch
Comment 15 Geoffrey Garen 2018-06-27 17:18:22 PDT
Here's what I see when I run this locally:

~/OpenSource/PerformanceTests/LaunchTime> ./startup.py 
Running tests.....done.

RESULTS:
mean: 3.84745621681 sec
std dev: 1.22243723266 sec (31.772609%)
=====
~/OpenSource/PerformanceTests/LaunchTime> ./new_tab.py 
Unable to start test server on localhost:8080. Double check that the port is not in use and try again.
=====
~/OpenSource/PerformanceTests/LaunchTime> ./new_tab.py 
Unable to start test server on localhost:8080. Double check that the port is not in use and try again.
=====
~/OpenSource/PerformanceTests/LaunchTime> ./startup.py 
Unable to start test server on localhost:8080. Double check that the port is not in use and try again.

Have you seen a variance number this high?

Have you seen this server start failure?

Relatedly, it seems like there's a long delay between when I first see Safari try to load and when the load finishes. Is that a problem in the benchmark or a real performance problem in Safari? Usually, I would expect loading from localhost to be immediate.
Comment 16 Ben Richards 2018-06-27 17:56:27 PDT
Wow I'm not sure what went wrong there because I have never seen a std dev close to that high. It's normally ~1%. I just ran it on my machine a few times and it worked each time. The output I got was 

➜  LaunchTime git:(master) ✗ ./startup.py
Running tests.....done.

RESULTS:
mean: 1.05537396669 sec
std dev: 0.0103563437425 sec (0.981296%)

In regards to the error for the server starting, I have seen that before. I believe I'm shutting down the server properly on each test but for some reason it seems like it takes a few seconds for the port to free up (at which point it will start to work again). I could have it try catch 10 times or so (and fail otherwise) with 1 sec delays to avoid that issue if that's what we wanna do
Comment 17 Ben Richards 2018-06-27 17:58:40 PDT
For clarification, the server start error is only thrown when the port is not available for the test server so it occurs if the script is run multiple times one after another with no delay
Comment 18 Ben Richards 2018-06-27 18:00:18 PDT
Std dev for new_tab.py should also be ~1%
Comment 19 Ben Richards 2018-06-29 16:42:44 PDT
Created attachment 343968 [details]
Patch
Comment 20 Ben Richards 2018-06-29 20:14:04 PDT
Created attachment 343992 [details]
Patch
Comment 21 Ben Richards 2018-07-03 17:51:45 PDT
Created attachment 344248 [details]
Patch
Comment 22 Ben Richards 2018-07-11 18:01:38 PDT
Created attachment 344801 [details]
Patch
Comment 23 Build Bot 2018-07-11 18:03:28 PDT
Attachment 344801 [details] did not pass style-queue:


ERROR: PerformanceTests/LaunchTime/feedback_server.py:25:  [MainHandler.get] Instance of 'MainHandler' has no 'write' member  [pylint/E1101] [5]
ERROR: PerformanceTests/LaunchTime/feedback_server.py:92:  [send_all_messages] Instance of 'WSHandler' has no 'write_message' member  [pylint/E1101] [5]
ERROR: PerformanceTests/LaunchTime/feedback_server.py:93:  [send_all_messages] Instance of 'WSHandler' has no 'write_message' member  [pylint/E1101] [5]
ERROR: PerformanceTests/LaunchTime/feedback_server.py:95:  [send_all_messages] Instance of 'WSHandler' has no 'write_message' member  [pylint/E1101] [5]
Total errors found: 4 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 24 Build Bot 2018-07-11 19:37:40 PDT
Comment on attachment 344801 [details]
Patch

Attachment 344801 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8510565

New failing tests:
http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-audio.html
Comment 25 Build Bot 2018-07-11 19:37:52 PDT
Created attachment 344808 [details]
Archive of layout-test-results from ews204 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 26 Ben Richards 2018-07-14 20:53:47 PDT
Created attachment 345051 [details]
Patch
Comment 27 Ben Richards 2018-07-16 14:53:36 PDT
Created attachment 345120 [details]
Patch
Comment 28 Ryosuke Niwa 2018-07-16 17:16:49 PDT
Comment on attachment 345120 [details]
Patch

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

As a high level comment, I think it makes senes to have an abstract class like LaunchTimeBenchmark
and then each test to subclass & override its methods as needed.

> PerformanceTests/ChangeLog:12
> +        (MainHandler):

Add some per-function comments?

> PerformanceTests/LaunchTime/launch_time.py:134
> +    parser.add_argument('-p', '--path', type=parse_browser_bundle_path,
> +                        help='path for browser application bundle (default: "{}")'.format(BROWSER_BUNDLE_PATH))

Nit: We use exactly 4 spaces of indentation.

> PerformanceTests/LaunchTime/launch_time.py:136
> +    parser.add_argument('-n', '--iterations', type=int,
> +                        help='number of iterations for each round to run benchmark (default: "{}")'.format(NUM_ITERATIONS_PER_ROUND))

Since startup test doesn't have any difference between rounds & iterations,
it's probably better to have an option explicitly in the new tab test like --tab-count.

> PerformanceTests/LaunchTime/launch_time.py:159
> +        NUM_ITERATIONS_PER_ROUND = args.iterations
> +    if args.path:
> +        BROWSER_BUNDLE_PATH = args.path
> +    if args.verbose is not None:
> +        VERBOSE = args.verbose
> +    if args.wait_time:
> +        WAIT_TIME_LOW, WAIT_TIME_HIGH = args.wait_time
> +    if args.rounds:
> +        NUM_ROUNDS = args.rounds

It's a bit weird to use ALL CAPS for configuration variables.
I think we should probably use lowercase letter naming for these.

> PerformanceTests/LaunchTime/launch_time.py:229
> +        mean = np.mean(results, dtype=np.float64)
> +        dev = np.std(results, dtype=np.float64)

I think we're better off rolling our own stddev computation rather than requiring bumpy.

> PerformanceTests/LaunchTime/new_tab.py:24
> +                    var time = performance.timing.navigationStart / 1000
> +                    var request = new XMLHttpRequest();

Use const maybe?

> PerformanceTests/LaunchTime/startup.py:20
> +                    var time = +new Date() / 1000

A better function to use here would be Date.now() / 1000.
Also use const or let where appropriate.
Comment 29 Ben Richards 2018-07-17 15:43:26 PDT
Created attachment 345198 [details]
Patch
Comment 30 Ryosuke Niwa 2018-07-17 16:59:00 PDT
Comment on attachment 345198 [details]
Patch

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

> PerformanceTests/LaunchTime/feedback_client.html:20
> +    var host = "ws://localhost:{{ port }}/ws";

Let's use const here.

> PerformanceTests/LaunchTime/feedback_client.html:23
> +    // event handlers for websocket

I don't think this comment is necessary.

> PerformanceTests/LaunchTime/feedback_client.html:37
> +    function showServerResponse(txt) {

Please void abbreviations such as txt. Spell out text.

> PerformanceTests/LaunchTime/feedback_client.html:39
> +      var p = document.createElement('p');

Ditto.

> PerformanceTests/LaunchTime/feedback_client.html:40
> +      p.innerHTML = txt;

Why not textContent?

> PerformanceTests/LaunchTime/feedback_server.py:26
> +        self.client = None
> +        self.feedback_server_thread = None
> +        self.port = 9090
> +        self.application = None
> +        self.server_is_ready = None
> +        self.io_loop = None
> +        self.messages = Queue()
> +        self.client_loaded = threading.Semaphore(0)

We usually prefix "private" variables with _ as in `self._client = None`.

> PerformanceTests/LaunchTime/launch_time.py:22
> +

Why blank line?

> PerformanceTests/LaunchTime/launch_time.py:145
> +        parser.add_argument('-g', '--groups', type=int,
> +            help='number of groups of iterations to run while scanning for optimal standard deviation (default: "{}")'.format(self.iteration_group))

I think we should just put this into more_args. We can keep self.iteration_group as an instance variable here
but just override to a new value in NewTabBenchmark so that this option won't be shown for startup test.

> PerformanceTests/LaunchTime/launch_time.py:146
> +        parser.add_argument('-w', '--wait-time', type=self._parse_wait_time,

It's probably cleaner to specify the defaults here instead of relying on __init__ to do that.

> PerformanceTests/LaunchTime/launch_time.py:178
> +    def _init(self):

It's a bit confusing to have so many init functions.
Why don't call this one _start_server_in_new_thread or maybe _setup_server?

> PerformanceTests/LaunchTime/launch_time.py:179
> +        self._parse_args()

I think we should just call directly in run().

> PerformanceTests/LaunchTime/launch_time.py:227
> +    def get_results(self, results, should_print=False):

We should probably call this compute_results

> PerformanceTests/LaunchTime/launch_time.py:228
> +        def get_mean(results):

We can probably just put this one liner where it's called since it's only called once.

> PerformanceTests/LaunchTime/launch_time.py:236
> +            variance = ssd / num_items

Let's use the Euler's correction and divide this by num_items - 1 instead of num_items.

> PerformanceTests/LaunchTime/launch_time.py:240
> +        if len(results) > 2:
> +            results = results[1:]

We should add a comment here saying that we ignore the very first result.
Maybe we should also consider making this an option??

> PerformanceTests/LaunchTime/launch_time.py:247
> +                self.log('mean: {} ms\n'.format(mean * 1000))

We should probably just print these in run.

> PerformanceTests/LaunchTime/launch_time.py:267
> +        if self.verbose and message != '\n':

It seems like we should just get rid of log_verbose('\n') so that we don't have to skip them here.

> PerformanceTests/LaunchTime/launch_time.py:274
> +        h = float(self.wait_time_high - self.wait_time_low) / (self.iteration_group - 1)

Why don't we give it a more descriptive name like `step` or `increment_per_group`?

> PerformanceTests/LaunchTime/launch_time.py:288
> +            if self.iteration_group == 1:
> +                self.wait_time_low = self.wait_time_high = (self.wait_time_low + self.wait_time_high) / 2

We should probably do this while parsing the argument.

> PerformanceTests/LaunchTime/launch_time.py:302
> +                        result = self.run_iteration()
> +                        self.log_verbose('({}) {} ms\n'.format(i + 1, result * 1000))

Why don't we keep the results in milliseconds so that we don't have to keep dividing & multiplying by 1000.
Also, we should probably call this result_ms or result_in_ms.

> PerformanceTests/LaunchTime/launch_time.py:313
> +                mean, dev = self.get_results(tests, should_print=self.verbose)

Let's call the second variable stdev.

> PerformanceTests/LaunchTime/launch_time.py:315
> +                if dev < min_dev:
> +                    min_dev = dev

I think the final result should probably the geomean of group means
because we expect the mean of a group to change as the wait time increases.

> PerformanceTests/LaunchTime/new_tab.py:35
> +

Nit: Blank line here seems unnecessary.

> PerformanceTests/LaunchTime/new_tab.py:43
> +                            var time = performance.timing.navigationStart / 1000
> +                            var request = new XMLHttpRequest();

Nit: Use const instead of var.

> PerformanceTests/LaunchTime/startup.py:32
> +

Nit: blank line here seems unnecessary.

> PerformanceTests/LaunchTime/startup.py:36
> +                    <meta http-equiv="Content-Type" content="text/html; charset=utf-8" />

We don't really need to specify utf-8 since that's the default encoding anyway.

> PerformanceTests/LaunchTime/startup.py:37
> +                    <script type="text/javascript">

We also don't need to specify type.

> PerformanceTests/LaunchTime/startup.py:39
> +                            var time = +new Date() / 1000

Use const and Date.now().

> PerformanceTests/LaunchTime/startup.py:40
> +                            var request = new XMLHttpRequest();

Use const.

> PerformanceTests/LaunchTime/startup.py:45
> +                        window.onload = sendDone;

We should probably explain in the change log that we're trying to measure the initial page load time as well as the process launch time in this test.

> PerformanceTests/LaunchTime/thirdparty/__init__.py:1
> +# Copyright (C) 2010 Chris Jerdonek (cjerdonek@webkit.org)

Let's add Apple copyright for 2018 as well.
Comment 31 Build Bot 2018-07-17 18:49:20 PDT
Comment on attachment 345198 [details]
Patch

Attachment 345198 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8569252

New failing tests:
http/tests/security/canvas-remote-read-remote-video-localhost.html
http/tests/preload/onload_event.html
Comment 32 Build Bot 2018-07-17 18:49:32 PDT
Created attachment 345215 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 33 Ben Richards 2018-07-18 17:03:33 PDT
Created attachment 345306 [details]
Patch
Comment 34 Ryosuke Niwa 2018-07-18 17:26:43 PDT
Comment on attachment 345306 [details]
Patch

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

> PerformanceTests/LaunchTime/feedback_server.py:19
> +        self.client = None

Please use _ prefix for private instance variables

> PerformanceTests/LaunchTime/feedback_server.py:56
> +                # if message != '\n':

Nit: Remove this.

> PerformanceTests/LaunchTime/feedback_server.py:89
> +                # pylint: disable=E1101

Just get rid of it.

> PerformanceTests/LaunchTime/launch_time.py:113
> +            'browser-bundle-path': dict(enabled=True,

I think we're better off adding this in _parse_args

> PerformanceTests/LaunchTime/launch_time.py:119
> +            'iterations': dict(enabled=True,

Ditto.

> PerformanceTests/LaunchTime/launch_time.py:125
> +            'groups': dict(enabled=True,

This definition should really appear in NewTabBenchmark instead.

> PerformanceTests/LaunchTime/launch_time.py:132
> +        # This convenience method let's subclasses initialize instance variables

Nit: I don't think this comment is needed.

> PerformanceTests/LaunchTime/launch_time.py:141
> +            num = float(string)

Rather than first trying to parse the argument as a float and then splitting with ':',
why don't we always split with ':' and handle the case when len(range) is 1.

> PerformanceTests/LaunchTime/launch_time.py:199
> +        for var_name, value in vars(self._parser.parse_args()).iteritems():
> +            var_name = '_' + var_name
> +            setattr(self, var_name, value)

I think should have a manual mapping as we used to, and add new method which gets called with the parsed argument values
so that it can set instance variables such as self._iteration_groups for --group.

> PerformanceTests/LaunchTime/launch_time.py:247
> +        mean = stdev = None

We should initialize these variables in two separate lines:
https://webkit.org/code-style-guidelines/#linebreaking-multiple-statements

> PerformanceTests/LaunchTime/launch_time.py:363
> +            self.log('Average standard deviation compared to mean:\n-> {}%\n'.format(self._geometric_mean(group_stdevs)))

I think we need to compute the stdev from the geomean for each iteration across groups as we talked about it in person.
Comment 35 Ben Richards 2018-07-18 20:52:06 PDT
Created attachment 345322 [details]
Patch
Comment 36 Build Bot 2018-07-18 20:55:01 PDT
Attachment 345322 [details] did not pass style-queue:


ERROR: PerformanceTests/LaunchTime/feedback_server.py:83:  [FeedbackServer.MainHandler.Handler.get] Instance of 'Handler' has no 'write' member  [pylint/E1101] [5]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 37 Ryosuke Niwa 2018-07-18 21:09:59 PDT
Comment on attachment 345322 [details]
Patch

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

> PerformanceTests/LaunchTime/launch_time.py:275
> +                        results_by_iteration_number[i].append(result_in_ms)

We could also do: results_by_iteration_number.setdefault(i, []) here instead.

> PerformanceTests/LaunchTime/new_tab.py:58
> +        self.argument_parser.add_argument('-w', '--wait-time', type=self._parse_wait_time,
> +            help='wait time to use between iterations or range to scan (format is "N" or "N:M" where N < M, default: {}:{})'.format(self.wait_time_low, self.wait_time_high))

Don't we want to specify the wait time for startup test as well?
Comment 38 Build Bot 2018-07-19 00:09:44 PDT
Comment on attachment 345322 [details]
Patch

Attachment 345322 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8583906

New failing tests:
http/tests/security/canvas-remote-read-remote-video-localhost.html
Comment 39 Build Bot 2018-07-19 00:09:56 PDT
Created attachment 345336 [details]
Archive of layout-test-results from ews205 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 40 Ben Richards 2018-07-19 13:55:57 PDT
Created attachment 345378 [details]
Patch for landing
Comment 41 Ryosuke Niwa 2018-07-19 14:10:01 PDT
Comment on attachment 345378 [details]
Patch for landing

Found a bug.
Comment 42 Ben Richards 2018-07-19 14:13:53 PDT
Created attachment 345381 [details]
Patch for landing
Comment 43 WebKit Commit Bot 2018-07-19 14:54:55 PDT
Comment on attachment 345381 [details]
Patch for landing

Clearing flags on attachment: 345381

Committed r234006: <https://trac.webkit.org/changeset/234006>
Comment 44 WebKit Commit Bot 2018-07-19 14:54:57 PDT
All reviewed patches have been landed.  Closing bug.
Comment 45 Radar WebKit Bug Importer 2018-07-19 15:02:00 PDT
<rdar://problem/42401939>