johnpatrickmorgan / TCACoordinators

Powerful navigation in the Composable Architecture via the coordinator pattern
MIT License
428 stars 36 forks source link

Performance issue using Observation in v0.10.1 #69

Open mirkobraic opened 1 week ago

mirkobraic commented 1 week ago

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:

https://github.com/johnpatrickmorgan/TCACoordinators/assets/22961673/c82f309d-dab3-4637-8722-22c960e31682

And this is the result for v0.10.1

https://github.com/johnpatrickmorgan/TCACoordinators/assets/22961673/31a27041-18f7-4e1c-9cea-dbcd1ae85546

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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.

https://github.com/johnpatrickmorgan/TCACoordinators/assets/22961673/d9b02859-91a2-4184-9230-f6bf3768f7d4

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.

https://github.com/johnpatrickmorgan/TCACoordinators/assets/22961673/3df4c761-356b-4461-a121-3499eaa67be5

Coordinators090Test.zip Coordinators0101Test.zip

yurihan commented 1 week ago

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