-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[stdlib] Set, Dictionary: Make Cocoa indices resilient for now #19599
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
@swift-ci please smoke benchmark |
@swift-ci please smoke test |
cc @airspeedswift What do you think? This leaves the door open for a Cocoa indexing overhaul. |
This comment has been minimized.
This comment has been minimized.
Hm, this seems a bit excessive. |
Gosh; |
b4cd2a7
to
326a98a
Compare
@swift-ci please smoke test |
@swift-ci please smoke benchmark |
Build comment file:Build failed before running benchmark. |
@swift-ci please smoke test |
@swift-ci please smoke benchmark |
This comment has been minimized.
This comment has been minimized.
Hm, I'll have to make the Cocoa Index storage representation I'll do that by converting it to a CoW type, which I was planning to do anyway. |
b99ff62
to
fb0cf68
Compare
@swift-ci please smoke benchmark |
@swift-ci please smoke test |
Oops, I forgot to apply these changes to Dictionary; I'm afraid these next benchmarks will still disappoint |
This enables making the struct @_fixed_layout, which should win us back some of the performance lost by hiding Cocoa indexing internals.
This comment has been minimized.
This comment has been minimized.
fb0cf68
to
900c930
Compare
@swift-ci please smoke benchmark |
@swift-ci please smoke test |
Sigh @swift-ci please smoke test linux platform |
Okay, this should do the trick. (He said, with a naive look on his face.) |
Build comment file:Performance: -O
Code size: -O
Performance: -Osize
Code size: -Osize
Performance: -Onone
How 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 regressions before you merge the PR. Noise: Sometimes the performance results (not code size!) contain false alarms. Unexpected regressions which are marked with '(?)' are probably noise. If you see regressions which you cannot explain you can try to run the benchmarks again. If regressions still show up, please consult with the performance team (@eeckstein). Hardware Overview
|
Oh hi! I'm just sitting over here, improving benchmarks by making things non-inlinable. (This probably just means we lack indexing benchmarks for bridged sets/dictionaries.) |
@@ -1897,7 +1897,7 @@ extension Dictionary.Index: Comparable { | |||
} | |||
|
|||
extension Dictionary.Index: Hashable { | |||
@inlinable | |||
@_effects(readonly) // FIXME(cocoa-index): Make inlinable |
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.
Is this correct? It writes to the hasher, after all.
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.
Oh, oops, good catch!
This makes
_CocoaSet.Index
and_CocoaDictionary.Index
fully resilient for now, allowing us to replace/fix its internals at any point in the future.