-
-
Notifications
You must be signed in to change notification settings - Fork 832
Improve the performance of MemberEventListSummary #590
Conversation
- 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.
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/necessarily/unnecessarily/?
// 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() : ""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because when back paginating, the first membership event should change, and prevEvent should be consistently null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh I understand now, that works for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, sorry yep I misunderstood - that makes sense.
let summary = null; | ||
shouldComponentUpdate: function(nextProps, nextState) { | ||
return ( | ||
nextProps.events.length !== this.props.events.length || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I didn't mean children, I meant inner components. I wasn't referring to the EventTile
s 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returning true when expanded sounds like a good idea.
if (!userEvents[userId].first) { | ||
userEvents[userId].first = e; | ||
} else { | ||
userEvents[userId].last = e; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not make this unconditional and remove the if(!lastEvent) { lastEvent = firstEvent; }
at line 202?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WFM
return ( | ||
nextProps.events.length !== this.props.events.length || | ||
nextState.expanded !== this.state.expanded | ||
this.state.expanded || nextState.expanded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about the fewEvents case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would we randomly change behaviour for "a few" events?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we auto-expanded if there were "few" events?
So we should treat "few" events the same as "state.expanded"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then surely we just keep relying on the expanded flag? There's no extra conditionals afaik?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fewEvents
is based on the length of the events, which when changed will cause an update as per shouldComponentUpdate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, yes absolutely, fair point!
shouldComponentUpdate
so that it only updates if it is given a different number of events to previous or if it is toggled to expand.