-
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
Cutting the SignalProducer overhead further. #487
Conversation
043c47f
to
104f5af
Compare
Heh, the compiler gets stuck. |
ea452ff
to
c9f93e8
Compare
e0d9c49
to
b0e1020
Compare
Sources/Signal.swift
Outdated
return self.observe { event in | ||
observer.action(event.map(transform)) | ||
} | ||
public func flatMapEvent<U, E>(_ transform: @escaping (@escaping Signal<U, E>.Observer.Action) -> (Event) -> Void) -> Signal<U, E> { |
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.
Not sure if this should be public
, but the documentation is definitely wrong.
Sources/SignalProducer.swift
Outdated
} | ||
} | ||
|
||
internal class SignalProducerCore<Value, Error: Swift.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.
Can we make this a protocol
instead of a class
? I have an aversion to inheritance.
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.
Since we don't have generalised existential for now, we would need a type-erased wrapper if we use protocol.
In the end, it would be similar to what SignalProducerCore
is doing 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.
Instead of inheritance, it can also be implemented with an enum backing (what Foundation Data
uses).
https://github.com/apple/swift/blob/master/stdlib/public/SDK/Foundation/Data.swift
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 think an enum backing would be preferable. 👍
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.
Hmm, I just realised that EventTransformingCore
relies on the inheritance to erase generic parameters. So unless we ditch EventTransformingCore
, an enum backing cannot sufficiently represent all these. 🤔
d1459d9
to
58c87a0
Compare
58c87a0
to
c5ab301
Compare
Sources/SignalProducer.swift
Outdated
} | ||
|
||
fileprivate let builder: () -> Instance | ||
fileprivate let core: SignalProducerCore<Value, 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.
Can you add some documentation for this?
Sources/SignalProducer.swift
Outdated
} | ||
} | ||
|
||
internal class SignalProducerCore<Value, Error: Swift.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.
I think an enum backing would be preferable. 👍
Sources/SignalProducer.swift
Outdated
/// the upstreams for producer interruption. | ||
/// | ||
/// `observerDidSetup` must be invoked before any other post-creation side effect. | ||
struct Product { |
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 really dislike the name Product here. Could we come up with something more specific?
Sources/SignalProducer.swift
Outdated
/// customized `observerDidSetup` post-creation side effect for the `Signal` and a | ||
/// disposable to interrupt the produced `Signal`. | ||
/// | ||
/// Unlike the safe `startWithSignal(_:)` API, `builder` shifts the responsibility of |
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.
builder
here needs to be updated
d0c2e2e
to
ce3fe65
Compare
Sources/Disposable.swift
Outdated
var isDisposed = false | ||
func dispose() {} | ||
private init() {} | ||
} |
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 breaks the contract of Disposable
by not changing isDisposed
when you call dispose
. It may be fine in practice, but this makes me very wary.
I think we'd be better off bringing back the implementation of SimpleDisposable
and using that instead.
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 treat it as disposed then? It still makes sense for its use case.
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.
Why not bring back SimpleDisposable
? This still strikes me as a little weird.
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 is a shared disposable for constant producers that practically would never be disposed of, because they terminate before the disposable is returned.
When I iterate on the design, benchmark shows allocation contributes considerable overhead over NopDisposable.shared
(which uses swift_once
). I could check again to see if the numbers are still high though.
Sources/SignalProducer.swift
Outdated
/// It is the responsibility of the `ProducedSignalReceipt` consumer to ensure the | ||
/// starting side effect is invoked exactly once, and is invoked after observations | ||
/// has properly setup. | ||
struct ProducedSignalReceipt { |
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.
How about calling this Instance
?
Sources/SignalProducer.swift
Outdated
let right = otherProducer.core.make() | ||
|
||
return .init(signal: transform(left.signal)(right.signal), | ||
observerDidSetup: { |
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 we put all the arguments on their own line? 🙈
return .init(
signal: transform(left.signal),
observerDidSetup: {
...
},
...
)
It's minor, but I think it has readability benefits. (Including outdenting the block body.)
@@ -18,6 +18,24 @@ extension Signal { | |||
/// Whether the observer should send an `interrupted` event as it deinitializes. | |||
private let interruptsOnDeinit: Bool | |||
|
|||
/// The target observer of `self`. | |||
private let wrapped: AnyObject? |
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 adding more to Observer
. 😕
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.
Sadly, we have to retain observers marked with interruptsOnDeinit
, and retaining it using a thunk adds an overhead to all calls to the observer, as opposed to an 8 byte storage for a reference. :|
// @testEventTransformingCoreMapFilter(): avg 6091 ns; min 5250 ns
self.action = transform(observer.action)
self.wrapped = observer
// @testEventTransformingCoreMapFilter(): avg 6238 ns; min 5402 ns
self.action = transform { observer.action($0) }
Sources/SignalProducer.swift
Outdated
@@ -77,23 +63,24 @@ public struct SignalProducer<Value, Error: Swift.Error> { | |||
/// - parameters: | |||
/// - startHandler: A closure that accepts observer and a disposable. | |||
public init(_ startHandler: @escaping (Signal<Value, Error>.Observer, Lifetime) -> Void) { | |||
self.init { () -> Instance in | |||
core = SignalCore { |
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 small, but the other init
s case self.init(<core instance>)
.
Can we update this one or the others to make them consistent?
Sources/SignalProducer.swift
Outdated
let interruptHandle: Disposable | ||
} | ||
|
||
func make() -> ProducedSignalReceipt { |
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 think this should be makeInstance()
(or whatever we call the struct).
Sources/SignalProducer.swift
Outdated
/// | ||
/// It is intended for constant `SignalProducers`s that synchronously emits all events | ||
/// without escaping the `Observer`. | ||
private final class EventGeneratingCore<Value, Error: Swift.Error>: SignalProducerCore<Value, 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.
We could probably just call this GeneratorCore
Sources/SignalProducer.swift
Outdated
/// level of event transforms, without any `Signal` in between. | ||
/// | ||
/// - note: This core does not use Signal unless it is requested via `make()`. | ||
private final class EventTransformingCore<Value, Error: Swift.Error, SourceValue, SourceError: Swift.Error>: SignalProducerCore<Value, 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.
We could probably call this TransformerCore
or FlatMapCore
.
This is looking good overall. 👍 👍 |
For
internal final class _SimpleDisposable: Disposable {
var isDisposed = false
func dispose() {
isDisposed = true
}
} |
798cc2d
to
4bc0c81
Compare
The cost of sequence backed producers is slightly up with the implementation being corrected in 56131fc, but the difference is trivial given its scale. The comparison in All other constant producers still use
|
Sources/SignalProducer.swift
Outdated
let instance = builder() | ||
setup(instance.producedSignal, instance.interruptHandle) | ||
guard !instance.interruptHandle.isDisposed else { return } | ||
let receipt = core.makeInstance() |
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.
receipt
should be instance
here
CHANGELOG.md
Outdated
# 2.0.1 | ||
1. Addressed the exceptionally high build time. (#495) | ||
|
||
1. New method ``retry(upTo:interval:on:)``. This delays retrying on failure by `interval` until hitting the `upTo` limitation. | ||
|
||
1. Addressed the exceptionally high build time. (#495) |
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 line is duplicated in the 2.0.1 notes. 🙈
🎉 |
Goals
Taking advantage of the deferred nature of
SignalProducer
to further reduce the overhead.TL;DR
Composing
SignalProducer
is going to be way, way cheaper than composingSignal
.Constant
SignalProducer
s, e.g.init(value:)
and sequenceinit(_:)
, are going to do significantly less work.There is still a ~16x overhead over standard library
lazy
for purely collection manipulation. But given the type erasing and reactive (passive) nature of FRP/ReactiveSwift, there is probably very little stuff we can do about it. IOW optimizing for this is a non-goal. :)The actual speedup is subject to the new internal API adoption — unmigrated
SignalProducer
operators would still be relying on the conventional behavior and/orlift
.For example, if you do:
The performance remains the same as
master
, becauseskipRepeats
hasn't been migrated yet.Introducing
SignalProducerCore
SignalProducerCore
is the new internal abstraction ofSignalProducer
. It has subsumed three major responsibilities fromSignalProducer
:the actual implementation ofstartWithSignal
;start
which accepts anSignal.Observer
; andCore.make()
, which produces aSignal
and the relevant context for composition andstartWithSignal
.SignalProducerCore
provides default implementations of all these capabilities for its subclasses, except forCore.make()
.The cores are not nested in
SignalProducer
because the Swift 3.1 compiler would get stuck. The Swift 4 compiler has resolved the issue though.SignalProducerCore
subclassesSignalCore
This is the conventional
SignalProducer
with theSignal
operator lifting optimisation in ReactiveSwift 2.0 (#140).EventTransformingCore
EventTransformingCore
composes event transforms, and is exposed as a new operatorCore.flatMapEvent
(internal for now).It takes advantage of the deferred, single-observer nature of
SignalProducer
. For example, when we do:It is contractually guaranteed that these operators would always end up producing a chain of streams, each with a single and persistent observer to its upstream. The multicasting & detaching capabilities of
Signal
is useless in these scenarios.So
EventTransformingCore
builds on top of this very fact, and composes directly at the level of event transforms. This change implicitly nullifies #129, since producers no longer useSignal
unless it absolutely has to.It is intended for all synchronous
SignalProducer
operators. To encourage code reusing, the pilot operators have their implementation moved underSignal.Event
, andSignal
has gainedflatMapEvent
to use theseEvent
-level operator implementations.This represents a new type of
SignalProducer
that does not useSignal
unless it is requested viamake()
.Example
EventGeneratingCore
EventGeneratingCore
wraps a generator closure that would be invoked when started. All the generator closure have are the observer and the cancel disposable.It is intended for the constant
SignalProducer
s, as in #486, which synchronously emits all events.It represents a new type of
SignalProducer
that does not useSignal
unless it is requested viamake()
.Pilot public APIs
map
,mapError
,filter
andfilterMap
.Various
SignalProducer.init
overloads that accept a constant.Results
(32 items)
master
min 26426 ns
min 8171 ns
min 50378 ns
min 13079 ns
min 739 ns
min 7198 ns
master
WMO
min 26136 ns
min 7169 ns
min 27497 ns
WMO
min 11382 ns
min 325 ns
min 5002 ns
WMO
Benchmark Source
Checklist