Refactors internal func _applyMapping using _FixedArray16 #31333
Conversation
There was a problem hiding this comment.
Thanks for working on addressing this TODO @valeriyvan! I have some notes below — you might want to look at rebinding the memory inside your first array.withUnsafeMutableBufferPointer and then doing all the work inside that one closure.
| return array[..<len].withUnsafeBufferPointer { | ||
| return String._uncheckedFromUTF16($0) | ||
| return array.withUnsafeBufferPointer { (bufPtr: UnsafeBufferPointer<UInt64>) -> String in | ||
| return UnsafeBufferPointer(rebasing: bufPtr[..<len]).withMemoryRebound(to: UInt16.self) { |
There was a problem hiding this comment.
From my reading, len is the UInt16 count, but it's being used here with the UInt64 storage. Do I have that backwards?
In addition, withMemoryRebound prohibits changing the size of the bound type, so this isn't a legal call. See https://developer.apple.com/documentation/swift/unsafebufferpointer/2950055-withmemoryrebound
|
@swift-ci Please smoke test |
| return _scalar.withUTF16CodeUnits { utf16 in | ||
| var array = _FixedArray16<UInt64>(allZeros: ()) // 16 * UInt64 take 128 bytes, same as 64 * UInt16 | ||
| let string: String = array.withUnsafeMutableBufferPointer { bufPtrUInt64 in | ||
| let bufPtrUInt16: UnsafeMutablePointer<UInt16> = UnsafeMutableRawPointer(bufPtrUInt64.baseAddress._unsafelyUnwrappedUnchecked) |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| @@ -692,15 +692,16 @@ extension Unicode.Scalar.Properties { | |||
| /// all current case mappings. In the event more space is needed, it will be | |||
| /// allocated on the heap. | |||
There was a problem hiding this comment.
This doc comment doesn't match the implementation proposed in this PR.
IIUC, a buffer capable of holding 64 code units is allocated on the stack with this change instead of the heap, whereas as originally intended (but not implemented) only 16 code units would have been allocated on the stack, falling back to a larger heap allocation if needed.
I think perhaps instead what the original author had in mind to complete the TODO was as follows (typed freehand; check wellformedness with compiler):
internal func _applyMapping(_ u_strTo: _U_StrToX) -> String {
// Allocate 16 code units on the stack.
var fixedArray = _FixedArray16<UInt16>(allZeros: ())
let count: Int = fixedArray.withUnsafeMutableBufferPointer { bufPtr in
return _scalar.withUTF16CodeUnits { utf16 in
var err = __swift_stdlib_U_ZERO_ERROR
let correctSize = u_strTo(
bufPtr.baseAddress._unsafelyUnwrappedUnchecked,
Int32(bufPtr.count),
utf16.baseAddress._unsafelyUnwrappedUnchecked,
Int32(utf16.count),
"",
&err)
guard err.isSuccess else {
fatalError("Unexpected error case-converting Unicode scalar.")
}
return Int(correctSize)
}
}
if _fastPath(count <= 16) {
fixedArray.count = count
return fixedArray.withUnsafeBufferPointer {
return String._uncheckedFromUTF16($0)
}
}
// Allocate `count` code units on the heap.
var array = Array<UInt16>(repeating: 0, count: count)
array.withUnsafeMutableBufferPointer { bufPtr in
_scalar.withUTF16CodeUnits { utf16 in
var err = __swift_stdlib_U_ZERO_ERROR
let correctSize = u_strTo(
bufPtr.baseAddress._unsafelyUnwrappedUnchecked,
Int32(bufPtr.count),
utf16.baseAddress._unsafelyUnwrappedUnchecked,
Int32(utf16.count),
"",
&err)
guard err.isSuccess else {
fatalError("Unexpected error case-converting Unicode scalar.")
}
_internalInvariant(count == correctSize, "inconsistent ICU behavior")
}
}
return array.withUnsafeBufferPointer {
return String._uncheckedFromUTF16($0)
}
}There was a problem hiding this comment.
That absolutely makes sense. I've changed a little the second part.
|
@valeriyvan Great; if you wouldn't mind squashing these commits into one ( |
valeriyvan
force-pushed
the
_applyMappingUnicodeScalarProperties
branch
from
cd36f63 to
c7fc0d8
Compare
valeriyvan
force-pushed
the
_applyMappingUnicodeScalarProperties
branch
from
c7fc0d8 to
b5392e8
Compare
|
@swift-ci test |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@swift-ci Please smoke benchmark |
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 |
|
@swift-ci Please benchmark |
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 |
|
@swift-ci Please benchmark |
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 |
|
@swift-ci Please smoke test and merge |
Refactors internal func _applyMapping using _FixedArray16.
Addresses TODO.