Scoping stores, potential performance issue

Related Projects Swift Composable Architecture
,
1

Hello everyone,

I've got a question regarding the scoping in stores. I've noticed that the mapping code (view computed var) between state & view state get called a lot with the same value.
For instance, in the TicTacToe sample application, if I add a print in the TwoFactorState mapping like so :

extension TwoFactorState {
  var view: TwoFactorViewController.ViewState {
    print("Mapping to viewState : \(self)")
    return .init(
      alert: self.alert,
      code: self.code,
      isActivityIndicatorHidden: !self.isTwoFactorRequestInFlight,
      isLoginButtonEnabled: self.isFormValid && !self.isTwoFactorRequestInFlight
    )
  }
}

I get the following 6 lines in console output each time I enter a number in the code field (here I entered 1):

Mapping to viewState : TwoFactorState(alert: nil, code: "1", isFormValid: false, isTwoFactorRequestInFlight: false, token: "deadbeef")
Mapping to viewState : TwoFactorState(alert: nil, code: "1", isFormValid: false, isTwoFactorRequestInFlight: false, token: "deadbeef")
Mapping to viewState : TwoFactorState(alert: nil, code: "1", isFormValid: false, isTwoFactorRequestInFlight: false, token: "deadbeef")
Mapping to viewState : TwoFactorState(alert: nil, code: "1", isFormValid: false, isTwoFactorRequestInFlight: false, token: "deadbeef")
Mapping to viewState : TwoFactorState(alert: nil, code: "1", isFormValid: false, isTwoFactorRequestInFlight: false, token: "deadbeef")
Mapping to viewState : TwoFactorState(alert: nil, code: "1", isFormValid: false, isTwoFactorRequestInFlight: false, token: "deadbeef")

I'm a bit afraid that for some less trivial examples with deep nested states, this could impact performance. Also I know it's one of the purposes of ViewStore to filter duplicates, but I'm wondering if it would be a good idea to add a scope method to Store that takes into account the fact that most states are equatable like so :

    public func scope<LocalState: Equatable, LocalAction>(
      state toLocalState: @escaping (State) -> LocalState,
      action fromLocalAction: @escaping (LocalAction) -> Action
    ) -> Store<LocalState, LocalAction> {
      let localStore = Store<LocalState, LocalAction>(
        initialState: toLocalState(self.state.value),
        reducer: { localState, localAction in
          self.send(fromLocalAction(localAction))
          localState = toLocalState(self.state.value)
          return .none
        }
      )
      localStore.parentCancellable = self.state
        .sink { [weak localStore] newValue in
          print("Optimized scoping")
            let newState = toLocalState(newValue)
            guard newState != localStore?.state.value else {
                print("Duplicate")
                return
            }
          localStore?.state.value = newState
      }
      return localStore
    }

This way we would avoid spreading the same value to the sub-stores multiple times.
I would love to know your point of view on this :slightly_smiling_face:

Thanks for reading !

1 Like
2

Hey @bioche, this would definitely be a good thing to fix! We even have a test right now that shows it's being called too much, and we'd love to get those numbers down.

The equatable-scope idea is interesting. My only worry would be slowing down compile times with the overload, but I haven't played with it at all yet. There's also a chance we solve this by restructuring how the store is represented internally, but we haven't had time looking into this yet. SwiftUI seems to have an efficient way of de-duping state, even non-equatable things, using memcpr or something, but I have no idea how that works or if someone has replicated this yet.

4 Likes
3

Thank you for your answer @mbrandonw !
I just tested the compile times as you suggested and found very few difference between the store with or without the overload. Here are the compile times of the ComposableArchitecture framework in seconds :

With overload :
2.243
2.227
2.260
2.212
2.222
2.215

Without overload :
2.176
2.216
2.197
2.220
2.206
2.318

Edit : Same test for TicTacToe sample app

With overload :
8.07
8.17
8.09
8.16

without overload :
8.17
8.23
8.27
8.18

Should I open a pull request with this overload or do you prefer looking into the store restructuration or memcpr method which would be quite interesting to see ? ^^

4

We could also use the Combine Operator on the stream removeDuplicates(by:).

localStore.parentCancellable = self.state
        .map(toLocalState)
        .removeDuplicates(by: { lhs, rhs in lhs == rhs})  // or .removeDuplicates(), if State: Equatable
        .sink { [weak localStore] newValue in 
          localStore?.state.value = newState
        }

This however would require State to implement the == operator. We could achieve this by restricting State in the Store definition to be : Equatable. Unfortunately, tuples cannot conform the Equatable or any other protocol. If we would restrict our stores to only work with equatable states, we could no longer use tuples as State. Alternatively, we could add an extension that would remove duplicates only for states that are equatable (i.e. extension Store where State: Equatable). This would mean that we would still have the duplicated downstream updates for all that are wrapping a tuples, but it would be solved for stores wrapping equatable structs.

EDIT: Missed the part <LocalState: Equatable> in the function declaration. My point is then just: we could use removeDuplicates instead of adding our own code in the sync. :slight_smile:

5

Exactly it's just an overload for the Equatable case.
As for the removeDuplicates, even if it is more elegant, it's actually not exactly the same as the "if" in the sink, because with the first option we ignore the duplicates in the parent state where with the second one, we ignore the duplicates in the local state (the local state is actually set both in the reducer and in the sink and I wouldn't want to miss this duplication as well ;) )

6

As we are first mapping toLocalState and then removing duplicates, we would be ignoring duplicates in the local state and not the global state. :)

By the way, why exactly do we set the local state in the reducer? Wouldn’t it be enough to set the state in the sink?

7

Based on @mbrandonw's idea, I went ahead and tried using memcmp in the non-equatable case. You can check the commit here:

However, I'm not 100% satisfied with this solution, as it requires MemCmp to be sprinkled throughout the store implementation. Additionally, it requires us to allocate copies of our new proposed state and the current state which we compare. I haven't done a lot of UnsafePointer magic, so maybe there is room for improvement.

Additionally, I added a test that the state publisher does not publish a new value if it is a duplicate.