-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Remove subscriptions from CurrentValueRelay
when cancelled
#2699
base: main
Are you sure you want to change the base?
Conversation
Subscription keeps strong reference of `CurrentValueRelay` similar to `CurrentValueSubject`
@mbrandonw @stephencelis any chance you could kick off CI for this one? This branch has some benchmarks for comparing the new implementation with the old one as well as I've included some results below and used diff to make it a bit easier to distinguish between the old and new implementation. name (subscribers) time std iterations
-------------------------------------------------------------------------------
CurrentValueSubject.subscribe (1000) 4075917.000 ns ± 1.40 % 343
+CurrentValueRelay.subscribe (1000) 443083.000 ns ± 1.68 % 3173
-CurrentValueRelayOld.subscribe (1000) 1217375.000 ns ± 1.83 % 1142 Let me know if you want any benchmarks included in this PR. Benchmark ResultsAdding subscribers name (subscribers) time std iterations
-------------------------------------------------------------------------------
CurrentValueSubject.subscribe (1) 667.000 ns ± 20.18 % 1000000
+CurrentValueRelay.subscribe (1) 583.000 ns ± 21.47 % 1000000
-CurrentValueRelayOld.subscribe (1) 1375.000 ns ± 11.04 % 1000000
CurrentValueSubject.subscribe (10) 9000.000 ns ± 5.49 % 155337
+CurrentValueRelay.subscribe (10) 5125.000 ns ± 7.64 % 271308
-CurrentValueRelayOld.subscribe (10) 12833.000 ns ± 4.52 % 107162
CurrentValueSubject.subscribe (100) 115625.000 ns ± 2.62 % 11990
+CurrentValueRelay.subscribe (100) 45042.000 ns ± 2.95 % 30879
-CurrentValueRelayOld.subscribe (100) 121750.000 ns ± 2.21 % 11436
CurrentValueSubject.subscribe (1000) 4075917.000 ns ± 1.40 % 343
+CurrentValueRelay.subscribe (1000) 443083.000 ns ± 1.68 % 3173
-CurrentValueRelayOld.subscribe (1000) 1217375.000 ns ± 1.83 % 1142 Removing subscribers name (subscribers) time std iterations
-------------------------------------------------------------------------------
CurrentValueSubject.cancel (1) 708.000 ns ± 21.75 % 1000000
+CurrentValueRelay.cancel (1) 750.000 ns ± 37.44 % 1000000
-CurrentValueRelayOld.cancel (1) 791.000 ns ± 16.69 % 1000000
CurrentValueSubject.cancel (10) 9416.000 ns ± 6.71 % 148132
+CurrentValueRelay.cancel (10) 7250.000 ns ± 7.56 % 191997
-CurrentValueRelayOld.cancel (10) 7208.000 ns ± 6.76 % 195131
CurrentValueSubject.cancel (100) 122959.000 ns ± 1.93 % 11280
+CurrentValueRelay.cancel (100) 70500.000 ns ± 2.63 % 19770
-CurrentValueRelayOld.cancel (100) 69417.000 ns ± 2.95 % 20100
CurrentValueSubject.cancel (1000) 4186750.000 ns ± 1.16 % 333
+CurrentValueRelay.cancel (1000) 757916.000 ns ± 1.52 % 1826
-CurrentValueRelayOld.cancel (1000) 712333.000 ns ± 8.65 % 1967 Sending to subscribers name (subscribers) time std iterations
-------------------------------------------------------------------------------
CurrentValueSubject.send × 1 (1) 167.000 ns ± 28.42 % 1000000
+CurrentValueRelay.send × 1 (1) 167.000 ns ± 42.38 % 1000000
-CurrentValueRelayOld.send × 1 (1) 291.000 ns ± 26.13 % 1000000
CurrentValueSubject.send × 10 (1) 1333.000 ns ± 9.51 % 1000000
+CurrentValueRelay.send × 10 (1) 1333.000 ns ± 10.44 % 1000000
-CurrentValueRelayOld.send × 10 (1) 2292.000 ns ± 9.58 % 601341
CurrentValueSubject.send × 100 (1) 11583.000 ns ± 4.84 % 122239
+CurrentValueRelay.send × 100 (1) 11417.000 ns ± 5.10 % 122059
-CurrentValueRelayOld.send × 100 (1) 21084.000 ns ± 3.29 % 65950
CurrentValueSubject.send × 1000 (1) 109834.000 ns ± 3.37 % 12704
+CurrentValueRelay.send × 1000 (1) 109833.000 ns ± 2.14 % 12676
-CurrentValueRelayOld.send × 1000 (1) 206417.000 ns ± 2.39 % 6672
CurrentValueSubject.send × 1 (10) 1167.000 ns ± 14.53 % 1000000
+CurrentValueRelay.send × 1 (10) 917.000 ns ± 17.16 % 1000000
-CurrentValueRelayOld.send × 1 (10) 1625.000 ns ± 13.78 % 855844
CurrentValueSubject.send × 10 (10) 9958.000 ns ± 6.47 % 140486
+CurrentValueRelay.send × 10 (10) 7542.000 ns ± 4.75 % 185173
-CurrentValueRelayOld.send × 10 (10) 14000.000 ns ± 3.46 % 99597
CurrentValueSubject.send × 100 (10) 94083.000 ns ± 2.05 % 14787
+CurrentValueRelay.send × 100 (10) 71208.000 ns ± 2.43 % 19518
-CurrentValueRelayOld.send × 100 (10) 135500.000 ns ± 1.62 % 10283
CurrentValueSubject.send × 1000 (10) 949958.000 ns ± 1.47 % 1490
+CurrentValueRelay.send × 1000 (10) 707666.000 ns ± 1.06 % 1974
-CurrentValueRelayOld.send × 1000 (10) 1354458.000 ns ± 1.16 % 1031
CurrentValueSubject.send × 1 (100) 8625.000 ns ± 6.86 % 161405
+CurrentValueRelay.send × 1 (100) 7084.000 ns ± 6.64 % 196241
-CurrentValueRelayOld.send × 1 (100) 13209.000 ns ± 6.32 % 101701
CurrentValueSubject.send × 10 (100) 81750.000 ns ± 2.61 % 16869
+CurrentValueRelay.send × 10 (100) 66875.000 ns ± 2.33 % 20838
-CurrentValueRelayOld.send × 10 (100) 127667.000 ns ± 1.96 % 10916
CurrentValueSubject.send × 100 (100) 812375.000 ns ± 1.28 % 1716
+CurrentValueRelay.send × 100 (100) 664250.000 ns ± 1.68 % 2098
-CurrentValueRelayOld.send × 100 (100) 1272083.000 ns ± 1.39 % 1098
CurrentValueSubject.send × 1000 (100) 8149333.500 ns ± 1.07 % 170
+CurrentValueRelay.send × 1000 (100) 6658312.500 ns ± 0.93 % 208
-CurrentValueRelayOld.send × 1000 (100) 12847583.500 ns ± 0.80 % 108
CurrentValueSubject.send × 1 (1000) 82083.000 ns ± 8.07 % 17030
+CurrentValueRelay.send × 1 (1000) 66666.000 ns ± 2.58 % 20907
-CurrentValueRelayOld.send × 1 (1000) 130291.000 ns ± 2.73 % 10617
CurrentValueSubject.send × 10 (1000) 813416.000 ns ± 1.45 % 1725
+CurrentValueRelay.send × 10 (1000) 660583.000 ns ± 1.28 % 2111
-CurrentValueRelayOld.send × 10 (1000) 1291417.000 ns ± 0.89 % 1083
CurrentValueSubject.send × 100 (1000) 8068458.000 ns ± 1.23 % 173
+CurrentValueRelay.send × 100 (1000) 6590250.000 ns ± 0.55 % 209
-CurrentValueRelayOld.send × 100 (1000) 12942270.500 ns ± 0.54 % 108
CurrentValueSubject.send × 1000 (1000) 81221209.000 ns ± 1.31 % 17
+CurrentValueRelay.send × 1000 (1000) 66941417.000 ns ± 0.23 % 21
-CurrentValueRelayOld.send × 1000 (1000) 129341166.000 ns ± 0.68 % 11 |
} | ||
|
||
func request(_ demand: Subscribers.Demand) { | ||
_ = demandBuffer?.demand(demand) | ||
precondition(demand > 0, "Demand must be greater than zero") |
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 breaking change, however, it reflects the behaviour of CurrentValueSubject
. Also, I think requesting zero demand would be considered a programmer error.
self.lock.lock() | ||
self.demand += demand | ||
|
||
guard | ||
!self.receivedLastValue, | ||
let value = self.upstream?.currentValue | ||
else { | ||
self.lock.unlock() | ||
return | ||
} | ||
|
||
self.receivedLastValue = true | ||
|
||
switch self.demand { | ||
case .unlimited: | ||
self.lock.unlock() | ||
// NB: Adding to unlimited demand has no effect and can be ignored. | ||
_ = downstream.receive(value) | ||
|
||
default: | ||
self.demand -= 1 | ||
self.lock.unlock() | ||
let moreDemand = downstream.receive(value) | ||
self.lock.lock() | ||
self.demand += moreDemand | ||
self.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.
I'm not super happy with the locking dance but this implementation seemed to run best for me.
From my understanding the key points are:
- mutating
demand
andreceivedLastValue
needs to be locked. - as soon as
demand
reachesunlimited
,receive(_:)
will always calldownstream.receive(value)
Previously, DemandBuffer
would handle most of this, however, it also had the potential to buffer all upstream values. In practice, this probably never happened, it would really depend on the downstream subscriber. The new behaviour only sends the current value if it hasn't already been received. This appears to be the same as CurrentValueSubject
.
CurrentValueRelay
does not remove cancelled subscriptions. Previously this was not an issue, it was only used byViewStore
and had a short lifetime. It is now used byRootStore
allowing the subscriptions to grow indefinitely.This PR adds logic to remove subscriptions when they are cancelled.
I'm unsure if we did this intentionally for performance or if it was an oversight. I remember we specifically didn't include locking because we could reasonably assume interactions happened on the main thread. I have a feeling this assumption is no longer true which is why I've included locking around adding and removing subscriptions.
The subscription also now holds a strong reference to the
CurrentValueRelay
. This appears to be the same behaviour asCurrentValueSubject
.The tests are somewhat indirect, however, I opted not to use
@_spi(Internals)
to keep things simple.