-
Notifications
You must be signed in to change notification settings - Fork 359
fix: performance improvement for message list render item #3306
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
SDK Size
|
…ve into perf/message-list-render-item
…ve into perf/message-list-render-item
isekovanic
left a comment
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 looks like the right direction from what we talked about, at least as an initial first pass !
Let's please make sure that:
- New messages do not cause
renderItemto change (i.e in the profiler "reason for change" suspects, should be the case as all dependencies of it now look stable) - New messages do not cause heavy downstream rerenders (I believe you already got this to some point from what we spoke last time)
Also, please benchmark with read events enabled (otherwise this won't make any difference).
…ve into perf/message-list-render-item
So,
I did test it on profiler and doesn't look bad for now.
Yeah number of re-renders are significantly less compared to develop branch
I enabled the events on our benchmark app and tested but didn't see anything worse. NOTE: I will request you to run the change as well so that if I am missing out on something around performance degradation, you can point me out. |
The PR focuses into optimising the behaviour of our
MessageList/MessageFlashList. As a result of it moverenderItemcompletely out of theMessageListandMessageFlashList. The changes mostly are as follows:getGroupStylesandgetDateSeparatorsto do the same job.renderItemjust was dependent on stable props so I decided to create a contextMessageListItemContextfor the same and use it insideMessageWrappercomponent thereby moving therenderItemout of the MessageList which was always our goal.This obviously came with a couple of deprecations which we cannot remove right now but may be in the future which will make the code cleaner and faster as we get rid of the unnecessary calculations we still need to have within the code.