-
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
Signal
lock elision.
#129
Signal
lock elision.
#129
Conversation
Sources/Signal.swift
Outdated
func sendLockLock() { | ||
if let lock = sendLock { | ||
lock.lock() | ||
} |
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 could be shortened to lock?.lock()
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 a pretty reasonable idea, and it's not a big change.
Are there any ramifications for existing code that might start signals as a side effect in on()
or some other operator? What would happen to recursive event sending that currently results in a deadlock — anything?
Sources/Signal.swift
Outdated
if let lock = sendLock { | ||
return lock.try() | ||
} | ||
return true |
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 could be shortened to return lock?.try() ?? true
Sources/Signal.swift
Outdated
func sendLockUnlock() { | ||
if let lock = sendLock { | ||
lock.unlock() | ||
} |
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 could be shortened to lock?.unlock()
Sources/Signal.swift
Outdated
|
||
let observer = Observer { [weak self] event in | ||
guard let signal = self else { | ||
return | ||
} | ||
|
||
@inline(__always) | ||
func sendLockLock() { |
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.
What do you think about just naming this lock()
?
Sources/Signal.swift
Outdated
} | ||
|
||
@inline(__always) | ||
func sendLockTryLock() -> 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.
What do you think about just naming this tryLock()
?
Sources/Signal.swift
Outdated
} | ||
|
||
@inline(__always) | ||
func sendLockUnlock() { |
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.
What do you think about just naming this unlock()
?
Sources/Signal.swift
Outdated
/// - disposable: An optional disposable to associate with the signal, and | ||
/// to be disposed of when the signal terminates. | ||
/// | ||
/// - returns: A tuple made of signal and observer. | ||
public static func pipe(disposable: Disposable? = nil) -> (Signal, Observer) { | ||
public static func pipe(_ concurrency: SignalConcurrency = .serialized, disposable: Disposable? = nil) -> (Signal, 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.
I think this should be a named parameter.
Sources/Signal.swift
Outdated
/// - generator: A closure that accepts an implicitly created observer | ||
/// that will act as an event emitter for the signal. | ||
public init(_ generator: (Observer) -> Disposable?) { | ||
public init(_ concurrency: SignalConcurrency = .serialized, _ generator: (Observer) -> Disposable?) { |
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 a named parameter.
Sources/Signal.swift
Outdated
/// The `Signal` may assume the events emitted to it have already been | ||
/// serialized. In other words, the event emitter of the `Signal` is assumed | ||
/// to only be called in synchronized routines. | ||
case contained |
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 not sure about the naming — how about this?
public enum SignalSerialization {
case mutex
case inherit
}
Signal(serialization: .mutex) { observer in
// ...
}
Signal.pipe(serialization: .inherit)
Sources/Signal.swift
Outdated
@@ -187,13 +219,16 @@ public final class Signal<Value, Error: Swift.Error> { | |||
/// retained. | |||
/// | |||
/// - parameters: | |||
/// - contained: Indicates if the event emitter passed to `generator` would |
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 comment will need to be updated when the name is finalised.
Sources/Signal.swift
Outdated
@@ -35,9 +35,11 @@ public final class Signal<Value, Error: Swift.Error> { | |||
/// Signal itself will remain alive until the observer is released. | |||
/// | |||
/// - parameters: | |||
/// - concurrency: The concurrency mode the signal should assume. |
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 comment will need to be updated too.
If you mean starting producers, I don't see it would cause harm. Edit: Recursive event sending was not allowed since the very beginning. So other than this, the side effects are still executing within the same giant protected section, in the very same order, unless If you mean observing any
Obviously, the unlocked mode would grant a hole for recursive event sending. But our stance should remain unchanged. As for recursive Edit: It is a different story to have |
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.
Before I look deeper I wanted to say that I'm 👎 for making this part of the public API. I'm okay if we use it as an optimization internally, but I think the increase in complexity of the API wouldn't be justified.
@NachoSoto Has to be part of the public API, or otherwise RAC can't use Edit: Having said that, RAC APIs may still enjoy the benefits as users apply more and more operators. But it feels sad (?) to me not to have RAC covered too. Edit 2: There is a middle ground though: obscurity. We can export this API to |
A note on the recursive AFAIU, it is made for
Edit: Updated the ASCII diagrams. The unlocks are misplaced. 🙈 |
Sources/Signal.swift
Outdated
|
||
/// The `Signal` assumes to have inherited the serialization from its owner, | ||
/// and its event emitter would not be called concurrently. | ||
case inherited |
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.
FWIW, I still think this case would read better as an imperative: Signal(serialization: .inherit)
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.
Followed FlattenStrategy
to use imperative verbs.
Sources/FoundationExtensions.swift
Outdated
@@ -29,7 +29,7 @@ extension Reactive where Base: NotificationCenter { | |||
/// - note: The signal does not terminate naturally. Observers must be | |||
/// explicitly disposed to avoid leaks. | |||
public func notifications(forName name: Notification.Name?, object: AnyObject? = nil) -> Signal<Notification, NoError> { | |||
return Signal { [base = self.base] observer in | |||
return Signal(strategy: .inherit) { [base = self.base] observer in |
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 we've lost too much meaning now at the point of use. It's no longer clear what's being inherited.
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.
Does inputStrategy
or serializeStrategy
make sense to you?
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'd say serializationStrategy
.
However, I'm still partial to serialization: .default
and serialization: .inherit
. What do you think?
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.
...with declaration: init(serialization strategy: SerializationStrategy, ...)
.
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.
Sometimes I do wonder if FlattenStrategy
should be FlatteningStrategy
instead...
fa79195
to
c44539b
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.
If it can't be private then like I said I don't think the benefits warrant the increase in complexity. This is exposing implementation details that most users won't care about 👎
c44539b
to
25123dd
Compare
@NachoSoto Maybe I should have phrased it better: It can for sure be private to ReactiveSwift. I just want to figure out a middle ground for RAC if it makes sense. If "internal" through obscurity (like stdlib) is still considered exposing the implementation, there is probably no other way to expose this to RAC based on my understanding. Edit: For example: public enum __WhenWeDoNotHaveSubmodulesThisHappens {
public static func __makeInheritlySerializedSignal<Value, Error: Swift.Error>(
_ generator: @escaping (Observer<Value, Error>) -> Disposable?
) -> Signal<Value, Error> {
return ReactiveSwift.Signal(serialization: .inherit, generator)
}
} |
05c3796
to
035d46c
Compare
@NachoSoto Hmm, I actually thought of a counter argument to keeping it private: while most users would not care, users - including depending libraries like ReactiveCocoa - who want custom operators or have all possible means to optimize should be given the tools. If what you worry is having it |
Benchmark Source TableView Scrolling Simulation Send 10000 values to 1 observer. Send 10000 values to 8 observers. Send 10000 values to 1 produced signal. Send 10000 values to 8 produced signals. |
1334b0d
to
b12d6bc
Compare
Blocked by #137. |
fde92fd
to
24f35b2
Compare
24f35b2
to
11046d4
Compare
The
|
Rolled back to be an internal API, and rebased on |
Test body let property = MutableProperty((0, 0, 0, 0))
let composed = { property.map { ($0.1, $0.3) }.combinePrevious((-1, -1)) }()
//let (signal, observer) = Signal<(Int, Int, Int, Int), NoError>.pipe()
//let composed = signal.map { ($0.1, $0.3) }.combinePrevious((-1, -1))
withExtendedLifetime(composed) {
_measureAndStart(times: 1_000_000, label: label) {
let value = 1
//observer.send(value: (value, value, value, value))
property.value = (value, value, value, value)
}
} Result
The locking overhead is around 5-7% per observer call-out. For the narrower gap in properties, it is likely due to not having lifting optimization (#140) in place, which elides the serialising, relaying |
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 not very excited about this. The thread safety is already hard to reason about without adding an additional level of complexity. I'm not eager to make it any more complex, especially for modest performance gains.
Sources/Signal.swift
Outdated
fileprivate init(_ generator: (Observer) -> Disposable?) { | ||
fileprivate init(_ attributes: SignalAttributes, _ generator: (Observer) -> Disposable?) { | ||
var attributes = attributes | ||
if (Thread.current.threadDictionary["reactiveswift_disable_inherit_serialization"] as! Bool?) ?? false { |
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.
What's this for?
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.
Ah, I forgot to revert this hack for benchmarking purpose. 🤦♂️
Sources/Signal.swift
Outdated
} | ||
|
||
/// Describes how `Signal` should behave. | ||
internal struct SignalAttributes: OptionSet { |
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 an OptionSet
instead of an enum
?
The annotation in this PR is built on top of the premises of the If a synchronous operator derives a |
fff4f32
to
420a3ed
Compare
Closed since this becomes trivial with an upcoming higher-level |
While we advocate for a safe-by-default API like how our beloved Swift does, there are places we may elide the serialization:
Synchronously
Signal
operators. e.g.map
,filter
,scan
,skipNil
.These can rely on the serialization at the ultimate upstream, e.g. signals created by
Signal.init
orSignal.pipe
with default settings.An illustration of how the call stack would look like with
signal.map(t).map(t).map(t).map(t)...
:Asynchronous
Signal
operators which deliver all kinds of events on the sameScheduler
.For example,
observe(on:)
forwards all the events to the suppliedScheduler
. Since the scheduler serialises all the enqueued actions, there is no need for the created signal to serialise the events again.Signal
s that are owned by synchronized entities.For entities that are already synchronised,
Signal
can safely assume events not to be emitted concurrently. For example,MutableProperty
always sends event in its critical section.Implementation
The PR introduces an optional parameter
attributes
toSignal.init
andSignal.pipe
that allows configurability of some of theSignal
behavior.For the purpose of the PR, a
inheritSerialization
option was introduced.Signal
s created with this flag would not attempt to serialise the inputs it received.