Improve RoomList performance via side-stepping React

[ara4n]

Previously we regenerated the whole RoomList state on pretty much any event from the js-sdk, causing React to them re-render the whole RoomList. This is incredibly heavy (~350ms on my account), and chews a lot of CPU and makes the UI laggy.

This switches the common paths of handling js-sdk events for selecting rooms, Room.timeline, Room.receipt and a few easy ones like Room.name, RoomMember.name, RoomState.members to side-step React and prod the specific relevant RoomTile or RoomSubList to update themselves rather than have to poke them via React props. Rarer ones like Room.tags, Room, deleteRoom, m.direct still cause a full regeneration of the room list.

It does this by dispatching an event to them using a new ConstantTimeDispatcher (CTD), which the tiles & sublists use to register themselves to be directly prodded for these events. CTD works by registering listeners for both an event type and an argument, meaning that only the right listener gets woken up for a given argument rather than iterating over all of them (which could be thousands of RoomTiles).

In a test jig benchmark (changing the name of a single roomtile in a list of 2000), it took 350ms (180ms in prod webpack) of processing when updating the tile by generating a new list, versus 3.5ms when doing a constant time dispatch on the right tile.

Alternative approaches could be to:

• have RoomTiles subscribe directly to the events emitted from Room objects in the js-sdk (rather than the events as re-emitted from MatrixClient). @dbkr suggested this after the ConstantTimeDispatcher stuff was already in existence, and given CTD feels like a vaguely useful tool to have around, I haven't had the heart to yank it out again, although if this works it'd simplify the dispatching a bit.
• have RoomTiles register themselves as references on their RoomSubList, so parents can prod them directly to re-render when needed.
• use immutable state (e.g. Immutable.js) to force the right React components to re-render on demand, at the expense of generating new data every time anything changes (meaning shouldComponentUpdate can just do a trivial shallow comparison). This is presumably the most 'reacty' solution.

Twinned with element-hq/element-web#3654 (although oh my god we need to move RoomSubList* into react-sdk)

[ara4n]

not entirely sure why travis is failing; tests pass locally. travis' logs say:

�[1A�[2KPhantomJS 2.1.1 (Linux 0.0.0) joining a room over federation should not get stuck at a spinner FAILED
	Expected null to exist
	assert@/home/travis/build/matrix-org/matrix-react-sdk/riot-web/test/all-tests.js:4946:28
	toExist@/home/travis/build/matrix-org/matrix-react-sdk/riot-web/test/all-tests.js:2309:29
	/home/travis/build/matrix-org/matrix-react-sdk/riot-web/test/all-tests.js:9371:53

...which isn't very helpful.

[kegsay]

I've just read your summary for now, not the code. Broadly speaking what you're doing makes a lot of sense, though I would've preferred option 2:

have RoomTiles register themselves as references on their RoomSubList, so parents can prod them directly to re-render when needed

Option 1 will end up being slow as the number of rooms increases as it still needs to loop every single RoomTile when any event fires, which is not free.

Option 3 as you say, generates a lot of junk, and I'm unconvinced that the GC churn this will create won't become a serious problem if we did this everywhere.

I'll update this comment when I've had a look at the code.

EDIT: Cursory skim looks good to me, I haven't looked at this in any real detail though.

[kegsay]

What makes ConstantTimeDispatcher different from just using the type and arg as the key name? E.g:

EventEmitter.on(`${type} ${arg}`, function() { ... });
EventEmitter.on("room !curbaf:matrix.org", function() { ... });

From what I can see, there is no difference at all other than sugar coating and not needing to decide on a suitable delimiter (" " in this case). If you want to be pedantic, unregister operations are more expensive in ConstantTimeDispatcher because it requires you to loop over all the listeners for a given type, whereas the EventEmitter form is just a hash lookup.

I'd rather we didn't re-invent the wheel on this.

[ara4n]

Answer: CTD actually exposes an API with the right shape & (in future) typing rather than relying on the kludge of concatenating the keys into the event type. Also, we're not using EventEmitter anywhere in matrix-react-sdk (or riot-web) yet, other than receiving events from matrix-js-sdk, so if we're adding a new dispatcher it might as well be CTD as much as EventEmitter.

That said, I'd be happy if CTD became an API facade for EventEmitter in future - or if we skipped it entirely in favour of some other dispatch mechanism in future; see comments below...

[ara4n]

After discussing IRL: kegan & matthew quite liked the idea of using refs, as they keep a parent->child relationship; dave quite liked using dispatched events (but would prefer not having CTD in there). Conclusion is that nobody cares enough to necessarily require the PR to be rewritten, so if it works here, keep it as is for now, but file a maintenance bug for restructuring it to perhaps use refs or a non-CTD dispatcher in future.

[dbkr]

I share @kegsay 's concern about having to remember to dispatch the right things to cause the updates, I think mostly because the things triggering the updates are coming from parent views, rather than directly from the store itself, so in future perhaps we can look at more targeted events from the js-sdk. This doesn't help for the selected room case though: I'll elaborate on my thoughts in element-hq/element-web#3659 but the selectedRoom stuff feels buggy because of the fact we only want the RoomTiles to go the 'selected' colour if they're in the RoomList and not if they're instantiated on their own, but all tiles will gets events from the global CTD, although as discussed IRL, it probably works because of the lifetime of the MemberInfo RoomTiles. It feels like it only works because of a bug though.

Otherwise, a couple of comments, but otherwise this looks fine: it's a nontrivial performance problem and this is an acceptable way of getting the performance.

[dbkr]

Looks like this is spurious?

[ara4n]

good catch - thanks

[dbkr]

I was expecting to see a RoomTile.select dispatched in RoomList's componentWillMount to set up the initial selection state, but I don't see where this is happening?

[ara4n]

good catch, i missed it.

[ara4n]

@dbkr PTAL