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

Less hops in operator lifting. #140

Merged
merged 3 commits into from
Jun 2, 2017
Merged

Less hops in operator lifting. #140

merged 3 commits into from
Jun 2, 2017

Conversation

andersio
Copy link
Member

@andersio andersio commented Dec 3, 2016

The PR proposes to overhaul how SignalProducer produces a new instance of Signal.

Summary

  1. The public API of SignalProducer remains untouched.

  2. startWithSignal(_:) is no longer responsible of creating the Signal.

  3. SignalProducer.Builder replaces the "start handler" as the internal representation of SignalProducer.

    The builder is responsible of creating an instance of Signal on request. It also creates a customised post-creation side effect and an interrupt handle for every produced Signal.

    Unlike startWithSignal(_:), the caller is responsible of invoking the post-creation side effect i.e. didCreate. The swap in responsibility here gives a room of optimisation for lifted Signal operators. They can now dodge the unnecessary relay Signal that would otherwise be created by the old startWithSignal(_:).

  4. Condensed the description of SignalProducer.

Result

For every lifted Signal operator applied, one less Signal would be produced. So given a producer chain SignalProducer(signal).map.map.map, it would now produce a graph of four Signals, instead of seven.

///
/// - returns: A signal producer that applies signal's operator to every
/// created signal.
public func lift<U, F>(_ transform: @escaping (Signal<Value, Error>) -> Signal<U, F>) -> SignalProducer<U, F> {
Copy link
Member Author

@andersio andersio Dec 3, 2016

Choose a reason for hiding this comment

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

These trampolines are made as liftLeft and liftRight cannot be included in SignalProducerProtocol (except the unary lift). Having said that, these trampolines can be removed when Swift 3.1 lands, should we drop the protocol in favour of concrete same-type requirements.

@andersio andersio force-pushed the producer-lift branch 5 times, most recently from 840259b to 2ca48ce Compare December 3, 2016 04:44
@andersio
Copy link
Member Author

andersio commented Dec 3, 2016

It seems intermediate signals do not get interrupted events but are disposed of directly. Fixing. Fixed.

@andersio andersio force-pushed the producer-lift branch 2 times, most recently from 381d1ce to a6c212c Compare December 3, 2016 21:30
@mdiep
Copy link
Contributor

mdiep commented Dec 7, 2016

The idea here makes sense: why create more signals than are needed? But I find the implementation here to be difficult to follow. I think we need to try to clarify/simplify the code before merging. I'll swing back around to this after we get 1.0 out (or at least until we get 1.0 PRs merged).

@andersio
Copy link
Member Author

andersio commented Dec 7, 2016

Not in a hurry. Gotta think of a better explanation to all this. ☕

@andersio
Copy link
Member Author

andersio commented Dec 7, 2016

I think we need to try to clarify/simplify the code before merging.

I do hope to figure out a way to unify both "modes" to burn down the giant switch in the binary lift. But I have my hands busy on other stuff right now. 🐝

@andersio
Copy link
Member Author

andersio commented Dec 7, 2016

Heh, it turns out to be quite straightforward. 🙈

@andersio andersio force-pushed the producer-lift branch 2 times, most recently from a84aab9 to ecf7dc2 Compare April 14, 2017 21:18
@mdiep
Copy link
Contributor

mdiep commented Apr 15, 2017

Trying to get to this. It's on my list 😅

@andersio
Copy link
Member Author

andersio commented May 1, 2017

Note that this addressed #227, since now downstreams are interrupted by upstreams. IOW interrupted would now respect the async operators applied.

@andersio andersio modified the milestone: 2.0 May 5, 2017
@andersio andersio force-pushed the producer-lift branch 2 times, most recently from 375869e to 3036e50 Compare May 29, 2017 10:17
/// attached.
/// Even if multiple instances of work from the same `SignalProducer` are executing
/// concurrently, produced `Signal`s would see only events from its own instance of work,
/// but not from the others.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the language was clearer before. Specifically this bit:

where each invocation of start() will create a new underlying operation.

return self.init { _ in return }
return self.init { observer, lifetime in
lifetime.observeEnded { _ = observer }
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this needs a merge from master?

/// performing any of its own post-creation side effect.
fileprivate struct Builder {
fileprivate let make: () -> (signal: Signal<Value, Error>, didCreate: () -> Void, interruptHandle: Disposable)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this would be clearer as a typealias.

typealias Builder = () -> (signal: Signal<Value, Error>, didCreate: () -> Void, interruptHandle: Disposable)

Or maybe a struct instead of a tuple for the return value:

struct Instance {
  var signal: Signal<Value, Error>
  var didCreate: () -> Void
  var interruptHandle: Disposable
}
let builder: () -> Instance

lifetime.observeEnded(innerDisposable.dispose)
transform(signal).observe(observer)
}
return SignalProducer<U, F>(SignalProducer<U, F>.Builder {
Copy link
Contributor

Choose a reason for hiding this comment

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

Basically: this could be return SignalProducer<U, F> { ... }.

@mdiep mdiep merged commit 66ae241 into master Jun 2, 2017
@mdiep mdiep deleted the producer-lift branch June 2, 2017 13:19
@mdiep
Copy link
Contributor

mdiep commented Jun 2, 2017

Sorry this one took so long! 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants