Skip to content

compare Set and Dictionary by address for equality #400

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

Merged

Conversation

dduan
Copy link
Contributor

@dduan dduan commented Dec 10, 2015

as noted in the comment, the native storage version of Set and Dictionary can
be considered equal if their Owner object have the same address. This should
speed up == by quite a bit in those cases.

// FIXME(performance): early exit if lhs and rhs reference the same
// storage?

if unsafeAddressOf(lhsNativeOwner) == unsafeAddressOf(rhsNativeOwner) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that's right, this code would compare addresses of the let bindings.

Does === work?

@gribozavr
Copy link
Contributor

@dduan Can you take a look at validation-test/stdlib/Set.swift and validation-test/stdlib/Dictionary.swift and verify that these cases are covered?

@dduan
Copy link
Contributor Author

dduan commented Dec 10, 2015

Hi, @gribozavr, both are covered in debug build.

cc @nadavrot

@dduan
Copy link
Contributor Author

dduan commented Dec 10, 2015

@gribozavr Switched to === for id comparison. Aforementioned tests are passing.

@gribozavr
Copy link
Contributor

@dduan Please squash commits into one.

@nadavrot Do you have performance concerns with this patch or is it good to merge? It LGTM.

as noted in the comment, the native storage version of Set and Dictionary can
be considered equal if their Owner object have the same address. This should
speed up == by quite a bit in those cases.

use === for equality of HashedCollection owner
@dduan dduan force-pushed the hashed_collection_equality_by_address branch from cfeab2b to 9c28ff0 Compare December 10, 2015 20:45
@nadavrot
Copy link
Contributor

@gribozavr @dduan No concerns. Thanks for doing the work.

@dduan
Copy link
Contributor Author

dduan commented Dec 10, 2015

@gribozavr @nadavrot squashed. Thank you :)

gribozavr added a commit that referenced this pull request Dec 10, 2015
compare Set and Dictionary by address for equality
@gribozavr gribozavr merged commit a5d48ad into swiftlang:master Dec 10, 2015
slavapestov pushed a commit to slavapestov/swift that referenced this pull request Nov 27, 2018
queue: clean up a covered switch
@dduan dduan deleted the hashed_collection_equality_by_address branch December 23, 2019 21:48
freak4pc pushed a commit to freak4pc/swift that referenced this pull request Sep 28, 2022
[perf_test] Update performance `smoke test` project configurations
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants