Performance issue using Observation in v0.10.1

[mirkobraic]

After upgrading from v0.9.0 to v0.10.1, I am experiencing severe performance issues. The withPerceptionTracking closure is being called much more frequently than expected, causing the app to become unresponsive and laggy.

To ensure that the issue wasn't due to a mistake on my part, I checked the behavior of the example project and found that it is also affected. To test the issue, I compared the v0.9.0 and v0.10.1 example projects. Specifically, I used the FormAppCoordinator tab and placed Self._printChanges() inside every view (Step1View, Step2View, Step3View, and FinalScreen).

This is the result for v0.9.0:

Screen.Recording.2024-07-05.at.19.12.26.mov

And this is the result for v0.10.1

Screen.Recording.2024-07-05.at.19.18.03.mov

We can see that with the latest version of the library, views are being recomputed much more frequently than before.
Note: I added Self._printChanges() inside of WithViewStore and withPerceptionTracking.

I have also profiled both cases to obtain a more detailed performance review. Here is the compressed folder containing the result: profiling.zip.

I attempted to determine the cause of this issue but was unable to find a solution. Any assistance would be greatly appreciated.

[mirkobraic]

I suspect the issue is related to how the new Observation is handled. In this Pointfree episode, Brandon explains how easy it is to observe too much state. Since this library uses @ObservableState screens, which are wrapped in a Route enum and stored in an array of routes, my guess is that any change to the routes array will cause all the views that use any property from that array to be recomputed.

[stephencelis]

Do you have a demo app that we can look at?

For context, TCA provides special logic to avoid observing child state from parents when it comes to IdentifiedArray, StackState, and other types. It does so by overloading the _$isIdentityEqual static function, which the macro calls to.

Here are examples:

https://github.com/pointfreeco/swift-composable-architecture/blob/f660a699d3cb45841bfac3dc7d76957d8ce348f4/Sources/ComposableArchitecture/Observation/ObservableState.swift#L113-L135

Now there is a generalization that should minimize observation of any collection of observable state:

https://github.com/pointfreeco/swift-composable-architecture/blob/f660a699d3cb45841bfac3dc7d76957d8ce348f4/Sources/ComposableArchitecture/Observation/ObservableState.swift#L137-L144

So as long as Route has an @ObservableState annotation I would imagine it to be OK, performance-wise.

If TCA coordinators uses its own data structure for routing, it could also define its own _$isIdentityEqual specialization that further improves things.

[mirkobraic]

I took the example from this repository and just added Self._printChanges() to the views in the Form tab.
Thank you for the provided info! I'll make sure to go over them and try to figure out what exactly is going wrong. I tried manually adding ObservableState conformance to the Route enum, but it didn't fix the issue.

[mirkobraic]

Here are demo apps showcasing the issue.

TCACoordinators v0.9.0, using WithViewStore

We can see that no redundant computations occur when pushing or popping views. When incrementing a value, the whole view is re-computed since we observe the whole state.

Screen.Recording.2024-07-08.at.22.43.57.mov

TCACoordinators v0.10.1, using WithPerceptionTracking

There are several issues in this case:

1. When pushing or popping a view, all views in the stack are re-computed.
2. When incrementing a value, the entire view is re-computed instead of just IntView.

Screen.Recording.2024-07-08.at.22.56.10.mov

Coordinators090Test.zip
Coordinators0101Test.zip

[yurihan]

I am in the same situation. So I'm still keep the previous version.
Even if just one change, all views are re-computed.

[sdkdimon]

I have created a more general sample based on @mirkobraic zip samples. It is a package based project with two targets Observable and Unobservable.
https://github.com/sdkdimon/TCACoordinatorsPerformance.git
Also i discovered that TCACoordinators v0.10.1 can work with Observable & Unobservable state here part of implementation of that feature in TCARouter.swift:

 public var body: some View {
    if Screen.self is ObservableState.Type {
      WithPerceptionTracking {
        Router(
          $store[],
          buildView: { screen, index in
            WithPerceptionTracking {
              screenContent(scopedStore(index: index, screen: screen))
            }
          }
        )
      }
    } else {
      UnobservedTCARouter(store: store, identifier: identifier, screenContent: screenContent)
    }
  }

So we can use latest version of TCACoordinators but without ObservableState in app implementation to avoid performance issues for now.

[mirkobraic]

Great resource, thank you @sdkdimon!
Yes, the library should work correctly without Observation. The cool thing is we can still use @ObservableState within our features; we just need to exclude it from the coordinator's state. I checked out your code and added @ObservableState to the feature reducers in the UnobservableState package. Everything seems to be working correctly, but I'm not entirely sure if I've missed anything or if this approach is suitable for production apps.
Here is a patch that you can apply to your code if you have time, I'd be happy to hear your feedback.

