BenchmarkDriver Strangler replaces Benchmark_Driver run #18924
Merged
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
Improving complience with PEP 257 -- Docstring Conventions https://www.python.org/dev/peps/pep-0257/
The test number column in the space justified column format emmited by the Benchmark_Driver to stdout while logging to file is right aligned, so it must handle leading whitespace.
Moved the `log_file` path construction to the `BenchmarkDriver`. Retired `get_*_git_*` functions.
Added tests for `log_results` and the *space-justified-columns* format emited to stdout while logging to file.
Moved `log_results` to BenchmarkDriver.
Clean up after removing bogus agregate statistics from last line of the log. It makes more sense to report the total number of executed benchmarks as a sentence that trying to fit into the format of preceding table. Added test assertion that `run_benchmarks` return csv formatted log, as it is used to write the log into file in `log_results`.
Moved all `run` command related functionality to `BenchmarkDriver`.
|
@eeckstein Please review 🙏 |
|
@swift-ci please benchmark |
|
@swift-ci please smoke test |
eeckstein
approved these changes
Aug 24, 2018
| @@ -308,7 +308,7 @@ def _reset(self): | |||
|
|
|||
| # Parse lines like this | |||
| # #,TEST,SAMPLES,MIN(μs),MAX(μs),MEAN(μs),SD(μs),MEDIAN(μs) | |||
| results_re = re.compile(r'(\d+[, \t]*\w+[, \t]*' + | |||
| results_re = re.compile(r'([ ]*\d+[, \t]*\w+[, \t]*' + | |||
There was a problem hiding this comment.
nitpick: brackets are not required around the single space
There was a problem hiding this comment.
I did not know that space and a star would work. This way it is in same style as all the other repeated groups. Do you want me to change it?
| @@ -55,6 +55,9 @@ class BenchmarkDriver(object): | |||
| self.tests = tests or self._get_tests() | |||
| self.parser = parser or LogParser() | |||
| self.results = {} | |||
| # Set a constant hash seed. Some tests are currently sensitive to | |||
| # fluctuations in the number of hash collisions. | |||
| os.environ['SWIFT_DETERMINISTIC_HASHING'] = '1' | |||
There was a problem hiding this comment.
That's an important change. It should be in a separate commit or at least be mentioned in the commit message
There was a problem hiding this comment.
I’m not sure what you mean. Deterministic hashing env variable was added some 5 months ago by @lorentey. I’ve just added a unit test for it and moved the implementation into BenchmarkDriver class.
Build comment file:Optimized (O)Regression (1)
Improvement (1)
No Changes (445)
Unoptimized (Onone)Regression (5)
Improvement (6)
No Changes (436)
Hardware Overview |
|
@eeckstein Benchmarks finished successfully. Can you merge this as is or should I change something? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This is a followup to PR #18719 that concludes the StranglerApplication of the
Benchmark_Driver'sruncommand. The remaining free-standing functions implementing theruncommand were replaced with methods onBenchmarkDriverclass with full unit test coverage.The functionality remains unchanged with the exception of cleaning up the last "Totals" line of the log reports, after removing the bogus aggregate statistics. It now simply states the number of executed benchmarks:
I've structured the PR as a series of small, incremental changes with detailed descriptions in the commit messages, it is therefore best reviewed sequentially by individual commits.