Performance issue when triggering child actions

[jtouzy]

Describe the bug
We are currently facing a performance issue, when triggering an action from a large number of child components (in a ForEachStore), for loading their content. When the action is dispatched (from onAppear event), even if nothing is done inside it, the scroll is very laggy. I can't even find a good reason in Time Profiler, but I suspect all the actions are dispatched in Main thread, that causes the lag.

To Reproduce
I created a very lightweight project to reproduce the laggy scroll.
Just checkout this : https://github.com/jtouzy/PerformanceTestTCA
You will see the laggy scroll when you accelerate a bit (using a real device is better to see the difference).
If you just comment the line 42 in Child.swift, you will see that the scroll is perfectly fine, when no actions are triggered.
Even if you comment the reducer side effects for the action, and just return .none, the lag is still present.
That's why we are thinking about the thread.

Maybe you have a solution for this or maybe we need to implement it another way?

Environment

• Xcode 11.4
• Swift 5.3.2
• iOS 14

[siejkowski]

I'd also love to hear and discuss the ideas on how to work around this in an elegant way.

In the meanwhile I can share the solution I currently use in my own project.
I had the exact same problem implementing async image loading in huge lazy grids in my own apps (collections with thousands of images). The send function, called in onAppear, was taking too much time, resulting in a sluggish scrolling performance.

My solution was to not use neither forEach reducer nor ForEachStore. I've extracted the async image loading stuff out of the main store entirely. I keep it in a separate object, completely not connected to the rest of the TCA-driven app.

Here's this solution adopted to your test project:

public final class ChildStores {
  
  private var childStores: [String: Store<Child.State, Child.Action>] = [:]
  
  func createIfNeeded(for child: Child.State) -> Store<Child.State, Child.Action> {
    guard let store = self.childStores[child.id] else {
      let store = Store<Child.State, Child.Action>(
        initialState: Child.State(item: child.item),
        reducer: Child.reducer,
        environment: Child.Environment(loadItem: { item in
          // ... pass load item here, probably it's worth extracting it to separate object to avoid code duplication
        })
      )
      self.childStores[child.id] = store
      return store
    }
    return store
  }
}

let childStores = ChildStores() // doesn't need to be global, really, but it must be kept somewhere

Then you can use it when constructing the Child.View:

extension Main {
  struct View: SwiftUI.View {
    let store: Store<State, Action>

    var body: some SwiftUI.View {
      WithViewStore(store) { viewStore in
        ScrollView {
          LazyVGrid(columns: [.init(), .init()]) {
            ForEach(viewStore.children) { (item: Child.State) in
              Child.View(store: childStores.createIfNeeded(for: item))
            }
          }
        }
      }
    }
  }
}

As you can test, the performance is fine again.

However, the solution, while working and pragmatic, looks more like a workaround to me. I'd love to scrap it for something better, so please share if you find anything!

[siejkowski]

Oh, and of course this brings the whole world of maintaining the separate stores (like clearing them when they are no longer needed etc.) which I'm dealing with in my actual project, I've just not include it in the samples above.

[manmal]

Just an idea, could you have an ImageLoader in the Environment which offers Publishers for loading the images? I think images are of ephemeral nature anyway, and don’t need their state persisted in a store. The ImageLoader can also employ caching if necessary.

[siejkowski]

Yeah, but how the View uses the image (or any kind of data) then? The view doesn't have the access to the environment, it only has the access to the state via view store. So even if ImageLoader does the caching and stuff, the image still has to be kept in the state so that it's exposed through the view store to the view.

Ofc you can just inject the ImageLoaded to the view directly. It is also cool, but it means you're just not using the TCA for that part of the app.

[manmal]

@siejkowski

you can just inject the ImageLoaded to the view directly

True, I think that's what I'd end up doing.

but it means you're just not using the TCA for that part of the app

IMO that's ok - TCA is probably not meant to be a low-level, high-frequency information bus.

[siejkowski]

IMO that's ok - TCA is probably not meant to be a low-level, high-frequency information bus.

With the current performance constraints looks like TCA is not a good tool for such use cases, I agree.

I wonder if it could be improved by some internal optimisation. Time Profiler has led me to believe that what really slows action sending is Combine and the usage of CurrentValueSubject for keeping state in store. Maybe using an alternative mechanism there (custom subject?) could help?

[manmal]

