Bug 178059 - Improve --help documentation and add --list-plans to show available benchmarks.
Summary: Improve --help documentation and add --list-plans to show available benchmarks.
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: Maciej Stachowiak
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-10-07 20:10 PDT by Maciej Stachowiak
Modified: 2017-10-16 01:17 PDT (History)
5 users (show)

See Also:


Attachments
New output for --help and --list-plans (2.52 KB, text/plain)
2017-10-07 20:51 PDT, Maciej Stachowiak
no flags Details
Patch (8.48 KB, patch)
2017-10-07 20:51 PDT, Maciej Stachowiak
rniwa: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Maciej Stachowiak 2017-10-07 20:10:20 PDT
Improve --help documentation and add --list-plans to show available benchmarks.
Comment 1 Maciej Stachowiak 2017-10-07 20:51:18 PDT
Created attachment 323121 [details]
New output for --help and --list-plans

For ease of review, attachment showing what the new --help and --list-plams output looks like.
Comment 2 Maciej Stachowiak 2017-10-07 20:51:44 PDT
Created attachment 323122 [details]
Patch
Comment 3 Ryosuke Niwa 2017-10-07 22:25:57 PDT
Comment on attachment 323122 [details]
Patch

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

> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:64
> +    @staticmethod
> +    def available_plans():

I don't think this function belongs to benchmark_runner class. This class shouldn't know anything about where plan files are located.
We should keep it in run_benchmark.py.

> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:66
> +        plans = [plan_file.replace(".plan", "") for plan_file in os.listdir(plan_directory) if plan_file.endswith(".plan")]

We should continue to use os.path.splitext(plan_file)[0]
This code replaces any string of ".plan" in its name like "x*.plan*ebench.plan".

> Tools/Scripts/webkitpy/benchmark_runner/run_benchmark.py:88
> -            raise Exception('Cant find any .plan file in directory %s' % plandir)
> +            raise Exception('Cant find any .plan file in directory %s' % BenchmarkRunner.available_plans())

I don't think this change makes sense. This error messages is about not being able to find any plans.
We should show the directory name instead.
Comment 4 Ryosuke Niwa 2017-10-07 22:26:33 PDT
Dewei, please take a second look at Maciej's patch to make sure landing this patch won't break bots.
Comment 5 Maciej Stachowiak 2017-10-07 22:55:33 PDT
(In reply to Ryosuke Niwa from comment #3)
> Comment on attachment 323122 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=323122&action=review
> 
> > Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:64
> > +    @staticmethod
> > +    def available_plans():
> 
> I don't think this function belongs to benchmark_runner class. This class
> shouldn't know anything about where plan files are located.
> We should keep it in run_benchmark.py.

I added it here because benchmark_runner already knows where plan files are located via  _find_plan_file(). I agree the knowledge should only be in one place, it seemed easier to put it in BenchmarkRunner than in run_benchmark.py. If you think it's a better direction for run_benchmark.py be the only place that knows about plan files instead, I can change things towards that direction instead. I put the code in benchmark_runner before I noticed that run_benchmark was already enumerating all plans.

> 
> > Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:66
> > +        plans = [plan_file.replace(".plan", "") for plan_file in os.listdir(plan_directory) if plan_file.endswith(".plan")]
> 
> We should continue to use os.path.splitext(plan_file)[0]
> This code replaces any string of ".plan" in its name like
> "x*.plan*ebench.plan".

Will do.

> 
> > Tools/Scripts/webkitpy/benchmark_runner/run_benchmark.py:88
> > -            raise Exception('Cant find any .plan file in directory %s' % plandir)
> > +            raise Exception('Cant find any .plan file in directory %s' % BenchmarkRunner.available_plans())
> 
> I don't think this change makes sense. This error messages is about not
> being able to find any plans.
> We should show the directory name instead.

You're right, that makes no sense. Will revert.
Comment 6 Ryosuke Niwa 2017-10-08 02:30:27 PDT
(In reply to Maciej Stachowiak from comment #5)
> (In reply to Ryosuke Niwa from comment #3)
> > Comment on attachment 323122 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=323122&action=review
> > 
> > > Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:64
> > > +    @staticmethod
> > > +    def available_plans():
> > 
> > I don't think this function belongs to benchmark_runner class. This class
> > shouldn't know anything about where plan files are located.
> > We should keep it in run_benchmark.py.
> 
> I added it here because benchmark_runner already knows where plan files are
> located via  _find_plan_file(). I agree the knowledge should only be in one
> place, it seemed easier to put it in BenchmarkRunner than in
> run_benchmark.py. If you think it's a better direction for run_benchmark.py
> be the only place that knows about plan files instead, I can change things
> towards that direction instead. I put the code in benchmark_runner before I
> noticed that run_benchmark was already enumerating all plans.

Oh, okay. I guess keeping it in benchmark_runner would be okay then.
Comment 7 Maciej Stachowiak 2017-10-16 01:16:32 PDT
Committed r223341: <https://trac.webkit.org/changeset/223341>
Comment 8 Radar WebKit Bug Importer 2017-10-16 01:17:37 PDT
<rdar://problem/35003097>