Skip to content
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

[OrderedSet] forward ordered set equality to elements property #340

Merged
merged 11 commits into from Dec 13, 2023

Conversation

vanvoorden
Copy link
Contributor

@vanvoorden vanvoorden commented Dec 9, 2023

Background

#335

Similar to OrderedDictionary, we can forward our OrderedSet equality checks down to the ContiguousArray instance to take advantage of early return when both arrays point to the same identity (no need to perform a linear comparison over all elements).

A side-effect of this optimization is we also optimize OrderedDictionary.==, which performs an equality check on the keys property (which is an OrderedSet).[1]

[1] https://github.com/apple/swift-collections/blob/main/Sources/OrderedCollections/OrderedDictionary/OrderedDictionary%2BEquatable.swift#L20-L22


Changes

From:

public static func ==(left: Self, right: Self) -> Bool {
  left.elementsEqual(right)
}

To:

public static func ==(left: Self, right: Self) -> Bool {
  left._elements == right._elements
}

We can also migrate OrderedSet.SubSequence. From:

public static func ==(left: Self, right: Self) -> Bool {
  left.elementsEqual(right)
}

To:

public static func ==(left: Self, right: Self) -> Bool {
  left._base._elements[left._bounds] == right._base._elements[right._bounds]
}

Test Plan

Five new tests are added:

  • OrderedSetTests.test_equal
  • OrderedSetTests.test_not_equal
  • OrderedSetTests.test_equal_elements
  • OrderedSetTests.test_subsequence_equality
  • OrderedSetTests.test_subsequence_not_equality

Checklist

  • I've read the Contribution Guidelines
  • My contributions are licensed under the Swift license.
  • I've followed the coding style of the rest of the project.
  • I've added tests covering all new code paths my change adds to the project (if appropriate).
  • I've added benchmarks covering new functionality (if appropriate).
  • I've verified that my change does not break any existing tests or introduce unexplained benchmark regressions.
  • I've updated the documentation if necessary.

@vanvoorden
Copy link
Contributor Author

2023-12-11-1

Here's what the benchmarks look like running locally.

@vanvoorden
Copy link
Contributor Author

2023-12-11-2

Here are more benchmarks running locally.

Copy link
Member

@lorentey lorentey left a comment

Choose a reason for hiding this comment

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

Great catch again -- we'll definitely want to have this change. 👍

I think I found a correctness issue, though; it also indicates that we should do another pass on the test additions, to improve coverage. 🤔 I commented some suggested changes. (Beware that I haven't tried them, though. 😛)

@lorentey lorentey added this to the 1.0.6 milestone Dec 12, 2023
@lorentey lorentey added the OrderedCollections OrderedSet and OrderedDictionary label Dec 12, 2023
vanvoorden and others added 4 commits December 12, 2023 11:38
Co-authored-by: Karoy Lorentey <klorentey@apple.com>
Co-authored-by: Karoy Lorentey <klorentey@apple.com>
Comment on lines +1399 to +1412
func test_subsequence_not_equality() {
let c = 5
let items1 = OrderedSet(0 ..< c)
let items2 = OrderedSet(0 ..< c)
withEvery("i", in: 0 ..< c) { i in
let leftSlice = items1[0 ..< i]
expectNotEqual(items1[0 ..< c], leftSlice) // same identity
expectNotEqual(items2[0 ..< c], leftSlice) // different identity

let rightSlice = items1[i + 1 ..< c]
expectNotEqual(items1[0 ..< c], rightSlice) // same identity
expectNotEqual(items2[0 ..< c], rightSlice) // different identity
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lorentey I confirmed that this test "broke" on the previous equality check with the bug.

Copy link
Member

@lorentey lorentey left a comment

Choose a reason for hiding this comment

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

This looks ready! 👍

@lorentey
Copy link
Member

@swift-ci test

@lorentey lorentey merged commit 8b8a14f into apple:release/1.0 Dec 13, 2023
1 of 2 checks passed
cgrindel-self-hosted-renovate bot added a commit to cgrindel/rules_swift_package_manager that referenced this pull request Dec 21, 2023
…v1.0.6 (#822)

This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
|
[com_github_apple_swift_collections](https://togithub.com/apple/swift-collections)
| http_archive | patch | `1.0.5` -> `1.0.6` |

---

### Release Notes

<details>
<summary>apple/swift-collections
(com_github_apple_swift_collections)</summary>

###
[`v1.0.6`](https://togithub.com/apple/swift-collections/releases/tag/1.0.6):
Swift Collections 1.0.6

[Compare
Source](https://togithub.com/apple/swift-collections/compare/1.0.5...1.0.6)

This bugfix release adds `Sendable` conformances to all public types
(fixing compatibility with Swift's strict concurrency checking), and
speeds up equality checks (`==`) of identical collection values.

##### What's Changed

- Fix typos: OrderedSet Documentation by
[@&#8203;kati-kms](https://togithub.com/kati-kms) in
[apple/swift-collections#322
- \[1.0] build: support building in Debug mode on Windows by
[@&#8203;compnerd](https://togithub.com/compnerd) in
[apple/swift-collections#337
- build: tweak search path for embedding by
[@&#8203;compnerd](https://togithub.com/compnerd) in
[apple/swift-collections#338
- \[OrderedDictionary] forward ordered dictionary values equality to
values property by [@&#8203;vanvoorden](https://togithub.com/vanvoorden)
in
[apple/swift-collections#335
- \[OrderedSet] forward ordered set equality to elements property by
[@&#8203;vanvoorden](https://togithub.com/vanvoorden) in
[apple/swift-collections#340
- \[Deque] check deque equality with buffer identity by
[@&#8203;vanvoorden](https://togithub.com/vanvoorden) in
[apple/swift-collections#341
- \[OrderedDictionary] Fix usage of deprecated API in index(forKey:)
docs by [@&#8203;lorentey](https://togithub.com/lorentey) in
[apple/swift-collections#342
- \[1.0] Backport Sendable conformances on all public types by
[@&#8203;lorentey](https://togithub.com/lorentey) in
[apple/swift-collections#343
- OrderedSet: Fix sendable conformance on old swifts by
[@&#8203;lorentey](https://togithub.com/lorentey) in
[apple/swift-collections#346
- Update CMake configuration by
[@&#8203;lorentey](https://togithub.com/lorentey) in
[apple/swift-collections#347

##### New Contributors

- [@&#8203;kati-kms](https://togithub.com/kati-kms) made their first
contribution in
[apple/swift-collections#322
- [@&#8203;vanvoorden](https://togithub.com/vanvoorden) made their first
contribution in
[apple/swift-collections#335

**Full Changelog**:
apple/swift-collections@1.0.5...1.0.6

Thank you to everyone who contributed to this release!

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR is behind base branch, or you tick the
rebase/retry checkbox.

👻 **Immortal**: This PR will be recreated if closed unmerged. Get
[config help](https://togithub.com/renovatebot/renovate/discussions) if
that's undesired.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Renovate
Bot](https://togithub.com/renovatebot/renovate).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi4xMDAuMCIsInVwZGF0ZWRJblZlciI6IjM2LjEwMC4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->

Co-authored-by: Self-hosted Renovate Bot <361546+cgrindel-self-hosted-renovate[bot]@users.noreply.github.enterprise.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OrderedCollections OrderedSet and OrderedDictionary
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants