Bug 192030 - Layout test should generate performance metrics
Summary: Layout test should generate performance metrics
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-11-27 13:24 PST by Zhifei Fang
Modified: 2019-01-08 13:15 PST (History)
11 users (show)

See Also:


Attachments
Patch (7.26 KB, patch)
2018-11-27 14:16 PST, Zhifei Fang
no flags Details | Formatted Diff | Diff
Patch (10.18 KB, patch)
2018-11-27 14:32 PST, Zhifei Fang
no flags Details | Formatted Diff | Diff
Patch (12.10 KB, patch)
2018-11-27 15:25 PST, Zhifei Fang
no flags Details | Formatted Diff | Diff
Patch (9.15 KB, patch)
2018-11-27 15:31 PST, Zhifei Fang
no flags Details | Formatted Diff | Diff
Patch (7.58 KB, patch)
2018-11-28 10:43 PST, Zhifei Fang
no flags Details | Formatted Diff | Diff
Patch (7.58 KB, patch)
2018-11-28 10:58 PST, Zhifei Fang
no flags Details | Formatted Diff | Diff
Patch (7.91 KB, patch)
2018-11-29 18:08 PST, Zhifei Fang
no flags Details | Formatted Diff | Diff
Patch (7.95 KB, patch)
2018-11-30 10:26 PST, Zhifei Fang
no flags Details | Formatted Diff | Diff
Patch (10.16 KB, patch)
2018-12-03 18:28 PST, Zhifei Fang
no flags Details | Formatted Diff | Diff
Patch (10.12 KB, patch)
2018-12-04 10:01 PST, Zhifei Fang
no flags Details | Formatted Diff | Diff
Patch (10.52 KB, patch)
2019-01-07 16:10 PST, Zhifei Fang
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews124 for ios-simulator-wk2 (24.32 MB, application/zip)
2019-01-07 18:56 PST, Build Bot
no flags Details
Patch (10.46 KB, patch)
2019-01-08 10:49 PST, Zhifei Fang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zhifei Fang 2018-11-27 13:24:55 PST
<rdar://problem/32779561>
Comment 1 Zhifei Fang 2018-11-27 13:27:13 PST
Wrong number

<rdar://problem/32779516>
Comment 2 Zhifei Fang 2018-11-27 14:16:54 PST
Created attachment 355780 [details]
Patch
Comment 3 Zhifei Fang 2018-11-27 14:32:17 PST
Created attachment 355784 [details]
Patch
Comment 4 Jonathan Bedard 2018-11-27 15:06:22 PST
Comment on attachment 355784 [details]
Patch

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

> Tools/ChangeLog:1
> +2018-11-27  Zhifei FANG  <zhifei_fang@apple.com>

Duplicate changelogs here.  Also, should your last name be capitalized like that? Seems like a bug, perhaps a tooling one.

> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator_unittest.py:273
> +        }

Maybe collapse these?
Comment 5 Zhifei Fang 2018-11-27 15:25:41 PST
Created attachment 355795 [details]
Patch
Comment 6 Zhifei Fang 2018-11-27 15:31:26 PST
Created attachment 355797 [details]
Patch
Comment 7 Jonathan Bedard 2018-11-27 16:34:26 PST
Discussed this with Zhifei and Ryan.

We need to be tracking total test-time here, not individual test time. Tracking individual test time will be a nightmare for the service consuming these results.
Comment 8 Zhifei Fang 2018-11-28 10:43:24 PST
Created attachment 355890 [details]
Patch
Comment 9 Zhifei Fang 2018-11-28 10:58:54 PST
Created attachment 355892 [details]
Patch
Comment 10 Jonathan Bedard 2018-11-28 11:55:17 PST
Comment on attachment 355892 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:130
> +        total_run_time += int(1000 * test_result.test_run_time)

...This will be an aggregate of of all test times, not the time it took to run the tests. Not sure which metric we want to track, I can see merit in both.

As an example, suppose we ran 100 tests in 10 processes with each test taking 30 seconds. This metric would report a total test run time of 50 minutes, when in reality, it actually took 5 minutes for the tests to run.
Comment 11 Zhifei Fang 2018-11-29 18:08:06 PST
Created attachment 356101 [details]
Patch
Comment 12 Jonathan Bedard 2018-11-30 08:36:13 PST
Comment on attachment 356101 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:412
> +            initial_results.results_by_name.values())

This seems like it would be fine on 1 line.

Side note, the one part of PEP-8 we ignore is the line size limit

> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:414
> +            self._results_directory, "layout_test_perf_metrics.json")

Why a new-line here?

> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:125
> +    Will need to generate the total runtime of layout tests as a perfromance metric

