[stdlib][SR-7136] Fast-path ad-hoc printing of Strings #15108
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
|
@swift-ci Please smoke test |
jckarter
requested changes
Mar 9, 2018
| ) { | ||
| if let string = value as? String { | ||
| let mirror = Mirror(reflecting: string) | ||
| _adHocPrint_unlocked(string, mirror, &target, isDebugPrint: false) |
There was a problem hiding this comment.
You can just do target.write(string).
|
Thanks for looking into this! _adHocPrint is probably not going to be much faster in practice, though. You can use |
|
@jckarter Thank you for reviewing! I fixed it. |
|
Awesome, thanks! I'll run the tests and merge. |
jckarter
approved these changes
Mar 9, 2018
|
@swift-ci Please smoke test |
|
Oh, |
|
OK, I'll fix it again. 👍 |
|
@swift-ci Please smoke test |
|
@swift-ci please smoke benchmark |
|
Do we have anything to measure this speedup? I'm interested in detecting regressions and exploring further improvements here such as plumbing this higher up. |
Build comment file:Optimized (O)Regression (6)
Improvement (5)
No Changes (374)
Unoptimized (Onone)Regression (10)
Improvement (12)
No Changes (363)
Hardware Overview |
|
(I suspect the string builders are noisy) @swift-ci please benchmark |
|
@swift-ci please smoke benchmark |
|
@swift-ci please smoke test |
Build comment file:Optimized (O)Regression (7)
Improvement (12)
No Changes (366)
Unoptimized (Onone)Regression (9)
Improvement (7)
No Changes (369)
Hardware Overview |
|
@swift-ci please smoke test |
|
@swift-ci please smoke benchmark |
|
I suspect the primary benefit would be in startup time and memory usage, since this should make it so that |
Performance: -O
Code size: -OPerformance: -Osize
Code size: -OsizePerformance: -Onone
Code size: -swiftlibsHow to read the dataThe tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.If you see any unexpected regressions, you should consider fixing the Noise: Sometimes the performance results (not code size!) contain false Hardware Overview |
|
Hmph. Yeah, this looks like noise. |
|
Do we have a benchmark (or should we write one) that will demonstrate the improvement? |
|
We might be able to rig something up that captures the warmup time aspect. We could definitely do with more memory usage benchmarks, but that might need more infrastructure. |
|
I've filed https://bugs.swift.org/browse/SR-12572 about the benchmark infrastructure @swift-ci test |
|
⛵ |
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.
I added dynamically checked whether something is a String and just print the string.
Resolves SR-7136.