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

Cutting the SignalProducer overhead further. #487

Merged
merged 14 commits into from
Aug 21, 2017
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,11 +1,15 @@
# master
*Please add new entries at the top.*

1. The `SignalProducer` internals have undergone a significant refactoring, which bootstraps the effort to reduce the overhead of constant producers and producer compositions. (#487, kudos to @andersio)

# 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)
Copy link
Contributor

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. 🙈


# 2.0.0
# 2.0.0-rc.3
1. `Lifetime.+=` which ties a `Disposable` to a `Lifetime`, is now part of the public API and is no longer deprecated.
Expand Down
6 changes: 2 additions & 4 deletions Sources/Atomic.swift
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,7 @@ internal struct UnsafeAtomicState<State: RawRepresentable> where State.RawValue
/// - returns: `true` if the current state matches the expected state.
/// `false` otherwise.
internal func `is`(_ expected: State) -> Bool {
return OSAtomicCompareAndSwap32Barrier(expected.rawValue,
expected.rawValue,
value)
return expected.rawValue == value.pointee
}

/// Try to transition from the expected current state to the specified next
Expand Down Expand Up @@ -82,7 +80,7 @@ internal struct UnsafeAtomicState<State: RawRepresentable> where State.RawValue
/// - returns: `true` if the current state matches the expected state.
/// `false` otherwise.
internal func `is`(_ expected: State) -> Bool {
return value.modify { $0 == expected.rawValue }
return value.value == expected.rawValue
}

/// Try to transition from the expected current state to the specified next
Expand Down
25 changes: 25 additions & 0 deletions Sources/Disposable.swift
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,31 @@ extension UnsafeAtomicState where State == DisposableState {
}
}

/// A disposable that does not have side effect upon disposal.
internal final class _SimpleDisposable: Disposable {
private let state = UnsafeAtomicState<DisposableState>(.active)

var isDisposed: Bool {
return state.is(.disposed)
}

func dispose() {
_ = state.tryDispose()
}

deinit {
state.deinitialize()
}
}

/// A disposable that has already been disposed.
internal final class NopDisposable: Disposable {
static let shared = NopDisposable()
var isDisposed = true
func dispose() {}
private init() {}
}
Copy link
Contributor

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.

Copy link
Member Author

@andersio andersio Aug 14, 2017

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.

Copy link
Contributor

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.

Copy link
Member Author

@andersio andersio Aug 14, 2017

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.


/// A type-erased disposable that forwards operations to an underlying disposable.
public final class AnyDisposable: Disposable {
private final class ActionDisposable: Disposable {
Expand Down
86 changes: 86 additions & 0 deletions Sources/Event.swift
Original file line number Diff line number Diff line change
Expand Up @@ -178,3 +178,89 @@ extension Signal.Event: EventProtocol {
return self
}
}

extension Signal.Event {
internal static func filter(_ isIncluded: @escaping (Value) -> Bool) -> (@escaping Signal<Value, Error>.Observer.Action) -> (Signal<Value, Error>.Event) -> Void {
return { action in
return { event in
switch event {
case let .value(value):
if isIncluded(value) {
action(.value(value))
}

case .completed:
action(.completed)

case let .failed(error):
action(.failed(error))

case .interrupted:
action(.interrupted)
}
}
}
}

internal static func filterMap<U>(_ transform: @escaping (Value) -> U?) -> (@escaping Signal<U, Error>.Observer.Action) -> (Signal<Value, Error>.Event) -> Void {
return { action in
return { event in
switch event {
case let .value(value):
if let newValue = transform(value) {
action(.value(newValue))
}

case .completed:
action(.completed)

case let .failed(error):
action(.failed(error))

case .interrupted:
action(.interrupted)
}
}
}
}

internal static func map<U>(_ transform: @escaping (Value) -> U) -> (@escaping Signal<U, Error>.Observer.Action) -> (Signal<Value, Error>.Event) -> Void {
return { action in
return { event in
switch event {
case let .value(value):
action(.value(transform(value)))

case .completed:
action(.completed)

case let .failed(error):
action(.failed(error))

case .interrupted:
action(.interrupted)
}
}
}
}

internal static func mapError<E>(_ transform: @escaping (Error) -> E) -> (@escaping Signal<Value, E>.Observer.Action) -> (Signal<Value, Error>.Event) -> Void {
return { action in
return { event in
switch event {
case let .value(value):
action(.value(value))

case .completed:
action(.completed)

case let .failed(error):
action(.failed(transform(error)))

case .interrupted:
action(.interrupted)
}
}
}
}
}
20 changes: 20 additions & 0 deletions Sources/Observer.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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?
Copy link
Contributor

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. 😕

Copy link
Member Author

@andersio andersio Aug 15, 2017

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) }


