Skip to content
This repository was archived by the owner on Nov 20, 2024. It is now read-only.

Add --benchmark support. #672

Merged
merged 3 commits into from
May 21, 2017
Merged

Add --benchmark support. #672

merged 3 commits into from
May 21, 2017

Conversation

pq
Copy link
Contributor

@pq pq commented May 19, 2017

New benchmark mode that prints lint stats based on the best of 10 runs.

Follow-up from: dart-lang/sdk#57574

@devoncarew @bwilkerson

@pq
Copy link
Contributor Author

pq commented May 19, 2017

FYI @alexeieleusis : even with the best of, it looks like invariant_booleans is running a bit slow... ?

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 95.852% when pulling 5d59e7c on benchmarking into f507e4f on master.

@alexeieleusis
Copy link
Contributor

What time is it taking?

@pq
Copy link
Contributor Author

pq commented May 19, 2017

144 ms (from here)

@alexeieleusis
Copy link
Contributor

I'll look into it once this is merged, at least we have a baseline now.

bin/linter.dart Outdated
@@ -54,6 +55,8 @@ void runLinter(List<String> args, LinterOptions initialLintOptions) {
abbr: "h", negatable: false, help: "Show usage information.")
..addFlag("stats",
abbr: "s", negatable: false, help: "Show lint statistics.")
..addFlag("benchmark",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

may not need a shorthand for this flag, given how rarely it'll be used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good.

bin/linter.dart Outdated
@@ -141,21 +144,27 @@ void runLinter(List<String> args, LinterOptions initialLintOptions) {
}

var stats = options['stats'];
if (stats == true) {
var benchmark = options['benchmark'];
if (stats == true || benchmark == true) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps (stats || benchmark)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! For some reason I thought I convinced myself that undefined would return null but yeah, much better. 👍

@@ -291,6 +266,57 @@ class SimpleFormatter implements ReportFormatter {
}
}

writeBenchmarks(IOSink out, List<File> filesToLint, LinterOptions lintOptions) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so few return types :(

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. Consider this bit of code my last stand against gratuitous returns (in non API bits). 😸

tool/travis.sh Outdated
# Run linter with all lints enabled.
dart bin/linter.dart -s -q -c example/all.yaml .
# Benchmark linter with all lints enabled.
dart bin/linter.dart -b -q -c example/all.yaml .
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--benchmark if you remove the shorthand

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 95.851% when pulling 93ed492 on benchmarking into f507e4f on master.

@pq
Copy link
Contributor Author

pq commented May 20, 2017

@devoncarew : take another gander?

@devoncarew
Copy link
Contributor

lgtm!

@pq
Copy link
Contributor Author

pq commented May 21, 2017

Thanks!

@pq pq merged commit fb322e7 into master May 21, 2017
@pq pq deleted the benchmarking branch June 28, 2017 16:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

5 participants