Bug 135409 - [iOS] run-webkit-tests runs webkit-build-directory on every test
Summary: [iOS] run-webkit-tests runs webkit-build-directory on every test
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: iPhone / iPad All
: P2 Normal
Assignee: David Farler
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-07-29 21:54 PDT by David Farler
Modified: 2015-02-28 17:24 PST (History)
7 users (show)

See Also:


Attachments
Patch (9.15 KB, patch)
2014-08-03 23:16 PDT, David Farler
no flags Details | Formatted Diff | Diff
Patch (2.69 KB, patch)
2014-08-05 15:40 PDT, David Farler
dbates: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Farler 2014-07-29 21:54:52 PDT
At least on the iOS simulator, run-webkit-tests calls webkit-build-directory a lot - at least once per tests with run times between 1/4 and over 2 seconds! I’ll investigate to see if this is affecting other ports and cache the result accordingly. It doesn’t need to be calling out to a Perl script for what is basically a constant.
Comment 1 Csaba Osztrogonác 2014-07-31 02:44:31 PDT
I checked, this script is called only twice on EFL, so it is an iOS
specific bug.

Maybe mac_config.build_directory call in relay_path causes this problem:
http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/port/ios.py?rev=171789#L76
Comment 2 David Farler 2014-07-31 10:01:18 PDT
relay_path is only called a handful of times. The culprit is in webkitpy.port.config.Config.build_directory, so unless a port overrides this somehow, it would be affecting everyone. You can check to see if it is eating up time by enabling debug logging in run-webkit-tests with --debug-rwt-logging.
Comment 3 David Farler 2014-08-03 18:13:08 PDT
I stand corrected - _path_to_image_diff is called for each ref test and I’m constructing a mac Config object in there. Silly. I’ll save the path during construction of the port.
Comment 4 David Farler 2014-08-03 23:16:51 PDT
Created attachment 235966 [details]
Patch
Comment 5 David Farler 2014-08-05 15:39:05 PDT
Comment on attachment 235966 [details]
Patch

Wrong patch, apologies.
Comment 6 David Farler 2014-08-05 15:40:04 PDT
Created attachment 236056 [details]
Patch
Comment 7 Daniel Bates 2014-08-13 14:47:40 PDT
Comment on attachment 236056 [details]
Patch

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

> Tools/ChangeLog:10
> +        (IOSSimulatorPort.__init__):
> +        Cache Mac build directory once.

Nit: It seems sufficient to collapse these two line into a single line such that it reads:

(IOSSimulatorPort.__init__): Cache Mac build directory.

(Notice that I removed the word "once" since this is implied by the use of the word "cache").

> Tools/ChangeLog:12
> +        (IOSSimulatorPort.relay_path):
> +        Use cached build directory.

Nit: Similarly I would write the contents of these two lines in a single line.

> Tools/ChangeLog:14
> +        (IOSSimulatorPort._path_to_image_diff):
> +        Use cached build directory.

Ditto.

> Tools/Scripts/webkitpy/port/ios.py:67
> +        self._mac_config = port_config.Config(self._executive, self._filesystem, 'mac')

This instance variable is referenced exactly once (on the line below). Unless you plan to make use of it in subsequent patches in the near future, I suggest we either inline its value into the line below or make it a local variable.
Comment 8 David Farler 2014-08-14 13:58:32 PDT
Committed r172602: <http://trac.webkit.org/changeset/172602>
Comment 9 David Kilzer (:ddkilzer) 2015-02-28 15:07:37 PST
Re-fixed in commit r180845:  <http://trac.webkit.org/changeset/180845>
Comment 10 David Kilzer (:ddkilzer) 2015-02-28 17:24:25 PST
(In reply to comment #9)
> Re-fixed in commit r180845:  <http://trac.webkit.org/changeset/180845>

This regressed with the fix for Bug 140949:

[iOS] run-webkit-tests --platform=ios* --lint-test-files does not work
https://bugs.webkit.org/show_bug.cgi?id=140949
<http://trac.webkit.org/changeset/179216>