unobservableCoordWithObservableFeatures.diff.zip

[mirkobraic]

It seems to me that the root cause of the issue lies in the FlowStacks library. The problem is that the objects within FlowStacks are not Observable. For instance, here is a snippet from Router struct:

public struct Router<Screen, ScreenView: View, Modifier: ViewModifier>: View {
  @Binding var routes: [Route<Screen>]
  @ViewBuilder var buildView: (Binding<Screen>, Int) -> ScreenView
  let navigationViewModifier: Modifier
// ...
}

The Router holds an array of our screens, which in our case are @ObservableState objects. Since the Router itself is not observable, it breaks the "observation chain", making the array sensitive for any change in any of the objects it holds.

I think we should update FlowStacks lib with the new observation framework. Here is a link to a slack conversation that I had with @rhysm94. Since it doesn't make sense for FlowStacks to depend on the ComposableArchitecture, it might be better to manually add all of the FlowStacks code directly to the TCACoordinators and adjust it to fit our needs. I've created a patch (TCACoordinatorsFlowStackInPlace.diff.zip) that does this, so you don't have to go through the hassle of copy-pasting a bunch of code. However, I haven't been able to make FlowStacks observable yet. I'll revisit this when I have more time, but any assistance would be much appreciated.

[rhysm94]

FWIW, FlowStacks wouldn’t need to import TCA, but instead Perception.
Still, that might not be ideal for FlowStacks users, and it might be better to inline an Observable/Perceptible version of FlowStacks into TCACoordinators going forward?

[mirkobraic]

That sounds good to me. Given that we don't support the latest version of FlowStacks and probably never will, I don't see a significant downside to this approach. @johnpatrickmorgan, do you have any thoughts on this?

[sdkdimon]

Sounds good, i mean to embed Perception modified FlowStacks into the TCACoordinators. Now i am trying to integrate Perception into the FlowStacks. @Binding var routes: [Route<Screen>] var should be replaced with something with that?

@Perceptible
class ScreenState<Screen> {
  var routes: [Route<Screen>]
  init(routes: [Route<Screen>]) {
    self.routes = routes
  }
}

public struct Router<Screen, ScreenView: View, Modifier: ViewModifier>: View {
 @Perception.Bindable var routes: ScreenState<Screen>

// ..
}

[rhysm94]

Sounds good, i mean to embed Perception modified FlowStacks into the TCACoordinators. Now i am trying to integrate Perception into the FlowStacks. @Binding var routes: [Route<Screen>] var should be replaced with something with that?

@Perceptible
class ScreenState<Screen> {
  var routes: [Route<Screen>]
  init(routes: [Route<Screen>]) {
    self.routes = routes
  }
}

public struct Router<Screen, ScreenView: View, Modifier: ViewModifier>: View {
 @Perception.Bindable var routes: ScreenState<Screen>

// ..
}

No, I’m almost certain that doesn’t need pushing out into a separate class. It should work just fine as a Binding like this.
It’s the Route type which is (likely) the issue here.

[mirkobraic]

@ObservableState macro can easily be added to the Route enum which ensures that it is observed correctly. My guess is that we have to make whole the Router struct observable/perceptible.

Edit: Here is a link to another slack thread on this topic that could be useful.

[sdkdimon]

Ah i see the @ObservableState is already presented in @mirkobraic diff (added to the Route), also i've played with WithPerceptionTracking but with no result. So the [Route<Screen>] var in Router is observable/perceptible out of the box if the Route marked as @ObservableState ?

[mirkobraic]

Ah i see the @ObservableState is already presented in @mirkobraic diff (added to the Route), also i've played with WithPerceptionTracking but with no result. So the [Route<Screen>] var in Router is observable/perceptible out of the box if the Route marked as @ObservableState ?

I think so. But the more I'm looking into this the more questions I have. Since TCARouter and FlowStacks Router are Views, we can exclude them from @ObservableState topic. This suggests that we might be missing an observation mechanism elsewhere.

Edit: The issue could be in IdentifiedArray, but I remember trying with a plain array (which should be observed by default) and it was still not working correctly.

[jshier]

To ignore things in @ObservableState you use @ObservationStateIgnored.

[mirkobraic]

To ignore things in @ObservableState you use @ObservationStateIgnored.

@jshier yes. But currently I think the issue is not related to the Router struct and its @Binding var routes: [Route<Screen>]. I feel the issue is somewhere else, maybe in the way we are storing our array of routes in the coordinators? Or maybe this array is getting modified in a way that is triggering the re-rendering of all screens.