Yeah, since the store isn't thread safe anyway, that would probably work. CombineExt has a ReplaySubject that could probably be adapted to storing only one value quickly: https://github.com/CombineCommunity/CombineExt/blob/main/Sources/Subjects/ReplaySubject.swift

[Pwyll28]

For me, the me main issue is that scope operations + equality tests are on the main thread.
An alternative would be to expose state via a Publisher and then scope is done by creating a new store with the parent exposed state.
(Sorry for the "RxSwift" boilerplate I'm doing some tests in a side project and I'm more confortable with RxSwift rather than Combine)

  public func scope<LocalValue, LocalAction>(
    state toLocalState: @escaping (State) -> LocalValue,
    distinctUntilChanged: @escaping (LocalValue, LocalValue) -> Bool,
    action fromLocalAction: @escaping (LocalAction) -> Action
  ) -> Store<LocalValue, LocalAction> {
    let childState = state
      .map(toLocalState)
      .distinctUntilChanged(distinctUntilChanged)
    let localStore: Store<LocalValue, LocalAction> = .init(
      state: childState,
      initialValue: toLocalState(internalValue.value),
      environment: environment
    )
    let parentDiposable = localStore
      .action
      .map(fromLocalAction)
      .subscribe(action)
    localStore.parentDiposable = parentDiposable
    return localStore
  }

With the corresponding private initializer

  // Initializers
  private init<Environment>(
    state: Observable<State>,
    initialValue: State,
    environment: Environment
  ) {
    self.environment = environment
    self.state = state
    self.internalValue = .init(value: initialValue)
  }

[jtouzy]

It would be interesting to get the point of view of @stephencelis or @mbrandonw about this.

[mbrandonw]

Thanks for all the discussion! We investigated this a bit this morning and there's definitely more work to be done with ForEachStore, in particular sending lots of actions in a ForEachStore. We don't have any solutions right now, but a couple of comments:

• First, running the project on a device in release mode shows much better performance than the simulator. It's still not perfect, but it's also not nearly as horrendous.

• Second, the work we are doing in ForEachStore seems a bit convoluted and probably not necessary. For example, replacing ForEachStore in the project with something as simple as this has much better performance:

  WithViewStore(store.scope(state: \.children.indices)) { viewStore in
    ForEach(viewStore.state) { index in
      Child.View(
        store: self.store.scope(
          state: { state in state.children[index] },
          action: { action in Main.Action.child(index: index, action: action)})
      )
    }
  }

  However, this is a little dangerous to do with the project as-is because it won't properly render re-orders (items will change but indices will not). It would be necessary to upgrade children to be an IdentifiedArray of Child.State. In reality, anytime you are using ForEachStore you should probably be using IdentifiedArray anyway because it's a far safer way of dealing with a ForEach data source.

• Finally, it can be instructive to compare TCA solutions with what the corresponding solution would look like in vanilla SwiftUI. We built a little vanilla version of your project using an ObservableObject view model and saw that performance was pretty good, but still when scrolling through lots of rows it would stutter. This must be because of all the DispatchQueue.main.async's firing.

  However, even this vanilla demo isn't a fair comparison to TCA. In TCA we are able to split off the child domain so that it is fully isolated from the parent. To do that in vanilla SwiftUI we would need some kind of mechanism for splitting of "child view models" from the parent view model, and there really hasn't been any movement in the community on how to attack that problem. One should try to accomplish that in vanilla SwiftUI in order to have a fully fair comparison.

We'll keep y'all updated with whatever we find in the future, and please let us know if you stumble upon anything!

Also, I am going to convert this issue to a discussion as that's a bit more appropriate for this.

[jtouzy]

We tried to replace ForEachStore with simple ForEach, but performance is still a problem, the scroll is still laggy.
The only solution that works for us, is to make the sub-component non-composable.
It's just a SwiftUI view with basic properties and handlers but no Composable pattern, and it works perfectly.
It seems like too many sub-scopes with actions dispatched in Main thread like this is not possible with the current project state.

We are thinking about a solution of sending all the actions in a background thread, and switch on the main thread only when updating the view... Working on it when we got some time...

[mbrandonw]

In your demo app you have a WithViewStore(self.store) even though the viewStore isn't actually used:

https://github.com/jtouzy/PerformanceTestTCA/blob/master/PerformanceTestTCA/Main.swift#L38

I would try removing that too to see if it helps at all.

