Bug 189135 - BenchmarkResults.format should support specifying depth of tests to show.
Summary: BenchmarkResults.format should support specifying depth of tests to show.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 189367
Blocks:
  Show dependency treegraph
 
Reported: 2018-08-29 17:49 PDT by dewei_zhu
Modified: 2018-09-06 15:33 PDT (History)
4 users (show)

See Also:


Attachments
Patch (4.62 KB, patch)
2018-09-05 20:43 PDT, dewei_zhu
no flags Details | Formatted Diff | Diff
Patch (3.99 KB, patch)
2018-09-06 14:59 PDT, dewei_zhu
rniwa: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description dewei_zhu 2018-08-29 17:49:32 PDT
BenchmarkResults.format should support specifying depth of tests to show.
Comment 1 dewei_zhu 2018-09-05 20:43:31 PDT
Created attachment 348999 [details]
Patch
Comment 2 Ryosuke Niwa 2018-09-06 00:31:34 PDT
Comment on attachment 348999 [details]
Patch

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

r=me with the suggested change.

> Tools/Scripts/webkitpy/benchmark_runner/benchmark_results.py:78
> -                output += cls._format_tests(test['tests'], scale_unit, show_iteration_values, indent=(indent + ' ' * len(test_name)))
> +                output += cls._format_tests(test['tests'], scale_unit, show_iteration_values, None if max_depth is None else max_depth - 1, indent=(indent + ' ' * len(test_name)))

This is such a convoluted way of doing this.
Please just check: "if 'tests' in test and max_depth > 1" above.
Comment 3 dewei_zhu 2018-09-06 13:44:57 PDT
Comment on attachment 348999 [details]
Patch

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

>> Tools/Scripts/webkitpy/benchmark_runner/benchmark_results.py:78
>> +                output += cls._format_tests(test['tests'], scale_unit, show_iteration_values, None if max_depth is None else max_depth - 1, indent=(indent + ' ' * len(test_name)))
> 
> This is such a convoluted way of doing this.
> Please just check: "if 'tests' in test and max_depth > 1" above.

I don't think this will handle the case when max_depth is None. It would have been much easier if Python have something similar to Infinity in JavaScript.
Also, due to the check on line 56, it ensures max_depth can either be None or integer larger than 0.
I can try to simplify it by using "max_depth - 1 if max_depth else None"
Comment 4 Ryosuke Niwa 2018-09-06 14:13:45 PDT
Comment on attachment 348999 [details]
Patch

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

> Tools/Scripts/webkitpy/benchmark_runner/benchmark_results.py:52
> +    def format(self, scale_unit=True, show_iteration_values=False, max_depth=None):
> +        return self._format_tests(self._results, scale_unit, show_iteration_values, max_depth)

Just use max_depth=sys.maxsize here.
Comment 5 dewei_zhu 2018-09-06 14:59:32 PDT
Created attachment 349078 [details]
Patch
Comment 6 dewei_zhu 2018-09-06 15:33:05 PDT
Re-landed the change in r235762.