[rhysm94]

It’s still really difficult to tell what’s causing the redraws when the underlying state is not changing.
Might be worth adding a bunch of logs/prints/breakpoints inside the TCARouter type and its helpers to get an idea of where the problem is occurring at the source.

The only thing I can think might be an issue is that Route isn’t ObservableState, but I can’t be remotely sure on that one.

[johnpatrickmorgan]

Thanks everyone for your efforts and insights so far. I agree it would make sense to remove this library's dependency on FlowStacks now. I've played with that approach but haven't yet managed to avoid unnecessarily reinvoking the withPerceptionTracking closure.

[mirkobraic]

Okay, I think I got something. If we expand @ObservableState macro that has been applied to the Route enum, we can see the following code:

extension Route: ComposableArchitecture.ObservableState, Observation.Observable {
  public var _$id: ComposableArchitecture.ObservableStateID {
    switch self {
    case let .push(state):
      return ._$id(for: state)._$tag(0)
    case .sheet:
      return ObservableStateID()._$tag(1)
    case .cover:
      return ObservableStateID()._$tag(2)
    }
  }
  
  public mutating func _$willModify() {
    switch self {
    case var .push(state):
      ComposableArchitecture._$willModify(&state)
      self = .push(state)
    case .sheet:
      break
    case .cover:
      break
    }
  }
}

This is incorrect and requires manual implementation of the extension. Fortunately, we can achieve this without modifying the FlowStacks code, which means we might not need to inline the entire library.
Here is the correct implementation:

extension Route: ComposableArchitecture.ObservableState, Observation.Observable {
    public var _$id: ComposableArchitecture.ObservableStateID {
        switch self {
        case let .push(state):
            return ._$id(for: state)._$tag(0)
        case let .sheet(state, _, _):
            return ._$id(for: state)._$tag(1)
        case let .cover(state, _, _):
            return ._$id(for: state)._$tag(2)
        }
    }

    public mutating func _$willModify() {
        switch self {
        case var .push(state):
            ComposableArchitecture._$willModify(&state)
            self = .push(state)
        case .sheet(var state, let embedInNavigationView, let onDismiss):
            ComposableArchitecture._$willModify(&state)
            self = .sheet(state, embedInNavigationView: embedInNavigationView, onDismiss: onDismiss)
        case .cover(var state, let embedInNavigationView, let onDismiss):
            ComposableArchitecture._$willModify(&state)
            self = .sheet(state, embedInNavigationView: embedInNavigationView, onDismiss: onDismiss)
        }
    }
}

For a test, let's take a look at the example project provided by @sdkdimon. By adding this extension to the ObservableState module, we can see that when incrementing the value on the last screen, only that screen gets re-rendered. Previously, the entire stack would be recomputed. Thanks @rhysm94 for keeping the focus on that enum.

Our next step is to figure out why the whole stack is recomputed on push/pop actions. I'll investigate whether we can fix this by providing custom _$isIdentityEqual functions.

[rhysm94]

Very nice stuff! Just wondering - does that break the non-ObservableState version of TCARouter? If so, what if you do conditional conformance, when Route.Screen is ObservableState?

[rhysm94]

I wonder if the recomputing the whole stack issue is related to this? swiftlang/swift#71122

There’s a Swift forum post about it here, as well, detailing more of the problem. https://forums.swift.org/t/observable-pessimizes-arrays/69508/28

It seems like the initial version of the Observation framework didn’t add a _modify access modifier, which made collection types more heavyweight to work with than necessary, always routing changes via set instead. I suspect this might be part of the issue.

(I could very well be completely wrong and off base here!)

[sdkdimon]

If we try access to the route element inside TCARouter.swift store as following

let route = self.store[1]

the compiler gives an error - Referencing subscript 'subscript(dynamicMember:)' on 'Store' requires that '[Route<Screen>]' conform to 'ObservableState' so our array of routes is not Observable out of the box, so that causing unwanted re render when the navigation stack is changed.
We have an Observable state but when we do store scoping to the routes store here we lose observation of our routes array

@ObservableState
public struct State: Equatable {
  var routes: [Route<Screen.State>]
}

//Here we constructing router with Store that holds non Observable array ('[Route<Screen>]')
 TCARouter(store.scope(state: \.routes, action: \.router)) { screen in 
//....
}

It's just my guess i could be completely wrong here

[sdkdimon]