/// An initializer that transforms the action of the given observer with the
/// given transform.
///
/// If the given observer would perform side effect on deinitialization, the
/// created observer would retain it.
///
/// - parameters:
/// - observer: The observer to transform.
/// - transform: The transform.
internal init<U, E: Swift.Error>(_ observer: Signal<U, E>.Observer, _ transform: @escaping (@escaping Signal<U, E>.Observer.Action) -> Action) {
self.action = transform(observer.action)
self.wrapped = observer.interruptsOnDeinit ? observer : nil
self.interruptsOnDeinit = false
}

/// An initializer that accepts a closure accepting an event for the
/// observer.
///
Expand All @@ -27,6 +45,7 @@ extension Signal {
/// event as it deinitializes. `false` otherwise.
internal init(action: @escaping Action, interruptsOnDeinit: Bool) {
self.action = action
self.wrapped = nil
self.interruptsOnDeinit = interruptsOnDeinit
}

Expand All @@ -37,6 +56,7 @@ extension Signal {
/// - action: A closure to lift over received event.
public init(_ action: @escaping Action) {
self.action = action
self.wrapped = nil
self.interruptsOnDeinit = false
}

Expand Down
58 changes: 20 additions & 38 deletions Sources/Signal.swift
Original file line number Diff line number Diff line change
Expand Up @@ -527,6 +527,22 @@ extension Signal where Error == NoError {
}

extension Signal {
/// Perform an action upon every event from `self`. The action may generate zero or
/// more events.
///
/// - precondition: The action must be synchronous.
///
/// - parameters:
/// - transform: A closure that creates the said action from the given event
/// closure.
///
/// - returns: A signal that forwards events yielded by the action.
internal func flatMapEvent<U, E>(_ transform: @escaping (@escaping Signal<U, E>.Observer.Action) -> (Event) -> Void) -> Signal<U, E> {
return Signal<U, E> { observer in
return self.observe(.init(observer, transform))
}
}

/// Map each value in the signal to a new value.
///
/// - parameters:
Expand All @@ -535,11 +551,7 @@ extension Signal {
///
/// - returns: A signal that will send new values.
public func map<U>(_ transform: @escaping (Value) -> U) -> Signal<U, Error> {
return Signal<U, Error> { observer in
return self.observe { event in
observer.action(event.map(transform))
}
}
return flatMapEvent(Signal.Event.map(transform))
}

#if swift(>=3.2)
Expand All @@ -562,11 +574,7 @@ extension Signal {
///
/// - returns: A signal that will send new type of errors.
public func mapError<F>(_ transform: @escaping (Error) -> F) -> Signal<Value, F> {
return Signal<Value, F> { observer in
return self.observe { event in
observer.action(event.mapError(transform))
}
}
return flatMapEvent(Signal.Event.mapError(transform))
}

/// Maps each value in the signal to a new value, lazily evaluating the
Expand Down Expand Up @@ -599,18 +607,7 @@ extension Signal {
///
/// - returns: A signal that forwards the values passing the given closure.
public func filter(_ isIncluded: @escaping (Value) -> Bool) -> Signal<Value, Error> {
return Signal { observer in
return self.observe { (event: Event) -> Void in
guard let value = event.value else {
observer.action(event)
return
}

if isIncluded(value) {
observer.send(value: value)
}
}
}
return flatMapEvent(Signal.Event.filter(isIncluded))
}

/// Applies `transform` to values from `signal` and forwards values with non `nil` results unwrapped.
Expand All @@ -620,22 +617,7 @@ extension Signal {
///
/// - returns: A signal that will send new values, that are non `nil` after the transformation.
public func filterMap<U>(_ transform: @escaping (Value) -> U?) -> Signal<U, Error> {
return Signal<U, Error> { observer in
return self.observe { (event: Event) -> Void in
switch event {
case let .value(value):
if let mapped = transform(value) {
observer.send(value: mapped)
}
case let .failed(error):
observer.send(error: error)
case .completed:
observer.sendCompleted()
case .interrupted:
observer.sendInterrupted()
}
}
}
return flatMapEvent(Signal.Event.filterMap(transform))
}
}

Expand Down
Loading