[OrderedCollections] Add move operations#660
Conversation
4d3fd21 to
5e53ecc
Compare
Adds two overloads for relocating elements within an `OrderedSet`: one taking a contiguous `Range<Int>`, and one taking a `Sequence<Element>`. The moved elements occupy consecutive positions starting at the destination, in the order they appear in the input. The hot path runs in O(*d* + *k*) time, where *d* is the distance moved and *k* is the number of elements moved; the implementation falls back to O(`count`) for large moves.
5e53ecc to
508f36f
Compare
lorentey
left a comment
There was a problem hiding this comment.
Nicely done! I have some naming recommendations and a refactoring suggestion
Reworks the relocation API and extends it to `OrderedDictionary`: * Renames the index-range move to `moveSubrange(_:to:)` (with a `RangeExpression` overload) and the by-value move to `move(contentsOf:to:)`, matching the conventions of `removeSubrange`/`insert(_:at:)`. Adds `move(fromIndices:to:)`, which takes the source indices directly. * Factors the element rearrangement into generic `UnsafeMutableBufferPointer` operations (rotation / rotation+permutation / scatter) so `OrderedDictionary` relocates its keys and values in lockstep through the same code, adding only the parallel value move; the hash-table maintenance stays entirely in `OrderedSet`. * The hot path runs in O(*d* + *k*) and falls back to O(`count`) for large moves; bucket maintenance chooses targeted updates or a full rebuild by the same threshold. * `move(contentsOf:to:)` ignores elements/keys that aren't present; duplicate members are detected and trap. Adds tests (exhaustive hash-table integrity, value alignment under lifetime tracking) and move benchmarks for both types.
|
Update public API: extension OrderedSet {
public mutating func moveSubrange(_ range: some RangeExpression<Index>, to destination: Index)
public mutating func move(contentsOf elements: some Sequence<Element>, to destination: Index)
public mutating func move(fromIndices indices: [Index], to destination: Index)
}I also recalled an older conversation with my team and update I have also added another fast path. When a scattered move requires a full hash-table rebuild and the While refactoring it was now also trivial to add API to extension OrderedDictionary {
public mutating func moveSubrange(_ range: some RangeExpression<Index>, to destination: Index)
public mutating func move(contentsOf keys: some Sequence<Key>, to destination: Index)
public mutating func move(fromIndices indices: [Index], to destination: Index)
}
I wasn't too sure about the |
| extension OrderedSet { | ||
| @inlinable | ||
| internal mutating func _move( | ||
| fromIndices sourceOffsets: UnsafeBufferPointer<Int>, |
There was a problem hiding this comment.
Generally I could also replace all (or most) UnsafeBufferPointer usage with Span internally.
I shortly experimented and it seemed that at least with Swift 6.4 I didn't run into any limitations and it seemed to work just fine.
Downside is that it will require at least Swift 6.2+, maybe higher depending on if we need any API that came out later.
What's your feeling here? I don't think anything else in this module uses Span so it might be a bit inconstant.
There was a problem hiding this comment.
This is fine!
Span has SwiftStdlib 5.0 availability and that prevents us from migrating existing code to use it until the minimum deployment target of our oldest supported toolchain grows beyond that. (New API additions can start using it right now, though; they would just need to be marked with @available(SwiftStdlib 5.0, *).
7b322de to
5a6af29
Compare
| extension OrderedSet { | ||
| @inlinable | ||
| internal mutating func _move( | ||
| fromIndices sourceOffsets: UnsafeBufferPointer<Int>, |
There was a problem hiding this comment.
This is fine!
Span has SwiftStdlib 5.0 availability and that prevents us from migrating existing code to use it until the minimum deployment target of our oldest supported toolchain grows beyond that. (New API additions can start using it right now, though; they would just need to be marked with @available(SwiftStdlib 5.0, *).
Description
Adds new method for relocating elements within an
OrderedSetandOrderedDictionary:This fills a gap that comes up regularly in drag-and-drop UIs and other reordering workflows: today users have to fall back to
remove(at:)+insert(_:at:), which is O(count* k) and re-hashes elements unnecessarily.Performance
12 new benchmarks in
OrderedSetBenchmarkscover the public API across the four dispatch paths. The charts below were produced by disabling the fast paths selectively.Fast path 1: contiguous-range detection in
move(_:Sequence). When the input forms a contiguous range, dispatching to the range-rotation path avoids the compact-and-place allocation.Fast path 2: contiguous-reordered detection. When the sorted sources are contiguous but the input is in a different order, rotation followed by in-place permutation beats compact-and-place.
Fast path 3: targeted hash updates in
move(_:Range). For small range moves over short distances, touching just the affected buckets beats scanning the whole hash table.Fast path 4: targeted hash updates in scattered moves. When the affected region is small relative to table capacity, per-element bucket updates beat a full hash-table rebuild.
All benchmarks:

Source Impact
Additive.
Checklist