Guess i got some result. In my playground I've used Router view from FlowStacks directly without TCARouter, and pass an Observable binding of [Route], also inside Router view I wrapped stuff with WithPerceptionTracking view.
As result the whole stack now not recomputed anymore on array changes, but there is an issue with store scoping. So later I will provide more details.

[mirkobraic]

I’ve been quite busy lately and haven't had time to work on this, apologies. We've successfully incorporated v0.10.1 and observation into our project using the extension I shared earlier. However, we’ve noticed an odd issue. When fully replacing the state in the coordinator, the navigation would completely stop working. For some reason the observation got "disconnected". I’ll investigate this tomorrow and share more details with you then.

[sdkdimon]

So here what i got. Let have focus on our previously created sample example project, here is modified AppCoordinatorView:

public struct AppCoordinatorView: View {
  @Perception.Bindable public var store: StoreOf<AppCoordinator>
  
  public init(store: StoreOf<AppCoordinator>) {
    self.store = store
  }
    
  public var body: some View {
//    var storeBindable: _StoreBindable_Perception<AppCoordinator.State, AppCoordinator.Action, [Route<Screen.State>]> = $store.routes
//    var routesBinding: Binding<[Route<Screen.State>]> = storeBindable.sending(\.router.updateRoutes)
    WithPerceptionTracking {
// Here i passing an observable binding of [Route<Screen.State>]
      Router($store.routes.sending(\.router.updateRoutes)) { screenState, index in
        WithPerceptionTracking {
          let store = self.store.scopedStore(state: screenState, index: index)
          switch store.case {
          case let .landing(store):
            LandingFeatureView(store: store)
          case let .step1(store):
            Step1FeatureView(store: store)
          case let .step2(store):
            Step2FeatureView(store: store)
          }
        }
      }
    }
  }
}
// Also we need to do store scoping for router result
// Here is an extension to our store
extension Store where State: ObservableState, State == AppCoordinator.State, Action == AppCoordinator.Action {
  
  func scopedStore(state: Screen.State, index: Int) -> StoreOf<Screen> {
    let stateKeyPath: KeyPath<AppCoordinator.State, Route<Screen.State>?> = \.routes[safe: index]
    let actionCaseKeyPath: CaseKeyPath<AppCoordinator.Action, IndexedRouterActionOf<Screen>> = \.router
    let storeId:ScopeID<AppCoordinator.State, AppCoordinator.Action> = self.id(state: stateKeyPath, action: actionCaseKeyPath)
    let toState: ToState<AppCoordinator.State, Screen.State> =
    ToState { rootState in
// If we uncomment this the whole stack will start recomputing again
//      return rootState[keyPath: stateKeyPath]?.screen ?? state
//If just return a state then stack will NOT be recomputed BUT the actions from views STOP's modifying own state
// only AppCoordinator.State modified
      return state
    }
    return self.scope(id: storeId, state: toState) { ch in
        .router(.routeAction(id: index, action: ch))
    } isInvalid: {
      $0[keyPath: stateKeyPath] == nil
    }
  }
}

All changes available in example project, also i've embedded TCACoordinators + FlowStacks as package to the sample so you can easely modify all sources to play around it.

[sdkdimon]

Another thing that i am figured out that even such keypath call:
let childState = rootState[keyPath: stateKeyPath]?.screen were added to the ToState closure will cause screen recomputation.

ToState { rootState in
  let childState = rootState[keyPath: stateKeyPath]?.screen // --> this call will cause redrawing whole stack on it changes
//State from func arg
  return state
}

[mirkobraic]

Hi @sdkdimon, here are my thoughts.

1. let route = self.store[1] gives a compiler error in TCARouter.swift

It's a bit unclear to me what this line should do and what does it prove. For example, if we write let route = self.currentState.store[1] the compiler does raise any errors.
Additionally, if we create a test ObsevableState struct like this:

@ObservableState
struct ObservableStruct { }

We can then set up a similar store in TCARouter.swift, and the compiler will throw the same error:

let test: Store<[ObservableStruct], RouterAction<ID, Screen, ScreenAction>>
let _ = test[1] // error: Referencing subscript 'subscript(dynamicMember:)' on 'Store' requires that '[ObservableStruct]' conform to 'ObservableState'

Despite this, I don't believe it will lead to any issues with observing the array incorrectly, as I have used arrays of ObservableState within the reducer and everything behaves as expected.

2. "so our array of routes is not Observable out of the box, so that causing unwanted re render when the navigation stack is changed."

That's correct. But to be more precise, Route enum is not Observable out of the box. Once we fix this issue, array will get the conformance for free thanks to _$isIdentityEqual functions.

3. Regarding the example project, have you tried the Route extension I provided earlier? How does the app behave then?

