element-hq / element-web

A glossy Matrix collaboration client for the web.
https://element.io
GNU Affero General Public License v3.0
11.19k stars 2k forks source link

Dispatcher v2 #17055

Open turt2live opened 3 years ago

turt2live commented 3 years ago

Working theory: image (not to scale)

We can likely improve app performance by simply waking up less components with the global dispatcher. The diagram above is one possible solution, where there are core dispatchers (to avoid waking each other up with noise) and "feeder" dispatchers which are relevant to a particular entity (room, user, model, etc). Those feeder dispatchers would subscribe to specific details from the more coarse dispatchers and filter down accordingly, then filter down again before activating downstream listeners.

As a performance baseline, this is what a random snapshot of activity on my main account (develop, chrome) looks like: image

The first few spikes are room switches (with Spaces enabled), then we move into sending messages, browsing rooms, and handling stress tests in megolm-test.

The metrics to beat are (if we assume that the performance issues are in fact because of the dispatcher):

turt2live commented 3 years ago

A slightly better drawing (click to zoom): image

The overarching theory is that we can get better performance by waking up less things. As such, we can integrate the ViewModel concept into the dispatcher system as well, reducing the chance that the component will be woken up for an arbitrary dispatch. We don't necessarily want to wake all the models either though, as this could result in a lot of state dedupe checks: this is why there's multiple layers of dispatcher above it.

In the diagram, we've created a number of "feeds" that a model can listen for, which can then inform the components about updates. Anything the components want to dispatch would get dispatched back to the model [dispatcher] which then dutifully sends it off the central dispatcher. The feeds help avoid waking the models too much by allowing the models to subscribe to mildly more interesting information than "all of the Room stuff". In practice, this feeder layer might have near-zero effect on performance, but might help with developer happiness in knowing that they are not having to repeat the if (payload.roomId !== this.roomId) return; checks over and over.

The feeders are expected to filter coarse topics down to something that models might be interested in, such as per-room streams and splitting the crypto stream in two (or combining two coarse streams into a filtered User topic, such as combining user trust changes with user presence). The coarse dispatchers are largely responsible for picking off major topics from the central dispatcher: the diagram above has the likely 6 we'd have. We wouldn't really end up with many more.

Where a coarse topic might not need any filtering (ie: the UI topic), a passthrough feeder would be created to avoid breaking the semantics of models being fed data from a "filtered" perspective.


In a multiaccount world, we could end up with something like this: image

The global dispatcher would be a broker between the various CentralDispatchers which represent 1 account each. The lines between the central and global dispatchers are smaller because the central dispatchers would know what sort of cross-account information is interesting, and the global dispatcher would likely re-fire it to all the other central dispatchers it knows about.

The names "central" and "global" would probably need renaming in these scenarios, given central is obviously not central anymore. Possibly just "global" and "client".


As mentioned, these layers might be overkill for reducing wake counts, but might help with more fluid development. I'm interested in people's thoughts though, given this is a pretty major change (and one that affects pretty much all future development).

turt2live commented 3 years ago

Early API structures are up at https://github.com/matrix-org/matrix-react-sdk/pull/6050 - see included checklist for plan of progression.

turt2live commented 2 years ago

Having looked at the diagrams a year later, something which isn't obvious is how it actually solves the issue: surely if the problem is a single dispatcher then having a central dispatcher will still be a bottleneck?

The feeds could have a short circuit to echo the dispatch quickly and wait for the echo to crawl back over the remainder of the tree (inhibiting the update), though this gets complex to manage.

An alternative approach would be a "stop at" flag so the dispatch only bubbles up so high before getting sent back around. This gets complicated to represent though.

A good option might be a mix of short circuiting echo, spidering, and "stop at" flags: the dispatches only bubble up so high and we are very careful not to echo the dispatch down the dispatcher which sent it.

turt2live commented 2 years ago

In random (brief) review of this, we might be able to leverage an EventEmitter interface as glue between the current dispatcher and a few key areas to build confidence that this is actually a [solvable] problem. If it goes well, we'd go all-in on event emitters.

kegsay commented 2 years ago

The diagram in the original post feels incredibly complex. I don't understand why we are proposing a rewrite to the dispatcher when we don't even know if this is the cause of slowness as per:

(if we assume that the performance issues are in fact because of the dispatcher)

Perhaps we should work out what the slowness is before proposing a complete rewrite?

Having looked at the diagrams a year later, something which isn't obvious is how it actually solves the issue: surely if the problem is a single dispatcher then having a central dispatcher will still be a bottleneck?

I don't see what the concern is here. The performance of the dispatcher will surely depend on the implementation of said dispatcher. A noddy dispatcher which is just a Record<string, Function[]> will scale based on the number of listeners to the same notification ID. If we want to improve the performance of the dispatcher, then just namespace your IDs? You'd end up with a map with many keys, but that's fine? If it isn't, you can trivially nest the maps so each map does not exceed a certain value, but at this point I really feel this isn't actually solving the problem, as I don't believe this will significantly affect performance.

My primary concern here is that we will waste time and effort rewriting the dispatcher, not see any perf gains, and just add extra complexity as you now need to think about which dispatcher to notify/listen on.

turt2live commented 2 years ago

@kegsay https://github.com/vector-im/element-web/issues/17055#issuecomment-1096870432 is the latest on this, noting that the original plan to rewrite the dispatcher is effectively off the table.