[stdlib] Modernize Set and Dictionary internals #18181
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
| @@ -390,7 +382,7 @@ extension Set: Collection { | |||
| /// to the set. Don't expect any particular ordering of set elements. | |||
| /// | |||
| /// If the set is empty, the value of this property is `nil`. | |||
| @inlinable // FIXME(sil-serialize-all) | |||
| @inlinable | |||
| public var first: Element? { | |||
There was a problem hiding this comment.
I wonder if this really does materially perform better than the default.
There was a problem hiding this comment.
The default creates an iterator, which complicates the native case, but improves the bridged case -- _CocoaSet.startIndex is currently O(n), while the iterator uses fast enumeration. 😦
Ideally this should dispatch to _variant and do the right thing in each case. Or we could just remove it.
There was a problem hiding this comment.
Actually, it turns out this is the correct way to implement this, despite the performance hit.
Bridged indices are currently based on -[NSDictionary allKeys], which is not guaranteed to use the same element ordering as an enumerator. (Which indicates Dictionary/Set iterators may sometimes return elements in a different order than indices. 😭)
I think we should change indices to use enumerators, and soon.
lorentey
force-pushed
the
dictionary-review
branch
2 times, most recently
from
4f2c2a1 to
4a7bb11
Compare
lorentey
changed the title
|
@swift-ci please test |
|
The benchmarks shouldn't be affected, although there were a couple changes to inlinability. |
|
Build failed |
|
Build failed |
Build comment file:Optimized (O)Regression (22)
Improvement (19)
No Changes (419)
Unoptimized (Onone)Regression (14)
Improvement (25)
No Changes (421)
Hardware Overview |
|
@swift-ci please test |
|
@swift-ci test |
|
@swift-ci benchmark |
|
The |
|
Build failed |
|
Build failed |
Build comment file:Optimized (O)Regression (13)
Improvement (15)
No Changes (431)
Unoptimized (Onone)Regression (14)
Improvement (28)
No Changes (417)
Hardware Overview |
|
The plot thickens! |
|
Please test with following PR: @swift-ci please test |
|
Build failed |
lorentey
force-pushed
the
dictionary-review
branch
from
b89f2df to
646e22d
Compare
|
Please test with the following PR: @swift-ci test |
|
Build failed |
lorentey
force-pushed
the
dictionary-review
branch
from
2b44d08 to
a5caa28
Compare
|
Please test with the following PR: @swift-ci test |
|
@swift-ci please test and merge |
lorentey
force-pushed
the
dictionary-review
branch
from
7461b7d to
b98c0e4
Compare
- Rename variant enum types and move them into the corresponding struct: _VariantSetBuffer => Set._Variant _VariantDictionaryBuffer => Dictionary._Variant I found the buffer suffix did not positively contribute to understanding these types. And the extra indirection of the typealiases wasn't exactly helpful, either. - Remove internal typealiases that resolved to these.
This is primarily for documentation purposes, although the default implementation (based on an iterator) may not return the correct value for bridged dictionaries with exotic implementations.
A Dictionary.removeValue(forKey:) benchmark regressed 35% because recent changes in this PR caused an _UnsafeBitMap member to not be inlined in its implementation. (This was probably triggered by moving a method from Dictionary._Variant to _NativeDictionary.) Add @inline(__always) to _UnsafeBitMap members. While we’re at it, make _UnsafeBitMap @usableFromInline. It’s only public for testing purposes.
… property names This makes it easier to define lldb data formatters for these types.
DictionaryIterator & SetIterator have been renamed Dictionary.Iterator and Set.Iterator, with compatibility typealiases.
The startBucket argument is _bucket(key) in the vast majority of cases. Add an overload with this default value.
lorentey
force-pushed
the
dictionary-review
branch
from
b98c0e4 to
fa0a96e
Compare
|
@swift-ci please test |
lorentey
force-pushed
the
dictionary-review
branch
from
fa0a96e to
ac92c02
Compare
|
So many typos... 🤦♂️ @swift-ci please test |
lorentey
force-pushed
the
dictionary-review
branch
from
ac92c02 to
d1ec3b9
Compare
|
@swift-ci please test |
lorentey
changed the title
|
@swift-ci please smoke test compiler performance |
1 similar comment
|
@swift-ci please smoke test compiler performance |
Build comment file:Summary for master smoketestUnexpected test results, excluded stats for Kingfisher, ReactiveCocoa Regressions found (see below) Debugdebug briefRegressed (0)
Improved (0)
Unchanged (delta < 1.0% or delta < 100.0ms) (3)
debug detailedRegressed (1)
Improved (0)
Unchanged (delta < 1.0% or delta < 100.0ms) (22)
Releaserelease briefRegressed (0)
Improved (1)
Unchanged (delta < 1.0% or delta < 100.0ms) (2)
release detailedRegressed (1)
Improved (5)
Unchanged (delta < 1.0% or delta < 100.0ms) (17)
|
|
That first wall time regression was just noise, then, not a problem. |
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.
Clean up Set and Dictionary internals to improve readability and to prepare for upcoming resiliency improvements.
Nest public iterator types under the corresponding collection type, with a compatibility typealias.
SetIterator<Element>⟹Set<Element>.IteratorDictionaryIterator<Key, Value>⟹Dictionary<Key, Value>.IteratorNest auxiliary types under the corresponding higher-level type. Remove confusing
Buffersuffix. Unify naming of enums representing native/cocoa variants._VariantSetBuffer<Element>⟹Set<Element>._VariantSetIndexRepresentation<Element>⟹Set<Element>.Index._VariantSetIteratorRepresentation<Element>⟹Set<Element>.Iterator._Variant_NativeSetBuffer<Element>⟹_NativeSet<Element>_NativeSetIndex<Element>⟹_NativeSet<Element>.Index_CocoaSetBuffer⟹_CocoaSet_CocoaSetIndex⟹_CocoaSet.Index_CocoaSetIterator⟹_CocoaSet.Iterator(Ditto for
Dictionary.)Add explicit types representing native iterators:
_NativeSet<Element>.Iteratorand_NativeDictionary<Key, Value>.Iterator. Previously these were exploded into associated values on the native case of the top-level iterator variant.Make Cocoa helper class names a little bit more helpful in Cocoa context.
_NativeSetNSEnumerator⟹_SwiftSetNSEnumerator_NativeDictionaryNSEnumerator⟹_SwiftDictionaryNSEnumeratorAdjust internal property and variable names accordingly.
To enhance readability, remove internal typealiases wherever possible.
Audit inlinability.
rdar://problem/42585384