4. rootState[keyPath: stateKeyPath]?.screen causing screen recomputation.

I'm not entirely sure, but my guess is that without this, the screen won't be added to the observationRegistrar and thus won't be observed at all. However, once the screen is added to the registrar, it starts over-observing, meaning we still did not fix our "observation chain". That's why I'm curious how would this behave if we added ObservableState to the Router via extension. I'm taking a wild guess here, but it's a direction that makes sense to me.

[sdkdimon]

The sample that i provided above contains your Observable Router extension implementation, and it has same behaviour. More of all i am even tried to split FlowStacks into composable features with own state and issue still there, so seems to be we are missing something.
Also i am noticed that from ComposableArchitecture perspective if we have a collection of features in the State it representation is handled by ForEach on the View side.

[sdkdimon]

Seems the whole stack recalculation issue on it change is not related to the TCACoordinators and FlowStacks SwiftUI implementation part.
Whole stack recalculation issue appears even with simple ForEach, here the sample.

@Reducer
public struct ForEachScreenCoordinator {
  @ObservableState
  public struct State: Equatable {
    public static let initialState = State(routes: [.landing(.init())])
    var routes: IdentifiedArrayOf<Screen.State>
  }
  
  public enum Action {
    case router(IdentifiedActionOf<Screen>)
  }
  
  public init() {}
  
  public  var body: some ReducerOf<Self> {
    Reduce { state, action in
      switch action {
      case .router(.element(id: _, action: .landing(.nextStepTapped))):
        state.routes.append(.step1(Step1Feature.State()))
        return .none
      case .router(.element(id: _, action: .step1(.nextStepTapped))):
        state.routes.append(.step2(Step2Feature.State()))
        return .none
      default:
        return .none
      }
    }
    .forEach(\.routes, action: \.router, element: {
      Screen.State.StateReducer.body
    })
  }
}

public struct ForEachCoordinatorView: View {
  public  let store: StoreOf<ForEachScreenCoordinator>
  
  public init(store: StoreOf<ForEachScreenCoordinator>) {
    self.store = store
  }
  
  public var body: some View {
    WithPerceptionTracking {
      VStack {
        ForEach(store.scope(state: \.routes, action: \.router)) { childStore in
          WithPerceptionTracking {
            switch childStore.case {
            case let .landing(store):
              LandingFeatureView(store: store)
            case let .step1(store):
              Step1FeatureView(store: store)
            case let .step2(store):
              Step2FeatureView(store: store)
            }
          }
        }
      }
    }
  }
}

[sdkdimon]

Also i am noticed that TCA library used special types for Navigation to hold screen feature it StackState and StackAction for actions handling. I'll try to check whether our issue is reproduced with the native TCA navigation. Maybe we need to look at the implementation of StackState and look how the Observation is implemented there.

[sdkdimon]

I am checked the whole stack recalculation issue with TCA "native" navigation and seems it is there, maybe it is not an issue at all. Here the sample code, it also available here.

@Reducer
struct RootFeature {
  @Reducer(state: .equatable)
  enum Path {
    case landing(LandingFeature)
    case step1(Step1Feature)
    case step2(Step2Feature)
  }
  
  @ObservableState
  struct State: Equatable {
    var path = StackState<Path.State>()
  }

  enum Action {
    case path(StackActionOf<Path>)
    case pushLanding
  }
  
  var body: some ReducerOf<Self> {
    Reduce { state, action in
      switch action {
      case .pushLanding:
        state.path.append(.landing(.init()))
        return .none
      case let .path(action):
        switch action {
        case .element(id: _, action: .landing(.nextStepTapped)):
          state.path.append(.step1(.init()))
        case .element(id: _, action: .step1(.nextStepTapped)):
          state.path.append(.step2(.init()))
          return .none
        default:
          return .none
        }
      }
      return .none
    }
    .forEach(\.path, action: \.path)
  }
}

struct RootView: View {
  @Perception.Bindable var store: StoreOf<RootFeature>
  
  var body: some View {
    WithPerceptionTracking {
      NavigationStack(
        path: $store.scope(state: \.path, action: \.path)
      ) {
        WithPerceptionTracking {
          Button {
            store.send(.pushLanding)
          } label: {
            Text("Push Landing Page")
          }
        }
        
      } destination: { store in
        WithPerceptionTracking {
          switch store.case {
          case .landing(let store):
            LandingFeatureView(store: store)
          case .step1(let store):
            Step1FeatureView(store: store)
          case .step2(let store):
            Step2FeatureView(store: store)
          }
        }
      }
    }
  }
}