-
Notifications
You must be signed in to change notification settings - Fork 429
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
Streamline high-arity combineLatest
and zip
.
#345
Conversation
combineLatest
and zip
.combineLatest
.
331f0ca
to
2e9a468
Compare
84638f1
to
4b7e7c2
Compare
combineLatest
.combineLatest
and zip
.
b1a932e
to
d99848b
Compare
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.
I love that this is getting addressed. 🤘
Sources/Signal.swift
Outdated
_isInitialized = values.reduce(true, { $0 && !($1 is Placeholder) }) | ||
return _isInitialized | ||
} | ||
} |
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.
I don't love using the term initialized. How about flipping the meaning and naming it hasEmptySignal
?
Sources/Signal.swift
Outdated
|
||
private var values: ContiguousArray<Any> | ||
private var completionCount: Int | ||
private var _isInitialized: Bool |
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.
It's very minor, but in cases like this, I'd put the cached property immediately before the uncached version.
private let action: (ContiguousArray<Any>) -> Void
private var _isInitialized: Bool
private var isInitialized: Bool { … }
Sources/Signal.swift
Outdated
return true | ||
} | ||
|
||
_isInitialized = values.reduce(true, { $0 && !($1 is Placeholder) }) |
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.
Trailing closure syntax?
Sources/Signal.swift
Outdated
private struct CombineLatestStrategy: SignalAggregateStrategy { | ||
private struct Placeholder {} | ||
|
||
private var values: ContiguousArray<Any> |
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.
Would it be better to use an Optional
?
private var values: ContiguousArray<Any?>
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.
The shared aggregator interface uses (ContiguousArray<Any>) -> Void
, since zip does not need the optionality.
Sources/Signal.swift
Outdated
mutating func update(_ value: Any, at position: Int) -> Bool { | ||
values[position].append(value) | ||
|
||
if values.reduce(true, { $0 && !$1.isEmpty }) { |
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.
Maybe add a computed property for this to aid readability? Or add a mutating func
that returns a ContiguousArray<Any>?
of the values to send?
Sources/Signal.swift
Outdated
var buffer = ContiguousArray<Any>() | ||
buffer.reserveCapacity(values.count) | ||
|
||
for index in values.startIndex ..< values.endIndex { |
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.
values.indices
?
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.
Collection.indices
holds a strong reference to the collection, and is not recommended if you gonna do in-place mutation.
Sources/Signal.swift
Outdated
|
||
action(buffer) | ||
|
||
if Swift.zip(values, isCompleted).contains(where: { $0.isEmpty && $1 }) { |
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.
This conditional is repeated in complete
below. I think it'd be good to make this into a computed property.
Sources/Signal.swift
Outdated
|
||
@discardableResult | ||
func add<U>(_ signal: Signal<U, Error>) -> Self { | ||
precondition(startHandlers != nil, "Signals cannot be registered after the state has started.") |
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.
Can we do this as part of init
instead?
I think each make
below could become an init
that calls a top-level make
with the signals that it wants to start.
Sources/Signal.swift
Outdated
} | ||
|
||
private final class AggregateBuilder<Strategy: SignalAggregateStrategy> { | ||
private var startHandlers: [(Int, CompositeDisposable, Atomic<Strategy>, @escaping (Event<Never, Error>) -> Void) -> Void]? |
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.
This seems like it would benefit from a struct
. It's a bit large as a type.
Sources/Signal.swift
Outdated
return self | ||
} | ||
|
||
private func make<Composite>(count: Int, _ transform: @escaping (ContiguousArray<Any>) -> Composite) -> Signal<Composite, Error> { |
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.
Yikes :( it makes me sad that we lose type information here.
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.
Yeah. 😞 But until the type system is powerful enough, I don't mind some internal type erasure.
Managed to find a way. But we would have to pull back 6ee1fe7 (this PR) packs the tuple in one call regardless of the arity, since it is hardcoded. It is also space efficient, since the transform is a thin closure and does not need to capture closure references to repack stuff. IMO nesting isn't worth it. struct Builder<Composite, E> {
private let count: Int
private let transform: (ContiguousArray<Any>) -> Composite
func append<U, NewComposite>(_ signal: S<U, E>, _ pack: @escaping (Composite, Any) -> NewComposite) -> Builder<NewComposite, E> {
let transform = self.transform
let count = self.count
return .init(count: count + 1) { pack(transform($0), $0[count]) }
}
}
let _: Builder<(A, B, C, D, E), E> = Builder<(A, B), E>(a, b)
.append(c) { ($0.0, $0.1, $1 as! C) } // or use `repack`.
.append(d) { ($0.0, $0.1, $0.2, $1 as! D) } // or use `repack`.
.append(e) { ($0.0, $0.1, $0.2, $0.3, $1 as! E) } // or use `repack`. |
This is what I was thinking: extension Signal {
func erased() -> Signal<Any, Error> { … }
}
class Combiner<Strategy> {
var startHandlers: […]
init<Composite>(
signals: [Signal<Any, Error>],
_ transform: @escaping (ContiguousArray<Any>) -> Composite
) -> Signal<Composite, Error> {
// the code that was in make(count:_:), but also the code from add(_:)
}
convenience init<A, B>(
_ a: Signal<A, Error>,
_ b: Signal<B, Error>
) -> Signal<(A, B>), Error> {
self.init([a.erased(), b.erased()]) { ($0[0] as! A, $0[1] as! B) }
}
…
} |
I feel like 6ee1fe7 is pretty close, except for not being in the same place. extension Signal {
private convenience init<Strategy, A, B, C, D>(_ strategy: Strategy.Type, _ a: Signal<A, Error>, _ b: Signal<B, Error>, _ c: Signal<C, Error>, _ d: Signal<D, Error>) where Value == (A, B, C, D), Strategy: SignalAggregateStrategy {
self.init(AggregateBuilder<Strategy>().add(a).add(b).add(c).add(d)) {
return ($0[0] as! A, $0[1] as! B, $0[2] as! C, $0[3] as! D)
}
}
} |
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.
I'm onboard with the builder design you've used, but I've got a few other requests.
Sources/SignalProducer.swift
Outdated
} | ||
} | ||
|
||
private func flattenStart<A, B, Error>(_ disposable: CompositeDisposable, _ a: SignalProducer<A, Error>, _ b: SignalProducer<B, Error>, _ start: (Signal<A, Error>, Signal<B, Error>) -> Void) { |
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.
Can you add some documentation to these?
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.
Could these be structured after lift
instead?
Sources/SignalProducer.swift
Outdated
@@ -2200,3 +2212,66 @@ extension SignalProducer where Value == Date, Error == NoError { | |||
} | |||
} | |||
} | |||
|
|||
extension SignalProducer { | |||
fileprivate func startWithSignal(interruptingBy disposable: CompositeDisposable, setup: (Signal<Value, Error>) -> Void) { |
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.
Can you add some documentation for this?
Sources/Signal.swift
Outdated
private let action: (ContiguousArray<Any>) -> Void | ||
|
||
private var _hasAllSentInitial: Bool | ||
private var hasAllSentInitial: Bool { |
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.
This reads a little funny. Maybe rename to haveAllSentInitial
or hasAllInitialValues
?
Sources/Signal.swift
Outdated
|
||
private var hasCompletedAndEmptiedSignal: Bool { return Swift.zip(values, isCompleted).contains(where: { $0.isEmpty && $1 }) } | ||
private var canEmit: Bool { return values.reduce(true) { $0 && !$1.isEmpty } } | ||
private var isAllCompleted: Bool { return isCompleted.reduce(true) { $0 && $1 } } |
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.
Maybe rename this to areAllCompleted
so it reads nicely in English?
Sources/Signal.swift
Outdated
private var isCompleted: ContiguousArray<Bool> | ||
private let action: (ContiguousArray<Any>) -> Void | ||
|
||
private var hasCompletedAndEmptiedSignal: Bool { return Swift.zip(values, isCompleted).contains(where: { $0.isEmpty && $1 }) } |
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.
Would you mind putting these over multiple lines? I think that makes them more readable.
Binary to 10-ary `combineLatest` and `zip` operators now share a unified implementation, which has a constant calling and locking overhead, regardless of the arity.
These operators now call their `Signal` or `SignalProducer` counterpart of the same arity, instead of composing with tuple repacking on their own.
cfab84d
to
c9fc25f
Compare
c9fc25f
to
2b1fe54
Compare
Preceded by ReactiveCocoa/ReactiveCocoa#2521.
Fixes #156.
Signal
regardless of the arity or the number of signals.master
combineLatest
,Signal
combineLatest
,SignalProducer
combineLatest
,Signal
combineLatest
,SignalProducer
Note
combineLatest
andzip
adds signals in a chain to aSingal.AggregateBuilder
, which constructs aSignal
when the chain ends withmake()
.Singal.AggregateBuilder
encapsulates all the type erasure & recovery.In ReactiveCocoa/ReactiveCocoa#2521, a primary dissent was that it introduced risks that the compiler cannot statically validate. This PR does not, and cannot mitigate, such risk with the performance goal due to the lack of variadic generics.
Note: Xcode 8.3.2, i5 6360U, Whole Module Optimisation. Benchmark Source (Gist).