Some performance improvements in the stdlib, mainly for small string literals #16890
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
…ull-terminated C string. And remove the now unused _SentinelCollection utility.
We can always eliminate ARC operations on the result of a value_to_bridge_object instruction. There is no need to restrict the operand to a specific SIL pattern.
…nd pointers to start-pointer + count. This saves a few instructions for some operations, like getting the count. Also, it avoids the check for unwrapping the optional end pointer. For example, iterating over an unsafe buffer now has no overhead. Also remove the _unboundedStartingAt initializer, which is not needed anymore.
Making sure that makeIterator is always inlined, enables devirutalization of the iterator calls. Inlining was not done with -Osize which resulted in pretty bad performance when iterating over an existential collection.
…o 2 constant loads. The main part of this is to rewrite the small string literal-constructor to work with values (= shifting bytes) instead of setting bytes in memory. This allows the compiler to fold away everything and end up with the optimal code for small string literals.
|
@swift-ci test |
|
@swift-ci smoke benchmark |
|
@swift-ci test compiler performance |
Build comment file:Build failed before running benchmark. |
|
@swift-ci smoke benchmark |
milseman
requested changes
May 29, 2018
| } | ||
| _sanityCheck(self.count == 0, "overwrote count early?") | ||
| self.count = count | ||
| high |= (UInt(count) &<< (8*15)) |
There was a problem hiding this comment.
Can you instead put this in the setter for count, so that all other code paths can benefit?
Also, rather than have a naked 15, you can use capacity.
There was a problem hiding this comment.
Actually, the count setter would have to mask out the old value, which is not needed here.
Also, I followed the count getter+setter pattern which also hard-code the '15'.
| self.count = count | ||
| high |= (UInt(count) &<< (8*15)) | ||
| let low = _bytesToUInt(addr, lowCount) | ||
| _storage = (low, high) |
There was a problem hiding this comment.
I think the code would be more clear if this was in the if-else blocks. We'd also not need to introduce so many in-scope variables.
There was a problem hiding this comment.
I wanted to reduce the number of calls to _bytesToUInt. Although llvm can fold away the whole loop, this is not yet done on SIL level, so duplicating _bytes_ToUInt in the then- and else-branch generates an additional loop in SIL.
| shift = shift &+ 8 | ||
| } | ||
| return r | ||
| } |
There was a problem hiding this comment.
Isn't this whole function just a (potentially-unaligned) load and bitfield-insert mask? Can you verify what code is generated here?
There was a problem hiding this comment.
The whole code is folded away (that's the point of the change). I added a test for that: SILOptimizer/string_literals.swift
There was a problem hiding this comment.
(following up), this code is also called for many other dynamic String initializations, so we'd want efficient code even when it can't be constant folded.
There was a problem hiding this comment.
Erik and I discussed changing the UBP representation. I couldn't think of any good reason to keep the (start, end] representation for the buffer, as long as the iterator keeps its representation.
Build comment file:Optimized (O)Regression (39)
Improvement (26)
No Changes (373)
Hardware Overview |
milseman
approved these changes
May 29, 2018
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.
For details see the commit list