Make the UI rebuild only when the relevant data in PerAccountStore changes

[sm-sayedi]

Right now, we are rebuilding the UI based on the different settings (properties) of PerAccountStore, a ChangeNotifier injected into the widget tree through an InheritedNotifier widget.

I have noticed that unnecessary rebuilds often happen when a property of PerAccountStore changes, and because notifyListeners is called, all the registered build methods are called, even if they do not care about that specific property. I have examined this by putting some log statements in ComposeBox.build and _MessageListPageState.build methods both of which have a dependency on PerAccountStore by calling PerAccountStoreWidget.of(context). I then went to the web app and changed the time format settings for my account, which is PerAccountStore.userSettings in the codebase, a property that is not needed in the build methods mentioned above at the moment. This change of settings resulted in those unrelated build methods being called. There are numerous other places where such unnecessary rebuilds happen.

We could implement this granular control of listening from scratch, or we can use a third-party package. One of those packages I would recommend is Provider. When using this package, we can do something like this:

context.select<PerAccountStore, Map<int, User>>((store) => store.users);
context.select<PerAccountStore, UserSettings?>((store) => store.userSettings);

This will re-call the build methods only when the specified properties are changed.

[chrisbobbe]

I then went to the web app and changed the time format settings for my account, which is PerAccountStore.userSettings in the codebase, a property that is not needed in the build methods mentioned above at the moment.

Perhaps worth mentioning that the message-list page (or parts of it anyway) should be rebuilding with visible changes when the time-format setting is changed 🙂—I've just filed #1015 for that.

[gnprice]

This is purely a matter of performance, not correctness — the behavior is correct either way — so the best thing to do is a quantitative question of what gives the best performance, weighed against effects on the complexity of reading and writing the code.

We already do as the issue title says for many types of updates. If you take a look at PerAccountStore.handleEvent and highlight calls to notifyListeners, you'll see that for 9 event types we call that method but for 7 other event types we don't (plus 3 more where we don't update any data anyway). Notably those 7 include all the events expected to be the most frequent, like MessageEvent, ReactionEvent, and TypingEvent. For each of those, the data that they affect lives on sub-models like MessageListView or TypingStatus, which are ChangeNotifiers themselves and notify their own listeners.

I'm content for us to rebuild the whole tree when one of the infrequent types of events happens, including UserSettingsUpdateEvent. There are a couple of trade-offs that'd be involved in tracking the data dependencies more finely:

• It makes the code more complex, both in the UI that's consuming the data and in the model code that's updating it.
• It means additional bookkeeping at runtime, which has a performance cost of its own. The bookkeeping cost is paid whenever building any widgets that depend on the finely-tracked data, whereas the rebuild cost is paid only when that data actually updates. So if it's much less common for the data to actually update than it is to have UI that uses it, then the coarser-grained approach is better for performance.

So I'll close this issue because I think we don't need to go farther in this direction than we have already.

If at any point we find we have a performance issue in practice that's related to rebuilds, we'll want to investigate and resolve that. We can open a specific issue then.

[gnprice]

If we do see a rebuild-related performance problem in the future: in general, the canonical Flutter approach to that sort of problem is to arrange things so the build methods are cheap, more so than to try to prevent rebuilds — the Flutter architecture is that widgets generally may be rebuilt on every frame. Discussion in docs here, with more details:
https://main-api.flutter.dev/flutter/widgets/StatelessWidget/build.html
https://main-api.flutter.dev/flutter/widgets/StatefulWidget-class.html#performance-considerations
(including more nuance than in my one-sentence summary; as those docs explain, there is a place for widgets that avoid getting rebuilt).

We do have a few known places in the app where the build methods are expensive. For example InboxPage, which has a TODO comment for it describing the way forward:

    // TODO(perf) make an incrementally-updated view-model for InboxPage

For any places do where we want to track more fine-grained data dependencies: while I wouldn't absolutely rule out package:provider because I haven't looked into it closely, there are a couple of ways to do that job that are already present in Flutter upstream.

• One is what we do already as described above: have additional ChangeNotifier subclasses for sub-pieces of the model, like MessageListView.

• The other is InheritedModel, which is:

  An InheritedWidget that's intended to be used as the base class for models whose dependents may only depend on one part or "aspect" of the overall model.

  So in particular we could make PerAccountStoreWidget use that instead of InheritedNotifier. Then UI code would write things like (following the examples in the issue description above):

  final users = PerAccountStoreWidget.usersOf(context);
  final userSettings = PerAccountStoreWidget.userSettingsOf(context);