Remove subscriptions from CurrentValueRelay when cancelled
#2699
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 % 1142Let 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 % 1142Removing 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 % 1967Sending 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 |
iampatbrown
commented
Jan 13, 2024
| } | ||
|
|
||
| func request(_ demand: Subscribers.Demand) { | ||
| _ = demandBuffer?.demand(demand) | ||
| precondition(demand > 0, "Demand must be greater than zero") |
There was a problem hiding this comment.
This is a breaking change, however, it reflects the behaviour of CurrentValueSubject. Also, I think requesting zero demand would be considered a programmer error.
iampatbrown
commented
Jan 13, 2024
Comment on lines
+109
to
+135
| 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.
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
demandandreceivedLastValueneeds to be locked. - as soon as
demandreachesunlimited,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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
CurrentValueRelaydoes not remove cancelled subscriptions. Previously this was not an issue, it was only used byViewStoreand had a short lifetime. It is now used byRootStoreallowing 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.