Bug 186414 – Add benchmark for WebKit process launch times

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ben Richards]

...

[Alexey Proskuryakov]

Seeing Safari tests in WebKit would be quite surprising. What is the benefit of having these in WebKit?

[Ben Richards]

Created attachment 342371 [details]
Patch

[EWS Watchlist]

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.

[Geoffrey Garen]

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

[Geoffrey Garen]

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?

[Ben Richards]

Created attachment 342479 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Ben Richards]

Created attachment 343358 [details]
benchmark startup and new tab time

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Ben Richards]

Created attachment 343658 [details]
Patch

[Geoffrey Garen]

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.

[Ben Richards]

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

[Ben Richards]

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

[Ben Richards]

Std dev for new_tab.py should also be ~1%

[Ben Richards]

Created attachment 343968 [details]
Patch

[Ben Richards]

Created attachment 343992 [details]
Patch

[Ben Richards]

Created attachment 344248 [details]
Patch

[Ben Richards]

Created attachment 344801 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Ben Richards]

Created attachment 345051 [details]
Patch

[Ben Richards]

Created attachment 345120 [details]
Patch

[Ryosuke Niwa]

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.

[Ben Richards]

Created attachment 345198 [details]
Patch

[Ryosuke Niwa]

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 ([e-mail])

Let's add Apple copyright for 2018 as well.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Ben Richards]

Created attachment 345306 [details]
Patch

[Ryosuke Niwa]

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.

[Ben Richards]

Created attachment 345322 [details]
Patch

[EWS Watchlist]

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.

[Ryosuke Niwa]

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?

[EWS Watchlist]

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

[EWS Watchlist]

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

[Ben Richards]

Created attachment 345378 [details]
Patch for landing

[Ryosuke Niwa]

Comment on attachment 345378 [details]
Patch for landing

Found a bug.

[Ben Richards]

Created attachment 345381 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 345381 [details]
Patch for landing

Clearing flags on attachment: 345381

Committed r234006: <https://trac.webkit.org/changeset/234006>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/42401939>