pointfreeco / swift-composable-architecture

A library for building applications in a consistent and understandable way, with composition, testing, and ergonomics in mind.
https://www.pointfree.co/collections/composable-architecture
MIT License
12.22k stars 1.42k forks source link

IfLetStore performance issues when actions are sent back into the store #2354

Closed Xythus closed 1 year ago

Xythus commented 1 year ago

Description

I am working on an application where due to various reasons I'm making use of SwitchStore inside of a list of items for a feature. The items themselves are rather complex and need to behave and render very differently depending on their state. In order to achieve this, I have separated out the various shapes and forms these items can take into different reducers and views, which are then grouped into a generic Item reducer and view where I use an enum property to keep track of the item's type and associated state (stored in an associated value).

It is in this implementation that I have noticed a rather interesting behavior. When an action is sent back into a store with one of these lists of items are present in the application's state, the user interface can start hitching/stuttering. This happens regardless of whether the items are currently visible and even regardless of whether their state is affected at all or not.

After digging a little bit more (and with the help of Rhys Morgan on Slack), we narrowed it down to the IfLetStore view used inside CaseLet. With enough of these views present, sending actions back into the store causes noticeable enough CPU spikes on the main thread that the UI can start stuttering. The actions don't even have to contain any logic for this to happen. I have created a small torture test to demonstrate this problem:

struct Root: Reducer {
    struct State: Equatable {
        var optional: Int? = 1
    }

    enum Action: Equatable {
        case onAppear
        case spam
    }

    var body: some ReducerOf<Self> {
        Reduce { state, action in
            switch action {
            case .onAppear:
                return .run { send in
                    while true {
                        await send(.spam)
                        try? await Task.sleep(for: .seconds(0.1))
                    }
                }

            case .spam:
                return .none
            }
        }
    }
}

struct RootView: View {
    let store: Store<Int?, Root.Action>

    init(store: StoreOf<Root>) {
        self.store = store.scope(state: \.optional, action: { $0 })
    }

    var body: some View {
        List {
            ForEach((0...5000), id: \.self) { id in
                IfLetStore(self.store) { _ in
                    Text("\(id)")
                }
            }
        }
        .onAppear { self.store.send(.onAppear) }
    }
}

@main
struct IfLetStoreTortureTest: App {
    var body: some Scene {
        WindowGroup {
            RootView(store: .init(initialState: .init(), reducer: Root.init))
        }
    }
}

Running this small application on my iPhone 13 Mini pins the total CPU usage at a constant ~30% with consistent small stutters in the UI whilst scrolling (most visible when scrolling slowly). I'd like to stress that, while the demo application is obviously not a real-world application due to the amount of actions being sent back into the store, it is also much lighter in the sense that the actions that are being sent back into the store don't do anything. In my real-world application the (light) work done in the actions is enough to make the stutters themselves much more noticable (longer in duration), even if they are less common. Moreover, the stutters are much more pronounced on older hardware, and actions such as entering text in a textfield still send a lot of consecutive actions into stores.

Checklist

Expected behavior

I would have expected that using CaseLet/IfLetStore would not have such a major performance impact when the underlying state remains unaffected.

Actual behavior

No response

Steps to reproduce

I have attached three videos of me running (a redacted version of) my real-world application where I show off the stutters in a more pronounced fashion:

https://github.com/pointfreeco/swift-composable-architecture/assets/1338909/c8ebc45a-e1c1-4cab-bd90-1b721798e258 https://github.com/pointfreeco/swift-composable-architecture/assets/1338909/f7ea3970-8dc8-4294-a0c3-e2177cb66048 https://github.com/pointfreeco/swift-composable-architecture/assets/1338909/7848f2bb-93b8-4063-b8f6-9d558204f2c2

The Composable Architecture version information

1.0.0

Destination operating system

iOS 16.6 (20G75), iOS 17 beta 4

Xcode version information

15.0 beta 5 (15A5209g)

Swift Compiler version information

swift-driver version: 1.75.2 Apple Swift version 5.8.1 (swiftlang-5.8.0.124.5 clang-1403.0.22.11.100)
Target: x86_64-apple-macosx13.0
tgrapperon commented 1 year ago

Hey @Xythus, thanks for the report! The issue you're reporting is related to what is described here. More explicitly, because your row's content is potentially not present, SwiftUI needs to eagerly evaluate all rows when a change happens to be sure that each row gets an identity.

As a workaround, you can try to wrap your IfLetStore in a non-conditional view, like ZStack for example. It won't work in every situation, but it can do the trick in a few of them.

It doesn't mean that we can't improve a few things in this area at the library level, but the general rule of SwiftUI's lists and lazy stacks is that rows should be plain views (that is not directly IfLetStore or SwitchStore).

Xythus commented 1 year ago

Hey @tgrapperon. The issue seems to actually not be related to SwiftUI's lists. That is one thing I originally considered, but the performance issues are still present if you just stack the IfLetStores on top of each other, wrapped in a ZStack for example. You can test this for yourself by modifying the RootView in the torture test to the following:

struct RootView: View {
    let store: Store<Int?, Root.Action>

    init(store: StoreOf<Root>) {
        self.store = store.scope(state: \.optional, action: { $0 })
    }

    var body: some View {
        ZStack {
            ForEach((0...5000), id: \.self) { _ in
                IfLetStore(self.store) { _ in Text("Hello, world") }
            }
        }
        .onAppear { self.store.send(.onAppear) }
    }
}

