MM-35509 Do not use getDerivedStateFromProps to determine picker placement and instead use memoization

[jwilander]

Summary

A customer is experiencing slow typing issues. A JS profile showed the following:

A very significant amount of time is spent on "Recalculate Style" and the primary trigger for this seems to be the getBoundingClientRect function calls inside the EmojiPickerOverlay component. After some brief investigation it was apparent that on every single character typed into the textbox getBoundingClientRect was being called twice due to improper use of getDerivedStateFromProps. I have used memoization to properly implement the functionality in a much more performant way.

Unfortunately, I was unable to reproduce the exact slow typing issue the customer is experiencing so I'm not sure how much effect this will have or if this is the root cause of the issues or not.

Ticket Link

https://mattermost.atlassian.net/browse/MM-35509

Release Note

Performance improvement to emoji picker overlay that should improve typing performance.

[Willyfrog]

I'm not sure this would help, if function getBoundingRect is the culprit and is being called twice with the same target, wouldn't it be better to wrap and only memoize target.getBoundingRect?
if you memoize the parent call you'd still be calling getBoundingRect twice as they are two different functions.

so something like this might be enough to alleviate the double call:

const getTargetRect = memoize((target) => target.getBoundingRect());

my only concern with that approach is that I'm not entirely sure what target is but most likely an object, if that's the case we'll need to do a custom equality function based on the object to figure out when two call should yield the same result.

edit: digging a bit deeper, seems the overlay is called with a function that returns a reactRef which I think it uses the htmldivelement type. If so we can probably use isSameNode or isEqualNode to do a proper comparison.

edit 2: at least one of the calls is still using a stringRef instead of createRef, since we havent yet finished the migration campaign, so in order for this to work we would need to migrate edit_post_modal.jsx as well

[jwilander]

Thanks for the feedback @Willyfrog

I'm not sure this would help, if function getBoundingRect is the culprit and is being called twice with the same target, wouldn't it be better to wrap and only memoize target.getBoundingRect?
if you memoize the parent call you'd still be calling getBoundingRect twice as they are two different functions.

This helps because before this change on every single character typed getDerivedStateFromProps was running multiple times, even though the render function of EmojiPickerOverlay was not running at all. That's because the parent of EmojiPickerOverlay was rendering (e.g. CreatePost) and that triggers getDerivedStateFromProps even if the props didn't change and no render occurs. So getBoundingClientRect would be called multiple times for every key press. With this change it's only called on render and if the props of EmojiPickerOverlay change. Even if we removed the memoization it would still prevent getBoundingClientRect being called on every key press.

I'm not too worried about the double call when we do actually render, though your code would be a further improvement to prevent that.

getBoundingClientRect is a problem because it can trigger the browser to recalculate style and check the layout. It seems to be doing that way more often for the customer than my local or community testing.

my only concern with that approach is that I'm not entirely sure what target is but most likely an object, if that's the case we'll need to do a custom equality function based on the object to figure out when two call should yield the same result.

edit: digging a bit deeper, seems the overlay is called with a function that returns a reactRef which I think it uses the htmldivelement type. If so we can probably use isSameNode or isEqualNode to do a proper comparison.

edit 2: at least one of the calls is still using a stringRef instead of createRef, since we havent yet finished the migration campaign, so in order for this to work we would need to migrate edit_post_modal.jsx as well

Are we sure a strict comparison is not enough? If the React node is re-created and the ref points to a new node does the value not change? Can that even happen in this case without the EmojiPickerOverlay also being re-created?

[jwilander]

@jgilliam17 for testing we just need to make sure that all the places the Emoji Picker can open, it does so the same as it did before (e.g. which placement it has).

[Willyfrog]

This helps because before this change on every single character typed getDerivedStateFromProps was running multiple times, even though the render function of EmojiPickerOverlay was not running at all. That's because the parent of EmojiPickerOverlay was rendering (e.g. CreatePost) and that triggers getDerivedStateFromProps even if the props didn't change and no render occurs. So getBoundingClientRect would be called multiple times for every key press. With this change it's only called on render and if the props of EmojiPickerOverlay change. Even if we removed the memoization it would still prevent getBoundingClientRect being called on every key press.

I agree with this, I was mostly answering on your initial comment about it being called multiple times, but I didn't check where it was being called 👍

Are we sure a strict comparison is not enough? If the React node is re-created and the ref points to a new node does the value not change? Can that even happen in this case without the EmojiPickerOverlay also being re-created?

A strict comparison would also work (it won't work with the default comparison from memoize as the author says it is a shallow one), but if the object already provides a comparison method I usually prefer to trust the one who created object as a better/more efficient way.

anyways, I'll aprove the change 😄

[lieut-data]

Nice analysis, @jwilander! Seems like as solid change, even if we can't tie it directly to the performance issues.

[jgilliam17]

Thanks @jwilander
Tested, looks good to merge.

• Verified Emoji Picker placement when opening in main channel text box, edit post, on RHS etc.

[mm-cloud-bot]

Test server destroyed

[mattermod]

Cherry pick is scheduled.

[amyblais]

/cherry-pick release-5.35

[mattermod]

Cherry pick is scheduled.