[Foundation] Import shims with @implementationOnly #28918
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
We want to enable overlays to import their shims as @_implementationOnly, so that the shims disappear from the public interface. However, this isn’t possible when a shim is called from an @inlinable func, because then the existence (and definition) of the shim needs to be available to all callers of it. Unfortunately Foundation’s Data has three instances where it calls _SwiftFoundationOverlayShims._withStackOrHeapBuffer within @inlinable code: - Data.init<S: Sequence>(_: S) - Data.append<S: Sequence>(contentsOf: S) - Data.replaceSubrange<C: Collection>(_: Range<Int>, with: C) Rewrite the first two to write sequence contents directly into the target Data instance (saving a memcpy and possibly a memory allocation). In replaceSubrange, add fast paths for contiguous collection cases, falling back to a Swift version of _withStackOrHeapBuffer with a 32-byte inline buffer. The expectation is that this will be an overall speedup in most cases, with the possible exception of replaceSubrange invocations with a large non-contiguous collection. rdar://58132561
|
@swift-ci test |
|
@swift-ci benchmark |
lorentey
changed the title
This comment has been minimized.
This comment has been minimized.
|
This seems to be a surprisingly big improvement. 😮 (I wonder if the closure calls to the shim allocated a block?) |
|
Heh, no, it's probably a side effect of me breaking append. |
lorentey
added a commit
to lorentey/swift
that referenced
this pull request
Dec 21, 2019
Data provides a settable `count` property. Its expected behavior is undocumented, but based on the implementation, it is intended to zero-extend (or truncate) the collection to the specified length. This does not work correctly if we start with an empty Data and we try to increase the count by a small integer: ``` import Foundation var d = Data() d.count = 2 print(d.count) // ⟹ 0⁉️ d.count = 100 print(d.count) // ⟹ 100 ✓ ``` It looks like this bug was introduced with the Data overhaul that shipped in Swift 5. (This issue was uncovered by swiftlang#28918.) rdar://58134026
lorentey
changed the title
|
The failure turned out to have been caused by a previously shipped regression (see #28919). I'll merge the fix here to let me rerun the tests & benchmarks. |
|
@swift-ci benchmark |
|
@swift-ci test |
Performance: -O
Code size: -O
Performance: -Osize
Code size: -Osize
Performance: -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 |
|
We got some additional (spurious-looking) benchmark regressions, but the results otherwise match the original run. 🎉 |
|
🎉 This PR has been a real rollercoaster! |
lorentey
added a commit
to lorentey/swift-corelibs-foundation
that referenced
this pull request
Jan 9, 2020
Data provides a settable `count` property. Its expected behavior is undocumented, but based on the implementation, it is intended to zero-extend (or truncate) the collection to the specified length. This does not work correctly if we start with an empty Data and we try to increase the count by a small integer: ``` import Foundation var d = Data() d.count = 2 print(d.count) // ⟹ 0⁉️ d.count = 100 print(d.count) // ⟹ 100 ✓ ``` It looks like this bug was introduced with the Data overhaul that shipped in Swift 5. (This issue was uncovered by swiftlang/swift#28918.) rdar://58134026
Data.append now calls Data.count’s setter, which used to have a bug in previously shipped versions of the Foundation overlay (swiftlang#28918). To prevent working code from breaking when recompiled with the current version, avoid calling Data.count’s setter directly. Instead, extract its implementation (conveniently already packaged into a nested function) into a named method, marking it @_alwaysEmitIntoClient.
|
@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 benchmark |
Performance: -O
Code size: -O
Performance: -Osize
Code size: -Osize
Performance: -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 |
lorentey
added a commit
to lorentey/swift-corelibs-foundation
that referenced
this pull request
Jan 9, 2020
Data provides a settable `count` property. Its expected behavior is undocumented, but based on the implementation, it is intended to zero-extend (or truncate) the collection to the specified length. This does not work correctly if we start with an empty Data and we try to increase the count by a small integer: ``` import Foundation var d = Data() d.count = 2 print(d.count) // ⟹ 0⁉️ d.count = 100 print(d.count) // ⟹ 100 ✓ ``` It looks like this bug was introduced with the Data overhaul that shipped in Swift 5. (This issue was uncovered by swiftlang/swift#28918.) rdar://58134026
phausler
reviewed
Jan 22, 2020
| @@ -62,6 +62,39 @@ internal func __NSDataIsCompact(_ data: NSData) -> Bool { | |||
|
|
|||
| #endif | |||
|
|
|||
| @_alwaysEmitIntoClient | |||
| internal func _withStackOrHeapBuffer(capacity: Int, _ body: (UnsafeMutableBufferPointer<UInt8>) -> Void) { | |||
There was a problem hiding this comment.
this does not mimic the C behavior that has a larger capacity first off and second off it is missing the jumbo capacity for the main thread (which has a larger stack allocation)
There was a problem hiding this comment.
Given the benchmarks, what's the rationale for using a large buffer?
phausler
reviewed
Jan 22, 2020
| } | ||
| // The collection might still be able to provide direct access to typed memory. | ||
| // NOTE: It's safe to do this because we're already guarding on ByteCollection's element as `UInt8`. This would not be safe on arbitrary collections. | ||
| let replaced: Void? = newElements.withContiguousStorageIfAvailable { buffer in |
There was a problem hiding this comment.
Void? as a boolean? that seems almost UB territory no?
There was a problem hiding this comment.
It's a bit unusual, but the behavior is well-defined. Void is very much distinct from nil, so there is zero chance of a type system confusion.
I initially had this return a dummy boolean, but I felt it added irrelevant detail that obscured the intent of the code:
let replaced: Bool? = newElements.withContiguousStorageIfAvailable { buffer in
_representation.replaceSubrange(subrange, with: buffer.baseAddress, count: buffer.count)
return true
}
guard replaced == nil else { return }The withContiguousStorageIfAvailable API kind of forces this sort of weird control flow. If we were to design it now, I'd argue it should return a Result-like enum rather than an Optional -- but that's too late now.
phausler
reviewed
Jan 22, 2020
|
@swift-ci test |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
parkera
approved these changes
Jan 23, 2020
|
@swift-ci test and merge |
lorentey
added a commit
to lorentey/swift
that referenced
this pull request
Apr 28, 2020
The current version of the Foundation overlay doesn’t use these, but we should still keep them in case a toolchain snapshot build picks up on overlay module from one of the SDKs in Xcode 11. rdar://62339802
lorentey
added a commit
to lorentey/swift
that referenced
this pull request
Apr 28, 2020
The current version of the Foundation overlay doesn’t use these, but we should still keep them in case a toolchain snapshot build picks up on overlay module from one of the SDKs in Xcode 11. rdar://62339802 (cherry picked from commit 057b27b)
This was referenced Apr 28, 2020
millenomi
pushed a commit
to swiftlang/swift-corelibs-foundation
that referenced
this pull request
Nov 11, 2020
Data provides a settable `count` property. Its expected behavior is undocumented, but based on the implementation, it is intended to zero-extend (or truncate) the collection to the specified length. This does not work correctly if we start with an empty Data and we try to increase the count by a small integer: ``` import Foundation var d = Data() d.count = 2 print(d.count) // ⟹ 0⁉️ d.count = 100 print(d.count) // ⟹ 100 ✓ ``` It looks like this bug was introduced with the Data overhaul that shipped in Swift 5. (This issue was uncovered by swiftlang/swift#28918.) rdar://58134026
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.
Update the Foundation overlay to import its shims with the
@_implementationOnlyattribute, i.e., privately. This removes the shims module (_SwiftFoundationOverlayShims) from the Foundation overlay's swiftinterface.Unfortunately Foundation’s
Datahas three instances where it calls_SwiftFoundationOverlayShims._withStackOrHeapBufferwithin@inlinablecode. These need to be eliminated, since the calls are currently exposed in the public interface. The three instances are in:Data.init<S: Sequence>(_: S)Data.append<S: Sequence>(contentsOf: S)Data.replaceSubrange<C: Collection>(_: Range<Int>, with: C)Rewrite the first two of these to write sequence contents directly into the target
Datainstance (saving a memcpy and possibly a memory allocation).In
replaceSubrange, add fast paths for contiguous collection cases, falling back to a Swift version of_withStackOrHeapBufferwith a 32-byte inline buffer.The expectation is that this will be an overall speedup in most cases, with the possible exception of
replaceSubrangeinvocations with a large non-contiguous collection.rdar://58132561