This seems like a description of what needs to be done as opposed to what has been done.
Comment 13 Zhifei Fang 2018-11-30 10:26:18 PST
Created attachment 356194 [details]
Patch
Comment 14 Jonathan Bedard 2018-11-30 10:48:07 PST
Unofficial r=me.
Comment 15 Aakash Jain 2018-11-30 11:09:33 PST
Comment on attachment 356194 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:411
> +        perf_metrics_path = self._filesystem.join(self._results_directory, "layout_test_perf_metrics.json")

It would be a good idea to make the json file name a variable and define it in the beginning of the file. e.g.: LAYOUT_TEST_PERF_METRIC_FILENAME

That way if we want to use this file name in other portion of code, we can simply use the variable instead of hard-coding it at multiple places.

> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:123
> +def test_perf_metrics(run_time, individual_test_timings):

run_time seems confusing, it can be interpreted as a runtime (environment).

other alternatives might be:  total_execution_time, total_time

> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:127
> +    2. run time, which is how much time consumed by the layout tests script

This naming is confusing. Total time is not really the total time. Run time is actually the total time.

We should rename it to something like:

1) Test time  / Total test time
2) Total time /  Total run/execution time

> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:137
> +                "Time": {"current": [total_run_time]},

curious, why "current"?

> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:140
> +        "layout_tests_run_time": {

Ditto.
Comment 16 Zhifei Fang 2018-12-03 18:28:46 PST
Created attachment 356444 [details]
Patch
Comment 17 Zhifei Fang 2018-12-03 18:30:29 PST
As discussed change to run time of second level categories and total run time.
Comment 18 Dean Jackson 2018-12-04 09:51:32 PST
Ooop. I marked this as r+ accidentally. Sorry.
Comment 19 Zhifei Fang 2018-12-04 10:01:57 PST
Created attachment 356513 [details]
Patch
Comment 20 Zhifei Fang 2019-01-07 11:44:31 PST
hi, could you guys approve this ?
Comment 21 Jonathan Bedard 2019-01-07 13:07:06 PST
Comment on attachment 356513 [details]
Patch

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

> Tools/ChangeLog:8
> +

Can you add a high-level comment about what's being done here? As a side note, add_test_perf_metric and test_perf_metrics probably deserve some commend as well.

> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:122
> +def add_test_perf_metric(path, time, tests, depth, target_depth):

Can we append this function with an underscore since it's defiantly a private function?
Comment 22 Zhifei Fang 2019-01-07 16:10:44 PST
Created attachment 358550 [details]
Patch
Comment 23 Build Bot 2019-01-07 18:56:27 PST
Comment on attachment 358550 [details]
Patch

Attachment 358550 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10665230

New failing tests:
media/video-zoom.html
Comment 24 Build Bot 2019-01-07 18:56:33 PST
Created attachment 358564 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6
Comment 25 Aakash Jain 2019-01-08 09:46:46 PST
Comment on attachment 358550 [details]
Patch

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

r=me assuming Dean Johnson's in-person feedback has been incorporated.

> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:122
> +def _add_test_perf_metric(path, time, tests, depth, target_depth):

Ditto. _add_perf_metric_for_test() might read better.

> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:159
> +def test_perf_metrics(run_time, individual_test_timings):

'test_perf_metrics' name feels like this is a test for perf_metrics. perf_metrics_for_test might read better.
Comment 26 Dean Johnson 2019-01-08 09:58:34 PST
Comment on attachment 358550 [details]
Patch

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

Looks good overall, just a few nits on style / naming.

> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:122
> +def _add_test_perf_metric(path, time, tests, depth, target_depth):

Talked with Jonathan about target_depth as I was a bit confused by it. Could you rename it to depth_limit, which implies it is okay to exit before hitting target_depth?

> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:124
> +    Breaks a test name into chunks by directory, and keep track the depth.

the depth -> of the depth.

Both lines may be able to be simplified to:
"Aggregate test time to result for a given test at a specified target_depth."

> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:144
> +                    }}}

Is there a reason you expanded this block, but not the one below? We should try to be consistent in style here.

> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:183
> +        # for now, we only sent two level of categories

sent -> send
level -> levels
Comment 27 Zhifei Fang 2019-01-08 10:49:45 PST
Created attachment 358613 [details]
Patch
Comment 28 WebKit Commit Bot 2019-01-08 13:15:12 PST
Comment on attachment 358613 [details]
Patch

Clearing flags on attachment: 358613

Committed r239739: <https://trac.webkit.org/changeset/239739>
Comment 29 WebKit Commit Bot 2019-01-08 13:15:14 PST
All reviewed patches have been landed.  Closing bug.