Bug 192030 – Layout test should generate performance metrics

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Zhifei Fang]

<rdar://problem/32779561>

[Zhifei Fang]

Wrong number

<rdar://problem/32779516>

[Zhifei Fang]

Created attachment 355780 [details]
Patch

[Zhifei Fang]

Created attachment 355784 [details]
Patch

[Jonathan Bedard]

Comment on attachment 355784 [details]
Patch

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

> Tools/ChangeLog:1
> [phone]  Zhifei FANG  <[e-mail]>

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?

[Zhifei Fang]

Created attachment 355795 [details]
Patch

[Zhifei Fang]

Created attachment 355797 [details]
Patch

[Jonathan Bedard]

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.

[Zhifei Fang]

Created attachment 355890 [details]
Patch

[Zhifei Fang]

Created attachment 355892 [details]
Patch

[Jonathan Bedard]

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.

[Zhifei Fang]

Created attachment 356101 [details]
Patch

[Jonathan Bedard]

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.

[Zhifei Fang]

Created attachment 356194 [details]
Patch

[Jonathan Bedard]

Unofficial r=me.

[Aakash Jain]

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.

[Zhifei Fang]

Created attachment 356444 [details]
Patch

[Zhifei Fang]

As discussed change to run time of second level categories and total run time.

[Dean Jackson]

Ooop. I marked this as r+ accidentally. Sorry.

[Zhifei Fang]

Created attachment 356513 [details]
Patch

[Zhifei Fang]

hi, could you guys approve this ?

[Jonathan Bedard]

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?

[Zhifei Fang]

Created attachment 358550 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Aakash Jain]

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.

[Dean Johnson]

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

[Zhifei Fang]

Created attachment 358613 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 358613 [details]
Patch

Clearing flags on attachment: 358613

Committed r239739: <https://trac.webkit.org/changeset/239739>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.