Improve the performance of MemberEventListSummary #590

lukebarnard1 · 2016-12-14T15:36:11Z

The MessagePanel now uses the same key for the MELS instances rendered so that entirely new instances are not created, they are simply passed new props (namely when new events arrive).
MELS itself now uses shouldComponentUpdate so that it only updates if it is given a different number of events to previous or if it is toggled to expand.
The render event is now linear in complexity as opposed to O(n^2) as it was previously.

- The MessagePanel now uses the same key for the MELS instances rendered so that entirely new instances are not created, they are simply passed new props (namely when new events arrive). - MELS itself now uses `shouldComponentUpdate` so that it only updates if it is given a different number of events to previous or if it is toggled to expand.

richvdh · 2016-12-15T12:09:40Z

src/components/structures/MessagePanel.js

@@ -296,6 +296,12 @@ module.exports = React.createClass({
            // Wrap consecutive member events in a ListSummary
            if (isMembershipChange(mxEv)) {
                let ts1 = mxEv.getTs();
+                // Ensure that the key of the MemberEventListSummary does not change with new
+                // member events. This will prevent it from being re-created necessarily, and


s/necessarily/unnecessarily/?

richvdh · 2016-12-15T12:12:36Z

src/components/structures/MessagePanel.js

+                // instead will allow new props to be provided. In turn, the shouldComponentUpdate
+                // method on MELS can be used to prevent unnecessary renderings.
+                // `prevEvent` at this point is a non-member event or null.
+                const key = "memberlistsummary-" + (prevEvent ? prevEvent.getId() : "");


ok so the goal is not to change the key as new events arrive? or? (can you put this in the comment?)

why is prevEvent better than mxEv for this?

ok so the goal is not to change the key as new events arrive? or?

Oh yeah you said that in the PR description. But still: why is prevEvent better than mxEv?

Because when back paginating, the first membership event should change, and prevEvent should be consistently null.

Because when back paginating, the first membership event should change, and prevEvent should be consistently null.

Well, until you get to a non-memberevent, I guess. But still, using the prevEvent's eventid as the key feels odd here. How about (prevEvent ? mxEv.getId() : 'initial');. And it needs comments.

How about (prevEvent ? mxEv.getId() : 'initial');

This misses the point. When back paginating a lot of member events, mxEv is constantly changing as it represent the first/earliest member event. If you key off the event ID for mxEv it will be constantly changing as well, leading to unnecessary teardown/setting up of new MemberEventListSummary classes, rather than passing them through as prop updates which is what you really want.

But still, using the prevEvent's eventid as the key feels odd here. How about (prevEvent ? mxEv.getId() : 'initial');. And it needs comments.

This still does not keep the key of MemberEventListSummary constant WRT the set of events changing within it. mxEv is an event within the summary, which may therefore change. The (non-member) event prior to the summary will not change, whether it's null (during back-pagination through a number of membership events) or otherwise.

... yeah, what he said 😅

If you're back-paginating, prevEvent will be null, so you use a fixed string: you suggested memberlistsummary-; I am suggesting memberlistsummary-initial.

Once you've stopped back-paginating (or rather, back-paginated past the group of member events), prevEvent is no longer null. With your suggestion, you change the key to memberlistsummary-{prevEvent.getId()}. I am suggesting instead using memberlistsummary-{mxEv.getId()}. It still only changes once: at the point you stop back-paginating.

Am I missing something?

Ahh I understand now, that works for me.

Oh, sorry yep I misunderstood - that makes sense.

richvdh · 2016-12-15T12:26:09Z

src/components/views/elements/MemberEventListSummary.js

-        let summary = null;
+    shouldComponentUpdate: function(nextProps, nextState) {
+        return (
+            nextProps.events.length !== this.props.events.length ||


hrm. this is going to mean that, when the summary is expanded, any updates to the eventTiles in children won't be rerendered. I don't know if that's a problem in practice, but it certainly sounds like a potential future problem.

Maybe always return true if nextState.exanded or this.state.expanded is true, or if there are few events?

I might be missing why a shouldComponentUpdate is actually necessary here. Having removed the O(N^2) stuff in render I think it might be more hassle than it is worth. Or am I missing something?

@kegsay said something along the lines of if the parent gets updated, then it will re-render each child component, but only if shouldComponentUpdate is true. So as an example, it won't now re-render the MemberEventListSummary if a message is added to the scroll panel.

@kegsay said something along the lines of if the parent gets updated, then it will re-render each child component, but only if shouldComponentUpdate is true

I don't think that's correct. I believe it will only re-render children which are returned in the virtual DOM: if the summary is not expanded, the child components are not returned by MemberEventListSummary.render, so their render methods are not called.

So as an example, it won't now re-render the MemberEventListSummary if a message is added to the scroll panel.

yup... but I'm not sure if that's a worthwhile saving, having fixed render.

Sorry, I didn't mean children, I meant inner components. I wasn't referring to the EventTiles passed through, I was referring to the ScrollPanel updating (as the parent) and MemberEventListSummary (as a "child") re-rendering.

yup... but I'm not sure if that's a worthwhile saving, having fixed render.

It's still quite bad for large number of membership events, say 5000, which will be broken several times over when we do initial membership syncing on freenode. It's bad enough waiting for the view to re-render a single summary every time the set of underlying events change, let alone re-rendering every summary every time a message is put into the ScrollPanel or a read receipt changes position.

this is going to mean that, when the summary is expanded, any updates to the eventTiles in children won't be rerendered. I don't know if that's a problem in practice, but it certainly sounds like a potential future problem.

Indeed, but I don't believe this is a problem in practice. Member events don't change after they have been rendered. I agree that this could be very tricky to debug in the future, so I would be up for always returning true if the summary is expanded, leaving it up to the children to decide if they want to re-render or not.

Returning true when expanded sounds like a good idea.

richvdh · 2016-12-15T12:36:32Z

src/components/views/elements/MemberEventListSummary.js

+            if (!userEvents[userId].first) {
+                userEvents[userId].first = e;
+            } else {
+                userEvents[userId].last = e;


why not make this unconditional and remove the if(!lastEvent) { lastEvent = firstEvent; } at line 202?

richvdh · 2016-12-15T16:14:12Z

src/components/views/elements/MemberEventListSummary.js

        return (
            nextProps.events.length !== this.props.events.length ||
-            nextState.expanded !== this.state.expanded
+            this.state.expanded || nextState.expanded


what about the fewEvents case?

Why would we randomly change behaviour for "a few" events?

I thought we auto-expanded if there were "few" events?

So we should treat "few" events the same as "state.expanded"

Then surely we just keep relying on the expanded flag? There's no extra conditionals afaik?

https://github.com/matrix-org/matrix-react-sdk/pull/590/files#diff-43229f1e879e38ff8e94406f31406715R168

fewEvents is based on the length of the events, which when changed will cause an update as per shouldComponentUpdate

yes, but if there are few events, and one of those few events is updated, we ought to do an update, following the same logic as the expanded case.

Oh, yes absolutely, fair point!

…nding the summary

lukebarnard1 requested a review from richvdh December 14, 2016 15:44

richvdh requested changes Dec 15, 2016

View reviewed changes

richvdh assigned lukebarnard1 Dec 15, 2016

Luke Barnard added 3 commits December 15, 2016 13:15

Simplify handling of only one member event

86739e7

Spelling

e7564f4

Allow component to update if currently expanded or if about to collapse

55f85be

richvdh reviewed Dec 15, 2016

View reviewed changes

Luke Barnard added 3 commits December 15, 2016 18:23

Use the first member event ID or "initial" in the MELS key

88aeb64

Update comment on MELS key

6b52b24

MELS component should update if there are fewEvents, effectively expa…

7475056

…nding the summary

richvdh approved these changes Dec 16, 2016

View reviewed changes

richvdh merged commit b8c0fa5 into develop Dec 16, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve the performance of MemberEventListSummary #590

Improve the performance of MemberEventListSummary #590

lukebarnard1 commented Dec 14, 2016

richvdh Dec 15, 2016

richvdh Dec 15, 2016

richvdh Dec 15, 2016

lukebarnard1 Dec 15, 2016 •

edited

richvdh Dec 15, 2016

kegsay Dec 15, 2016

lukebarnard1 Dec 15, 2016 •

edited

richvdh Dec 15, 2016

kegsay Dec 15, 2016

lukebarnard1 Dec 15, 2016

richvdh Dec 15, 2016

lukebarnard1 Dec 15, 2016 •

edited

richvdh Dec 15, 2016

lukebarnard1 Dec 15, 2016 •

edited

kegsay Dec 15, 2016

lukebarnard1 Dec 15, 2016

richvdh Dec 15, 2016

lukebarnard1 Dec 15, 2016

richvdh Dec 15, 2016

kegsay Dec 15, 2016

richvdh Dec 15, 2016

kegsay Dec 15, 2016

richvdh Dec 15, 2016

lukebarnard1 Dec 15, 2016

richvdh Dec 15, 2016

lukebarnard1 Dec 15, 2016

Improve the performance of MemberEventListSummary #590

Improve the performance of MemberEventListSummary #590

Conversation

lukebarnard1 commented Dec 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukebarnard1 Dec 15, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukebarnard1 Dec 15, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukebarnard1 Dec 15, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukebarnard1 Dec 15, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukebarnard1 Dec 15, 2016 •

edited

lukebarnard1 Dec 15, 2016 •

edited

lukebarnard1 Dec 15, 2016 •

edited

lukebarnard1 Dec 15, 2016 •

edited