-
Notifications
You must be signed in to change notification settings - Fork 10.6k
[SE-0494][StdLib] Add isTriviallyIdentical(to:) Methods to Array, ArraySlice, and ContiguousArray
#82438
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
8ab92b0 to
0ccbcec
Compare
stdlib/public/core/Array.swift
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer this (and everything else in this PR) to be @_alwaysEmitIntoClient instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Azoy TBH… I have zero experience with either option. From reading through SE-0376 it sounds like backDeployed came with some advantages:
While @_alwaysEmitIntoClient can be used to back deploy APIs, there are some drawbacks to using it. Since a copy of the function is always emitted, there is code size overhead for every client even if the client's deployment target is new enough that the library API would always be available at runtime. Additionally, if the implementation of the API were to change in order to improve performance, fix a bug, or close a security hole then the client would need to be recompiled against a new SDK before users benefit from those changes.
I don't see much discussion in that proposal over when a library maintainer would still prefer _alwaysEmitIntoClient.
I did find this discussion from @lorentey:
It sounds like one issue here was that debugDescription was used to conform to a protocol. Our isIdentical function here would not be used to conform to a protocol. It sounds like that might make safer shipping backDeployed?
But we did leave a FIXME comment:
That we eventually want to make this backDeployed.
Hmm… would you have any more specific ideas why we prefer _alwaysEmitIntoClient here for these changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because @backDeployed commits this as the stdlib's ABI vs. @_aEIC which does not. If we find we need to replace this in the future with some more generalized thing or such, we pay the price of having to maintain this forever instead of just being able to update the definition. @_aEIC is the best attribute in my opinion because it is the "pay for what you use" attribute both for the stdlib and the client. The stdlib doesn't have to take the code size hit (unless it started using it in its own opaque implementation) or the ABI hit, and clients don't pay for anything unless they use it themselves or use something that uses it.
There's also the fact that @backDeployed introduces a runtime availability check for some configurations which does have a performance cost vs. @_aEIC which does not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Azoy SGTM. I'll make the changes and push a new commit. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
781b431 to
8d86f96
Compare
8d86f96 to
51a04aa
Compare
51a04aa to
c1a4725
Compare
isIdentical Methods for Quick Comparisons to Array, ArraySlice, and ContiguousArrayisIdentical Methods for Quick Comparisons to Array, ArraySlice, and ContiguousArray
isIdentical Methods for Quick Comparisons to Array, ArraySlice, and ContiguousArrayisTriviallyIdentical(to:) Methods for Quick Comparisons to Array, ArraySlice, and ContiguousArray
stdlib/public/core/ArraySlice.swift
Outdated
| @_alwaysEmitIntoClient | ||
| public func isTriviallyIdentical(to other: Self) -> Bool { | ||
| self._buffer.identity == other._buffer.identity && | ||
| self.count == other.count |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lorentey I think we have to check the count here… or else we could have the same buffer with two different ranges that compare as identical. But if the buffer identity itself already defends against that then I can remove this extra check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does appear sound for all cases I've seen in practice -- the buffer identity is simply the address of its first element, and including the count guarantees that the two slices are backed by the same region of memory.
But two slices being backed by the same memory does not necessarily imply that they must be identical slices. It does leave me worried that this shortcut may misfire in some exotic/hypothetical cases, such as if two distinct bridged read-only NSArray instances somehow end up getting backed by overlapping memory regions, and therefore two array slices may be backed by the same memory, but still be distinguishable -- for example, by looking at their startIndex. I'm not aware of any actual NSArray subclass that would do that, but I do believe it is technically possible.
To avoid this (and (perhaps more practical) similar cases I may be missing), I think we should just follow best practice and simply compare all stored properties, with no clever shortcuts, like in the draft below:
extension _SliceBuffer {
@_alwaysEmitIntoClient
internal func isTriviallyIdentical(to other: Self) -> Bool {
self.owner == other.owner
&& self.subscriptBaseAddress == other.subscriptBaseAddress
&& self.startIndex == other.startIndex
&& self.endIndexAndFlags == other.endIndexAndFlags
}
}
extension ArraySlice {
@_alwaysEmitIntoClient
public func isTriviallyIdentical(to other: Self) -> Bool {
self._buffer.isTriviallyIdentical(to: other._buffer)
}
}
(Note that this only applies to array slices; I believe comparing _buffer.identity will suffice for Array and ContiguousArray. In the bridged case, the identity is simply the object identity of the NSArray instance.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lorentey Ahh… that's a good idea! I'll push a new commit. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm… I'm running into trouble now trying to compare self.owner to other.owner because of the Builtin.NativeObject type…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I need this one… but for some reason I'm not able to call it without a compiler error…
|
@swift-ci Please smoke test |
isTriviallyIdentical(to:) Methods for Quick Comparisons to Array, ArraySlice, and ContiguousArrayisTriviallyIdentical(to:) Methods to Array, ArraySlice, and ContiguousArray
|
@swift-ci Please smoke test |
|
I think the smoke test failures might be related to #84925. |
stdlib/public/core/SliceBuffer.swift
Outdated
| // FIXME: use builtin == function | ||
| // self.owner == other.owner, | ||
| unsafe unsafeBitCast(self.owner, to: Int.self) == unsafeBitCast(other.owner, to: Int.self), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you'll need to use:
-
==for Embedded Swift, becauseownerisBuiltin.NativeObject(via the_SliceBuffer.AnyObjecttypealias). -
===for standard Swift, becauseownerisSwift.AnyObject.
The status page lists AnyObject as currently supported in Embedded Swift, so perhaps this can be simplified in the future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@benrimmington Ahh… that's a good idea! I'll push a new commit. Thanks!
|
Since your tests are the same for all types, you could try moving them to: where they'll be used by: |
|
@swift-ci Please smoke test |
|
@swift-ci Please smoke test |
1 similar comment
|
@swift-ci Please smoke test |
|
@swift-ci Please test |
|
@swift-ci Please test macOS platform |
|
@swift-ci please Apple silicon benchmark |
|
@swift-ci Please test macOS platform |
|
@swift-ci Please benchmark |
| /// compare equal with `==`, but not all equal arrays are considered | ||
| /// identical. | ||
| /// | ||
| /// - Complexity: O(1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amartini51 Please could you review this documentation comment for the Array.isTriviallyIdentical(to:) method.
(ArraySlice, ContiguousArray, Dictionary, Set, String, and Substring will share basically the same documentation.)
My suggestions and observations are:
-
Capitalize the word "Boolean".
-
Possibly combine and generalize the first two sentences.
Returns a Boolean value indicating whether this instance is indistinguishable from the other instance.
-
Possibly generalize the last paragraph (by replacing "array" with "instance" or "value").
-
The fourth list item (
Equatable) is repeated in the last paragraph. -
Are fenced code blocks (with three backticks) allowed in the standard library?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are all great ideas and I'm also open to workshopping this documentation from a more "conceptual" POV. So I think maybe as an alternative to some of the more tactical feedback on cleaning up specific details of this documentation using the current overall philosophy I am also open to brainstorming together how else we could think about teaching the ideas of these operations to engineers that are new to this topic. Some potential ideas and directions that sounded interesting were shared in our review thread.1
Because documentation fixes are easy to patch on release branches I also think we might be able to prioritize landing the implementations and getting the code right first so we can make sure that lands before the branch cut. If we then spend more time on improving documentation and we need to potentially patch after a branch cut then that is a very low risk change as opposed to trying to patch an implementation fix.
How does that sound to you? Any more ideas about that?
Footnotes
|
@swift-ci Please Build Toolchain macOS Platform |
amartini51
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replying to the points in the comment where I was at-mentioned:
- Yes, Boolean needs to be capitalized per Apple style guide.
- Yes, we can continue to iterate on the docs after the implementation lands. Thanks for including them in your initial PR!
- As you combine the first two sentences, ensure the abstract remains a single sentence without links or code voice. (The abstract is the text before the first blank line.) It needs to stand on its own in places like IDE help, autocomplete, and navigation where the rest of the docs aren’t shown.
- The stdlib normally uses indented code listings, although DocC does support both. I’m not sure where the precedent for indentation comes from, but it might be from the pre-DocC tools we used to publish documentation from comments.
- The word “trivial” appears in the symbol name and was discussed as part of the SE proposal, but there isn’t anything in the docs here about that part of the API. Can we add something? My understanding is that this returns
trueonly when the arrays can be trivially compared — that this is always fast, but==might be slower.
|
@swift-ci test |
| /// Returns a boolean value indicating whether this array is identical to | ||
| /// `other`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copying @benrimmington's suggestion from below, which I also think is an improvement:
| /// Returns a boolean value indicating whether this array is identical to | |
| /// `other`. | |
| /// Returns a Boolean value indicating whether this instance is indistinguishable | |
| /// from the other instance. |
Although I think it works fine with "array" instead of "instance".
Co-Authored-By: Ben Rimmington <[email protected]>
Co-authored-by: Alex Martini <[email protected]>
34ecc96 to
ee14e1f
Compare
|
@swift-ci Please test |
|
@swift-ci please Apple silicon benchmark |
|
@swift-ci Please benchmark |
|
@swift-ci Please Build Toolchain macOS Platform |
|
@amartini51 I appreciate all the help working through the documentation! I think my preference for now is to go ahead and try to land these implementations so we can start soaking ASAP. But I do want to open a new task for us to continue workshopping the documentation across all the types we merged in this proposal. I can tag you over there if you are interested in helping us to keep the conversation going. Thanks! |
|
@lorentey I think this one looks good to merge now. Thanks! |
|
@benrimmington @amartini51 We can track documentation improvements over in #86000. Thanks! |
|
@lorentey Thanks for all the help! Please let us know if you have any more ideas to help improve the documentation and we can always pick those changes in later on. |
Background
SE-0494 has been accepted by LSG.
Changes
Our
Arrayalready performs a "fast path" for equality checking in our==operator.1 We can implement a similar check forisIdentical:There are similar fast paths in
ArraySliceandContiguousArray.23Test Plan
New tests were added for
Array,ArraySlice, andContiguousArray.Benchmarks
New benchmarks were added for
Array,ArraySlice, andContiguousArray.Footnotes
https://github.com/swiftlang/swift/blob/swift-6.1.2-RELEASE/stdlib/public/core/Array.swift#L1816-L1824 ↩
https://github.com/swiftlang/swift/blob/swift-6.1.2-RELEASE/stdlib/public/core/ArraySlice.swift#L1398-L1406 ↩
https://github.com/swiftlang/swift/blob/swift-6.1.2-RELEASE/stdlib/public/core/ContiguousArray.swift#L1335-L1343 ↩