[siejkowski]

We are thinking about a solution of sending all the actions in a background thread, and switch on the main thread only when updating the view... Working on it when we got some time...

If you plan on doing that work on public repo, please share it, I'll try to help if I can. As I said, I've been struggling with that issue in my production app and would love to drop the current workaround.

The current implementation of Store is not thread safe (e.g. access to synchronousActionsToSend and bufferedActions is not synchronised), one possible solution would be to change that and verify if the performance impact on synchronisation is smaller then doing all the work on the main thread.

[stephencelis]

@siejkowski The store purposefully has no opinion on concurrency and will always process things on the same thread that store.send is called (for effects, this is wherever the effect is received on). This is to keep things unopinionated, but also allow for things like SwiftUI animations to work as you would expect when calling store.send in a withAnimation block.

[stephencelis]

@jtouzy Can you give this PR branch a try? #386

It should improve performance for both index-based forEach and identifiable array-based forEach, but please update your domain to use IdentifiedArray and ID-based actions, which should be much faster (and more correct) than their index-based alternatives. We support plain arrays and index-based forEach for convenience, but it typically leads to correctness issues, especially when effects are introduced, so we may even deprecate it in the future.

Edit: also please compare performance in release mode, as well!

[Pwyll28]

Our app is base on user's photo gallery. We're using PHImageManager to perform async loading of the images on display.
So each image load updates the main state (we store an image cache) and we go through the default path to display the elements.

[manmal]

Yes, my suggestion is that, wherever you now have a Array<Photo>, you'd instead have <Array<Bucket<Photo>>>. Since you need to preserve order, the bucket size will have to fixed, e.g. to 100 photos per bucket.

[Pwyll28]

To make it simple that's our structure

struct Grid<Group, Source> {
  struct Section {
    struct Item {
      let item: Source
    }
    var group: Group
    var datasource: IdentifiedArrayOf<Item>
  }
  var datasource: IdentifiedArrayOf<Section>
}

We have some small "group's" of generic items. All identifiable and equatable and indexed by id

[Pwyll28]

Plus that content is only calculated on the photo gallery's update. For the images we have a [Photo.ID: UIImage] where Photo.ID is Grid<Group, Source>.Section.Item.Source.ID

[stephencelis]

@Pwyll28 Thanks for weighing in with your experience. Just to make sure I understand: ForEachStore is very slow for you, even on the PR branch I opened, while switching to ForEach is better but still slow because of removeDuplicates? Does that sound right? WithViewStore on the PR branch relies on comparing the array of IDs directly instead of custom removeDuplicates logic, so it should hopefully be more comparable to vanilla SwiftUI, but in general remember that ForEachStore is doing a lot of extra work under the hood for coordinating parent/child store communication, so it's unlikely to ever match the performance.

We think there are a few paths forward when dealing with massive data like this:

• Sometimes it's ok to hand control back to SwiftUI by passing removeDuplicates: { _, _ in false } to WithViewStore, or by defining a more performant equality check. I don't think we want to introduce a queue to TCA for checking equality, especially because as @manmal points out, it's merely pushing a problem elsewhere. In the original demo with the grid of 2,000 loading items, a queue may have also avoided the scroll hitches, but queued work would also quickly get backed up, and after scrolling quickly you may land on a screen of "Unloaded" items for quite some time while all the queued work catches up.
• If ForEachStore is slowing you down, you can by all means use ForEach instead. ForEachStore is definitely not a requirement for working with lists of state in TCA. Most of the time you can start with a plain ole ForEach, especially for read-only state. You can pass a collection in a view store to ForEach and you're done. ForEachStore is helpful for hyper-local modularization and for action-heavy list item state, but you are always free to unroll things if performance becomes a concern and don't mind doing a little extra work to coordinate parent/child state.

All this could be better documented, so we'll take a pass at that soon. In general, though, we feel that it is instructive to ask, "If I were to try to accomplish the same in vanilla SwiftUI would I encounter the same problem?" In this case, if we were to port all of what ForEachStore buys us, which is to be able to isolate the child in its own module, and to implement parent-child/child-parent communication and synchronization, would we still have issues in a vanilla SwiftUI app? Or would we have a different set of issues, and which trade off is better? Etc. We're always open to tweaking things, since TCA's design definitely introduces certain consequences, but we have to be even more mindful of the consequences of altering that design 😄