Or even just:

struct RootView: View {
    let store: Store<Int?, Root.Action>

    init(store: StoreOf<Root>) {
        self.store = store.scope(state: \.optional, action: { $0 })
    }

    var body: some View {
        ZStack {
            IfLetStore(self.store) { _ in Text("Hello, world") }
            IfLetStore(self.store) { _ in Text("Hello, world") }
            IfLetStore(self.store) { _ in Text("Hello, world") }
            IfLetStore(self.store) { _ in Text("Hello, world") }
            IfLetStore(self.store) { _ in Text("Hello, world") }
            // Repeat as many times as needed (or as many times as your compiler lets you :) )
        }
        .onAppear { self.store.send(.onAppear) }
    }
}

I'm unfortunately not very familiar with the library's internal workings in this area, but my initial guess is that it has something to do with store invalidation, just from observing the function calls.

fabstu commented 1 year ago

but the general rule of SwiftUI's lists and lazy stacks is that rows should be plain views (that is not directly IfLetStore or SwitchStore).

Any idea how to get Switch-behavior without a switch? I rely heavily on multi-level SwitchStore (for extendability I thought) to differentiate item types in a list that might get long and I do not see any way around it. Or is that not an issue when doing the switch in a sub-view of each item?

Anyway, I currently use List (evaluates only what is visible) instead of ScrollView to avoid multi-seconds-lag. I thought that is normal.

mbrandonw commented 1 year ago

Hi @Xythus, as @tgrapperon mentioned, this is a subtlety of SwiftUI that is independent of TCA. And the problem even exist in vanilla SwiftUI. The "Demystifying SwiftUI" WWDC session covers this, and there is some good discussion in this Swift forums post. That post also includes some sample code to show that the problem does indeed occur in vanilla SwiftUI.

The tldr; is that if you do anything too specialized in the row of a List then you run the risk of ruining performance since the underlying UICollectionView will have to do more work for calculating number of rows, row heights, etc. This includes using things like conditionals (and hence IfLetStore), AnyView, and even sometimes just simple HStacks and VStacks.

Here's a completely vanilla SwiftUI demo that has a serious performance problem:

Click to expand ```swift import SwiftUI struct Item: Identifiable { var id: Int } struct ItemView: View { let item: Item var body: some View { if item.id <= 10000 { // Remove to improve performance, but not necessarily fix performance. Text("\(item.id)") } } } struct ContentView: View { @State var items: [Item] = (0 ..< 10_000).map { Item(id: $0) } var body: some View { List(items) { item in ItemView(item: item) } .task { while true { try? await Task.sleep(for: .seconds(0.1)) self.items[0].id += 1 // Just a no-op 😳 } } } } struct ContentView_Previews: PreviewProvider { static var previews: some View { ContentView() } } ```

This demo has a large list of integers, and it starts a timer that ticks ever 0.1 seconds in order to increment the first element in the list. You will find that this seemingly simple demo has horrible scroll performance. And if you comment out the no-op line then the performance returns to normal. And if you comment out the conditional in the row view, performance improves, but is still pretty bad.

So, this problem does plague vanilla SwiftUI and doesn't really have that much to do with TCA. In general, I'm just not sure lists of heterogenous data in SwiftUI can really be all that performant. I don't know if there is a solution at all, whether it's vanilla SwiftUI or TCA.

However, where I think TCA is somewhat at fault is that it provides tools are perhaps too powerful and it is easy to forget that such powerful abstractions do come with a cost. Perhaps we need more documentation around this.

For example, it may seem innocent enough to have a big list inside ForEachStore where each row is a SwitchStore, but that is actually an incredibly complex composition of features. You essentially have a child feature that is bifurcated via an enum so that each case can operate completely independently. And then you compose 5k copies of that feature into one big feature. And then on top of all of that, the parent feature has infinite introspection into what is happening in each row and even each case of the row's enum. These abstractions do come with a cost, and so you do have to be aware of that. If you were to construct the equivalent demo in vanilla SwiftUI it would just require a ton of boilerplate, and you would still be left with the same performance problems.

The issue seems to actually not be related to SwiftUI's lists. That is one thing I originally considered, but the performance issues are still present if you just stack the IfLetStores on top of each other, wrapped in a ZStack for example.

It's not that it's related to lists, it is related to how well SwiftUI can employ its "smarts" to do a better job than doing things naively. Lists can typically lazily delay work if it has enough information to do so. For example, typically the only views created in a List are the ones that are visible. But doing conditionals, or using AnyView, erases all of that information, and then it has no choice but to do the naive thing and construct the view for every row immediately, not just the visible ones.

And so the fact that the performance problem persists when ditching the List is not surprising. In fact, it's very much expected! You have removed the one thing that can layer on some smarts to delay executing work and instead forced SwiftUI to execute everything all at once. And vanilla SwiftUI would struggle with the exact same set up.

I believe this is a very good discussion to have (and there has been a similar one in the past), but I don't really think it's an issue with the library. There may be room for improvement in some of the tools, but that needs more experimentation to see what can be done. And perhaps docs can be improved there, and so we are open to ideas for that too.

I think for now it would be best to convert this to a discussion, and for everyone to keep sharing their findings. We are always on the look out for improvements to be made, and we would love to see anything the community uncovers.