-
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
Introduce FlattenStrategy.concurrent
.
#298
Conversation
ec30c09
to
b1cf473
Compare
b1cf473
to
23f0b40
Compare
Sources/Flatten.swift
Outdated
@@ -389,10 +395,11 @@ extension SignalProtocol where Value: SignalProducerProtocol, Error == Value.Err | |||
} | |||
} | |||
} | |||
state.modify { $0.isStarting = false } | |||
|
|||
isStarting = 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.
Is assigning to a Bool
an atomic operation? This write could happen concurrently with the read above. I think it needs to at least be in its own Atomic
.
I'm not sure that makes sense. Maybe we just add a precondition that count must be >0? |
Sources/Flatten.swift
Outdated
$0.active = nil | ||
return !$0.isStarting | ||
handle.remove() | ||
_ = cleanup |
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.
Is this enough to guarantee that producerState
remains initialized until after both the read and the write?
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.
Yes. Its lifetime is limited by the closure or the while
loop body scope, whichever is longer.
Do we have a way to test for regressions in performance/memory in |
Looking at it, I'm not sure why this would cause any performance issues. The code is basically the same. But I'm sure @andersio could run some benchmarks. |
It looks like those tests don't compile on Linux. |
Sources/Flatten.swift
Outdated
$0.active = nil | ||
return !$0.isStarting | ||
} | ||
withExtendedLifetime(deinitializer) { |
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 probably surround just the producerState.setStarted()
below?
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.
True,
Implements #297.
concat
andmerge
are now aliases ofconcurrent(limit: 1)
andconcurrent(limit: .max)
, respectively. The iterative process to start producers is preserved.WIP: