[stdlib][string] Refactors String(repeating:count:) #32369
Conversation
|
I just used |
valeriyvan
changed the title
valeriyvan
changed the title
| @@ -34,13 +34,19 @@ extension String { | |||
| return | |||
| } | |||
|
|
|||
| // TODO(String performance): We can directly call appendInPlace | |||
There was a problem hiding this comment.
It appears the original author intended to call the internal method appendInPlace, but this PR doesn't do that. Can you explain a bit about how your approach compares to appendInPlace?
| for i in 0..<count { | ||
| let offset = i * repeatedUTF8.count | ||
| let range = offset ..< offset + repeatedUTF8.count | ||
| _ = UnsafeMutableBufferPointer(rebasing: buffer[range]) |
There was a problem hiding this comment.
Nit: Indentation.
Question: Is there no other way to initialize a chunk of the buffer than to create another mutable buffer pointer and then throw it away?
There was a problem hiding this comment.
Question: Is there no other way to initialize a chunk of the buffer than to create another mutable buffer pointer and then throw it away?
UnsafePointer<T> has advanced(by:), which anyway produces UnsafePointer<T> - new pointer.
(Interesting why docs for UnsafePointer misses this method).
I don't see anything like advance().
UnsafeMutableBufferPointer<T> don't have advanced(by:) which might look like a gap in API.
But anyway, it would have produced a new pointer.
The answer for your question: I don't know a way to make it more effective and doubt if such a way exists.
There was a problem hiding this comment.
Nit: Indentation.
fixed
There was a problem hiding this comment.
What about using the raw pointer or raw buffer pointer APIs?
Edit: to be concrete, here's something like what I'm thinking (written freehand, not checked with compiler, could be a silly idea but I'd love to hear what you think):
var repeatedValue = repeatedValue
self = repeatedValue.withUTF8 {
let source = UnsafeRawBufferPointer($0)
return String(_uninitializedCapacity: count * source.count) {
let destination = UnsafeMutableRawBufferPointer($0)
for i in 0..<count {
UnsafeMutableRawBufferPointer(
rebasing: destination[(i &* source.count)...]
).copyMemory(from: source)
}
return count * source.count
}
}There was a problem hiding this comment.
Your code and mine both create range and pointer inside loop. Generated code should be more or less the same. The only substantial difference is copyMemory(from:) vs initialise(from:).
if copyMemory(from:) is just a wrapper around memcpy it should be more effective. The more effective the bigger repeatedValue is as I expect memcpy to be be vectorised in C library.
Found out copyMemory(from:) is a wrapper around _memmove(dest:src:size:) which is a wrapper around Builtin.int_memmove_RawPointer_RawPointer_Int64(,,,,).
My guess is - copyMemory(from:) should be faster than initialise(from:) on long enough repeatedValue but will be more or less same fast on very short (several characters) repeatedValue.
I will benchmark this as soon as PR with benchmark is merged.
There was a problem hiding this comment.
Right, and it gets rid of an addition operation in the tight loop since there's no need to calculate the upper bound :)
I also don't know if discarding the return value as you have it here is going to impact the generated code, but perhaps it's a little more elegant not to have to even if it makes no difference in the compiled binary.
(By the way, there's also no need to add to total step by step, as I don't think the copying can fail, so you can return the final value at the end.)
xwu
changed the title
|
Have added PR #32492 with benchmark |
|
ping |
|
ping |
|
Did we ever get the benchmark PR merged? I'd still like to explore the use of raw buffer APIs as discussed above, and I believe we still have a question above that's unanswered, though again I'm not sure who can answer it. |
No. PR #32492 is on review since 22 of June.
How could I be helpful with this? |
valeriyvan
force-pushed
the
StringRepeating
branch
from
51bfaf8 to
2e3da60
Compare
|
Could we benchmark, please? |
valeriyvan
force-pushed
the
StringRepeating
branch
3 times, most recently
from
ca2748a to
487fb29
Compare
|
I am trying to use var guts = _StringGuts()
guts.reserveCapacity(repeatedValue._guts.count &* count)
var repeatedValue = repeatedValue
let repeatedValueIsAscii = repeatedValue._guts.isASCII
repeatedValue.withUTF8 { repeatedUTF8 in
for _ in 0..<count {
guts.appendInPlace(repeatedUTF8, isASCII: repeatedValueIsAscii)
}
}
self.init(guts)As well as following variant, same crash: var result = String()
result.reserveCapacity(repeatedValue._guts.count &* count)
var repeatedValue = repeatedValue
let repeatedValueIsAscii = repeatedValue._guts.isASCII
repeatedValue.withUTF8 { repeatedUTF8 in
for _ in 0..<count {
result._guts.appendInPlace(repeatedUTF8, isASCII: repeatedValueIsAscii)
}
}
self = resultHere is log: |
|
This variant works: let repeatedValueGuts = repeatedValue._guts
let buffer = UnsafeMutableBufferPointer<UInt8>.allocate(capacity: repeatedValueGuts.count &* count)
var offset = 0
for _ in 0..<count {
let bufferRebased = UnsafeMutableBufferPointer(rebasing: buffer[offset...])
guard let copied = repeatedValueGuts.copyUTF8(into: bufferRebased) else {
fatalError("Buffer's capacity \(buffer.count)" +
" is not enough to accommodate '\(repeatedValue)' \(count) times")
}
offset += copied
}
_internalInvariant(offset == buffer.count)
self.init(_StringGuts(UnsafeBufferPointer(buffer), isASCII: repeatedValueGuts.isASCII))Let's benchmark it? |
|
@xwu, may I ask you run benchmark? |
|
@swift-ci benchmark |
|
I have a question ablout |
Performance (x86_64): -O
Code size: -OPerformance (x86_64): -Osize
Code size: -OsizePerformance (x86_64): -Onone
|
|
@xwu, may I ask you to run benchmark once again? Thank you. |
|
@xwu ping |
|
@swift-ci benchmark |
Co-authored-by: Karl <[email protected]>
valeriyvan
force-pushed
the
StringRepeating
branch
from
409955a to
127e0ab
Compare
|
ping |
| result += repeatedValue | ||
| let bufferRebased = UnsafeMutableBufferPointer(rebasing: buffer[offset...]) | ||
| guard let copied = repeatedValueGuts.copyUTF8(into: bufferRebased) else { | ||
| fatalError("Buffer's capacity \(buffer.count)" + |
There was a problem hiding this comment.
Unless I'm misunderstanding, this is not an error on the end user's part (the buffer capacity is not something under their control) and represents an internal logic error: probably an internal invariant failure is more appropriate here.
|
@swift-ci smoke test |
| _internalInvariant(offset == buffer.count) | ||
| self = repeatedValueGuts.isASCII ? | ||
| String._uncheckedFromASCII(UnsafeBufferPointer(buffer)) : | ||
| String._uncheckedFromUTF8(UnsafeBufferPointer(buffer), isASCII: false) |
There was a problem hiding this comment.
Do we need this ternary logic here, or does _uncheckedFromUTF8 do this anyway using the isASCII argument?
Refactors String(repeating:,count:) using String(_uninitializedCapacity:)
Addresses TODO.