-
-
Notifications
You must be signed in to change notification settings - Fork 18
Topological order update #122
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
Changes from all commits
a6adf5d
f3ffb28
ca18408
bc78787
080e94a
015b171
d98d473
e131fa5
bb79c39
e7ad694
b84abaa
d4e0849
578e108
f90fd3c
d4a68a5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,12 +1,12 @@ | ||
| @MainActor | ||
| final class SubscriberState { | ||
| let token = SubscriberKey.Token() | ||
| var subscribingKeys = Set<AtomKey>() | ||
| var subscribing = Set<AtomKey>() | ||
| var unsubscribe: ((Set<AtomKey>) -> Void)? | ||
|
|
||
| init() {} | ||
|
|
||
| deinit { | ||
| unsubscribe?(subscribingKeys) | ||
| unsubscribe?(subscribing) | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,72 @@ | ||
| internal enum Vertex: Hashable { | ||
| case atom(key: AtomKey) | ||
| case subscriber(key: SubscriberKey) | ||
| } | ||
|
|
||
| internal struct Edge: Hashable { | ||
| let from: AtomKey | ||
| let to: Vertex | ||
| } | ||
|
|
||
| /// DFS topological sorting. | ||
| @MainActor | ||
| internal func topologicalSort(key: AtomKey, store: AtomStore) -> ( | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Global main actor func just to access main actor isolated [nit]
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Sounds good to me.
I believe how it is sorted is important for the caller as the additional algorithm on the StoreContext doesn't work except with topological sorting.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm already on a bunch of refactoring in another branch, so will apply it there. |
||
| edges: ReversedCollection<[Edge]>, | ||
| redundants: [Vertex: [AtomKey]] // key = vertex, value = dependencies | ||
| ) { | ||
| var trace = Set<Vertex>() | ||
| var edges = [Edge]() | ||
| var redundants = [Vertex: [AtomKey]]() | ||
|
|
||
| func traverse(key: AtomKey, isRedundant: Bool) { | ||
| if let children = store.graph.children[key] { | ||
| for child in ContiguousArray(children) { | ||
| traverse(key: child, from: key, isRedundant: isRedundant) | ||
| } | ||
| } | ||
|
|
||
| if let subscriptions = store.state.subscriptions[key] { | ||
| for subscriberKey in ContiguousArray(subscriptions.keys) { | ||
| traverse(key: subscriberKey, from: key, isRedundant: isRedundant) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| func traverse(key: AtomKey, from fromKey: AtomKey, isRedundant: Bool) { | ||
| let vertex = Vertex.atom(key: key) | ||
| let isRedundant = isRedundant || trace.contains(vertex) | ||
|
|
||
| trace.insert(vertex) | ||
|
|
||
| // Do not stop traversing downstream even when edges are already traced | ||
| // to analyze the redundant edges later. | ||
| traverse(key: key, isRedundant: isRedundant) | ||
|
|
||
| if isRedundant { | ||
| redundants[vertex, default: []].append(fromKey) | ||
| } | ||
| else { | ||
| let edge = Edge(from: fromKey, to: vertex) | ||
| edges.append(edge) | ||
| } | ||
| } | ||
|
|
||
| func traverse(key: SubscriberKey, from fromKey: AtomKey, isRedundant: Bool) { | ||
| let vertex = Vertex.subscriber(key: key) | ||
| let isRedundant = isRedundant || trace.contains(vertex) | ||
|
|
||
| trace.insert(vertex) | ||
|
|
||
| if isRedundant { | ||
| redundants[vertex, default: []].append(fromKey) | ||
| } | ||
| else { | ||
| let edge = Edge(from: fromKey, to: vertex) | ||
| edges.append(edge) | ||
| } | ||
| } | ||
|
|
||
| traverse(key: key, isRedundant: false) | ||
|
|
||
| return (edges: edges.reversed(), redundants: redundants) | ||
| } | ||
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.
Q: Does this mean we're changing the timing of the side effects, and in case usage accidentally relies on the current order (performing side effects at the last of this method), it might get affected?
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.
Yes, before the change, it runs a side effect after finishing updating downstream, but now it runs immediately after updating its value.
In the first place, the order in which the atoms were updated was not guaranteed, so it's not possible that they were used in a way that depended on the update order. (Even if there were, it wouldn't work properly)
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.
Understood, thank you!