[benchmark] CheckResults with auto-generated error message #9330
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
In order to minimize impact of results checking on test performance, this removes the @autoclosure for error message. Added new version of `CheckResults` that takes only `resultsMatch: Bool` - rest of the parameters are defaulted to `StaticString`s for method and file name, plus line number. Old method was deprecated, but left in place as tool for debugging failing checks. All tests were move to use the new method.
palimondo
changed the title
CheckResults with auto-generated error message
|
@swift-ci Please smoke benchmark |
@palimondo: Please don't assume anything; instead run the benchmarks locally with and without your calls to checkResults and ensure that they are not having any significant impact on timing. Note also that this needs to be the case for -Onone as well as -O. I don't personally care about -Ounchecked performance, though others might, but if the -O results are acceptable -Ounchecked is very unlikely to be different. |
|
@dabrahams I’m assuming in a sense that that’s what makes theoretical sense to me without being familiar with the details of the StaticString implementation. I was hoping someone with knowledge about that will chime in… |
Build comment file:Optimized (O) Regression (19)
Improvement (37)
No Changes (213)
Regression (2)
Improvement (2)
No Changes (265)
|
|
@dabrahams On a quick glance these benchmark results look identical to those you got on #9298 when considering the But I don't seem to get as many improvements in -10% to -5% Delta, but when I look at the reported times, they look to be the same. |
|
I think I'll pack it for today. Its 11:30PM here. Good night! |
|
@swift-ci Please benchmark |
Build comment file:Optimized (O) Regression (4)
Improvement (9)
No Changes (256)
Improvement (4)
No Changes (265)
|
|
Clearly |
|
Yeah, I think we can ignore this for now. Maybe we can make this benchmark more reliable by making the test strings larger. |
|
@eeckstein Because I'm not sure what you mean by “ignore this” I'm going to cop out and leave it up to you whether to merge this PR or not 😉 |
|
@eeckstein Please merge? |
xwu
reviewed
May 10, 2017
| } | ||
| @inline(__always) | ||
| public func CheckResults( | ||
| _ resultsMatch: Bool, |
There was a problem hiding this comment.
Nit: indenting in this file is two spaces.
| file: StaticString = #file, | ||
| function: StaticString = #function, | ||
| line: Int = #line | ||
| ) { |
There was a problem hiding this comment.
Nit: closing paren here should be de-indented.
| public func CheckResults( | ||
| _ resultsMatch: Bool, | ||
| _ message: @autoclosure () -> String | ||
| ) { |
|
I think it's fine to merge |
|
@eeckstein I’ve noticed the magic spell used by others is @swift-ci Please smoke test and merge |
|
@swift-ci Please smoke test and merge |
1 similar comment
|
@swift-ci Please smoke test and merge |
|
Eric, Dave, thank you! |
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.
In order to minimize impact of results checking on test performance, this removes the @autoclosure for error message.
Added new version of
CheckResultsthat takes onlyresultsMatch: Bool- rest of the parameters are defaulted toStaticStrings for method and file name, plus line number. Old method was deprecated, but left in place as tool for debugging failing checks. All tests were move to use the new method.Design: I assume that
StaticStringis passed toCheckResultsas a direct reference, without any retain/release overhead that would impact the measurements. The method is also marked to@inline(__always).This is a followup to @dabrahams' experiment in #9298.
Please review. cc @eeckstein, @slavapestov, @gottesmm