Fix SR-4598 Add option to run subset of benchmarks matching a prefix #8975
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Looking at this one now. |
|
@palimondo at some point, we should really add unit tests for the driver. But we can do that later. It would give more confidence around these sorts of changes. |
gottesmm
requested changes
Apr 24, 2017
benchmark/scripts/Benchmark_Driver
Outdated
| parent_parser = argparse.ArgumentParser(add_help=False) | ||
| benchmarks_group = parent_parser.add_mutually_exclusive_group() | ||
| benchmarks_group.add_argument( | ||
| 'benchmark', |
There was a problem hiding this comment.
Since this is a list, can you change the destination to benchmarks? IIRC the argument is "dest".
So the user will still:
--benchmark foo --benchmark bar
But programmatically we will see:
args.benchmarks
There was a problem hiding this comment.
Benchmarks are positional arguments - just a list of names at the end of the line:
mondo$ $BUILD/bin/Benchmark_Driver run -o O MonteCarloE AngryPhonebook
1,AngryPhonebook,1,13177,13177,13177,0,13177,11046912
124,MonteCarloE,1,50568,50568,50568,0,50568,18423808
Totals,2,63745,63745,63745,0,63745,29470720
There was a problem hiding this comment.
Ah, I misunderstood. You just wanted to rename the internal argument name. I was talking about the fact that this argument has no user facing part.
benchmark/scripts/Benchmark_Driver
Outdated
| return filter(lambda name: name.startswith(args.filter), tests) | ||
| if not args.benchmark: | ||
| return tests | ||
| return sorted(list(set(tests).intersection(set(args.benchmark)))) |
There was a problem hiding this comment.
This will silently fail if someone passes in a non-matching benchmark. We should either emit a warning or fail in such a case.
There was a problem hiding this comment.
It doesn't fail silently. It executes no tests:
$ $BUILD/bin/Benchmark_Driver run -o O --iterations 2 Malarky
# TEST SAMPLES MIN(μs) MAX(μs) MEAN(μs) SD(μs) MEDIAN(μs) MAX_RSS(B)
$ $BUILD/bin/Benchmark_Driver run -o O --iterations 2 MonteCarloE Malarky
# TEST SAMPLES MIN(μs) MAX(μs) MEAN(μs) SD(μs) MEDIAN(μs) MAX_RSS(B)
124 MonteCarloE 2 36580 37602 37091 722 37602 24047616
Totals 1 36580 37602 37091 722 37602 24047616
Logging results to: /Users/mondo/Desktop/logs-test/refactor-compare-perf/Benchmark_O-20170425012616-f3b33f06d4.log
| help='optimization levels to use (default: O Onone Ounchecked)', | ||
| default=['O', 'Onone', 'Ounchecked']) | ||
| submit_parser.add_argument( | ||
| '-i', '--iterations', |
There was a problem hiding this comment.
I am going to ignore this for now, but we should really change this argument to --samples. In terms of the lower level benchmark driver, iterations is proportional to the length of a sample. This is really changing the number of individual samples that we are running.
|
I think I answered your remarks. Is there any other change you want me to make? |
|
Here's the current usage help, to illustrate how the |
Fixed pyton-lint warnings
|
Lets get this in now. In a separate PR can you add a hard error if a selected benchmark is specified but is not available? The reason that I called such a thing a silent failure is because the user is specifically stating that they want to see a specific benchmark run. If the benchmark doesn't run, from an API perspective, we should fail early and quickly. That being said, I am fine with doing this in a later commit. |
gottesmm
approved these changes
Apr 25, 2017
|
@swift-ci smoke benchmark |
Build comment file:Optimized (O) Regression (0)Improvement (0)No Changes (268)
Regression (1)
Improvement (1)
No Changes (266)
|
|
@swift-ci smoke test and merge |
1 similar comment
|
@swift-ci smoke test and merge |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves SR-4598.
Broken out of #8793.
@gottesmm Please review