Optimize contains method implementation in CxxSet
#85488
Conversation
CrazyFanFan
requested review from
Xazax-hun,
beccadax,
egorzhdan,
hnrklssn,
ian-twilightcoder,
j-hui and
susmonteiro
as code owners
|
@swift-ci please test |
|
The Windows CI tests are currently failing, but looking at the logs, it looks like it’s unrelated to my changes here. |
|
|
||
| init() | ||
|
|
||
| func __endUnsafe() -> RawIterator | ||
| func __findUnsafe(_ value: Element) -> RawIterator |
There was a problem hiding this comment.
This was mutating previously, is there a reason why it's not anymore?
There was a problem hiding this comment.
Thanks for pointing this out!
I removed mutating because neither __endUnsafe() nor __findUnsafe(_:) modifies the underlying storage—this aligns with their semantic purpose, and they’re used in the non-mutating contains(_:).
That said, I recognize this is a breaking change to the public API. Though, since these methods are prefixed with __, I wondered if that might make the change more acceptable.
I’d like to align with the project’s preferences here—would you recommend restoring the original signatures to preserve compatibility, or keeping the semantically accurate version?
Happy to implement whichever approach makes the most sense.
There was a problem hiding this comment.
The Cxx and CxxStdlib overlays aren't part of the Swift's ABI stability guarantees, so it's totally fine to change their interfaces. For this reason these two libraries are distributed as part of the toolchain, not part of the SDK, and are version-locked to the compiler.
As long as making these functions non-mutating doesn't break any tests, it should be OK to make this change.
| @@ -19,16 +19,22 @@ | |||
| public protocol CxxSet<Element>: ExpressibleByArrayLiteral { | |||
| associatedtype Element | |||
| associatedtype Size: BinaryInteger | |||
| associatedtype RawIterator: UnsafeCxxInputIterator | |||
| where RawIterator.Pointee == Element | |||
| associatedtype RawMutableIterator: UnsafeCxxInputIterator | |||
There was a problem hiding this comment.
Should this not be UnsafeCxxMutableInputIterator?
There was a problem hiding this comment.
std::set iterators are indeed immutable since modifying set elements would break the internal ordering. This aligns with the existing StdSet implementation in Swift's ClangImporter
|
@swift-ci please smoke test windows platform |
CrazyFanFan
force-pushed
the
optimization/cxx_set_contains
branch
from
197856e to
f4eecc9
Compare
|
@swift-ci please test |
egorzhdan
left a comment
egorzhdan
left a comment
There was a problem hiding this comment.
Thanks @CrazyFanFan! As long as the CI is green, this LGTM.
hnrklssn
left a comment
hnrklssn
left a comment
There was a problem hiding this comment.
There's a couple of C++ interop benchmarks in https://github.com/swiftlang/swift/tree/main/benchmark/cxx-source. Feel free to add one for CxxSet.contains if you want. I won't block on it though.
|
I'll merge this PR now. @CrazyFanFan if you're interested in adding a benchmark as @hnrklssn suggested, please feel free to submit a separate PR – no pressure on that though 😉 |
3 participants
Replaces
count(element) > 0with__findUnsafe(element) != __endUnsafe()inCxxSet.containsimplementation to avoid redundant counting operations and improve performance.