-
Notifications
You must be signed in to change notification settings - Fork 1
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
POC: OrderedSet implementation #25
Open
DandyLyons
wants to merge
15
commits into
ladvoc:main
Choose a base branch
from
DandyLyons:poc-orderedset
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Conversation
This file contains 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
I'll explore more later and push more changes. |
update from `swiftLanguageVersions` to `swiftLanguageModes`
Updates
|
Interesting Areas for Further ExplorationThe new implementation has semantics like It may or may not be advantageous to add conformance to |
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.
This PR is a proof of concept of a reimplementation of
BijectiveDictionary
using two OrderedSets rather than twoDictionary
s.Per our earlier conversation, I don't recommend this implementation until AFTER a 1.0 release.
Why
OrderedSet
is a good candidateTo maintain bijectivity, we need the following guarantees:
An
OrderedSet
should be able to guarantee these characteristics. Since it's a set, it already guarantees uniqueness. (This guarantees 1 and 2).Since it's ordered, it is indexed with an
Int
just like anArray
. (In fact, theOrderedSet
is implemented with anArray
). If we can keep the left and right collections then we will be able to use the same index on both sides. Lookups on anOrderedSet
are O(1) (just like lookups by index on an Array).Finally, while two dictionaries is a good solution, with very fast lookup times, it is very wasteful when it comes to memory. The memory usage is roughly twice that of a vanilla
Dictionary
. With two ordered sets, we are storing each value only once, so we should be using roughly half as much memory compared to the current implementation.Status
This PR is still in a very early state and I haven't yet proven that it is viable, but it is looking promising. There are some blockers, but I feel confident that we should be able to find solutions to these.
While there are definite potential benefits, including cutting our memory usage roughly in half, there are definitely some tradeoffs. For example, it seems likely that we would lose O(1) mutation (while keeping O(1) lookups). This still needs to be explored much more.
Current Blockers
UPDATE: Unblocked.
I've discovered that the
OrderedSet
API does not make it easy to update a value in place.update(_:at:) requires that the new value be equal to the old value. I'm not sure what use case this would be useful in, but it certainly doesn't match our use case.
Potential Solutions
It may not be possible to update values in place. If so we may have to first remove the values, then append the updated values. If this were the case, it would be a major bummer. It would mean that we would prefer O(1) lookups, but would almost certainly lose O(1) mutation.
Mutating
OrderedSet
through it'sUnorderedView
UPDATE: This approach seems unfeasible.
See: https://swiftpackageindex.com/apple/swift-collections/1.1.3/documentation/orderedcollections/orderedset/unorderedview
This view is O(1) for both the setter and getter.
This view should be conceptually equivalent to a
Set
since it is generic overSetAlgebra
.Mutating
OrderedSet
through it's Array ViewUPDATE: This is the approach that I used.
The
OrderedSet
API allows you to mutate values in place using a view on its Array. However this requires some post-processing, and we'll probably lose O(1) mutation. We should expect O(n) mutation instead.Other Potential Opportunities
OrderedSet
has a few other properties that could be beneficial to us, and our users. It could even have an effect on the public API.Ordered
It's conceivable that since
OrderedSet
is ordered, we could guarantee thatBijectiveDictionary
is ordered. I'm not quite sure if this is benefit, since a vanillaDictionary
is not ordered, but it's nice to know that it is a potential benefit.Random Access Collections
Ordered sets are random-access collections. This could be beneficial for certain use cases.
For more info, see: https://swiftpackageindex.com/apple/swift-collections/1.1.0/documentation/orderedcollections/orderedset#Sequence-and-Collection-Operations
We might not need
invariantCheck()
anymoreI haven't yet figured out how I would implement
invariantCheck
and in fact I'm not even certain if a variant would be possible in this new implementation.