[Performance] [Audit] ReportActionContextMenu slows down cell rendering time significantly

[jsamr]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

This report is part of #3957, scenario "Rendering Individual chat messages".

Pattern

The full commit log can be inspected with Flipper or React Devtools, see #3957 (comment) for instructions.

This commit chart spans over 1.4s (c77 to c144). Each spike in the chart is a ReportActionContextMenu rendered for one cell.

Flamegraph

This is an excerpt of the 100th commit (c100):

Also note that those spikes could be caused by the heavy usage of react-native-svg, which can impede performance. Quoting William Candillon

Using SVG heavily in an app can degrade its performance, therefore use it with parsimony."

Proposal: render only one context menu, change dynamically its content on press

Instead of rendering ReportActionContextMenu for every cell, render only one menu at the root of the screen. When a report action is pressed, pass the parameters required to update the view to a callback and render the content accordingly. The callback can be shared via a React context.

[MelvinBot]

Triggered auto assignment to @stephanieelliott (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

[jsamr]

@parasharrajat I agree with you; too much DOM event subscribers is a bad pattern and most certainly impedes performance. Much better to have one subscriber with a state shared in a top-level React context!

[jsamr]

@parasharrajat The great advantage of one context to share a state is that it guarantees React fiber will batch every update related to a dimension event in the same commit. Whereas when updating each component because of a DOM event, updates might happen in short bursts of commits which is most likely slower.

[marcaaron]

This one makes perfect sense to me, but unsure how to prioritize.

I think @roryabraham might have the most context on this feature. Any thoughts on how difficult this would be to unwind? I think this could end up being a pretty significant refactor, but since we now understand the importance of keeping virtualized list items simple it feels like it needs to be done.

Ideas for benchmarking this one:

Disable the context menu code and just render a chat without the extra bells and whistles we added. What kinds of performance improvements do we see?

[marcaaron]

@parasharrajat @kidroca Thanks for the extra context. Just a heads up I'm gonna clean these comments up a bit since I think this issue is broader than the withWindowDimensions HOC. If we think the issue over there is worth reopening we can continue conversation here. 🙇

[roryabraham]

Actually, I did sort of try this once before, albeit for a different reason. My implementation caused an undesirable wiggling effect with the mini ReportActionContextMenu that appears on hover as its position is dynamically calculated.

So my thoughts on this is that if we move forward with this, we should:

1. Break apart the ReportActionContextMenu into two separate components – a very lean simple one called MiniReportActionContextMenu that is basically the existing ReportActionContextMenu with the isMini prop, and another one that displays a modal popover.
   • MiniReportActionContextMenu could probably strip out a fair amount of the expensive layout measuring code.
   • For now, the MiniReportActionContextMenu could probably be a noop on native platforms, because it only ever appears on hover anyways. That way I think RN might be smart enough to optimize it out of the render tree entirely.
2. Keep MiniReportActionContextMenu as a child of the ReportActionItem (avoid the wiggle effect).
3. Make a single PopoverReportActionContextMenu a sibling of this InvertedFlatList, as this issue suggests, and calculate its position when the modal is meant to open.

[marcaaron]

Did some A/B testing with context menu logic removed and it looks like we could maybe shave off about ~500ms from every chat switch by refactoring this. Here are some rough numbers (also keep in mind this was in iOS simulator + using the production API)...

Avg. Chat Switch Time w/ Context Menu	Avg. Chat Switch Time w/ Context Menu removed	Difference
1332.25 ms	901.75 ms	430.5 ms

That seems pretty signifcant since anything over 1 second is going to feel very slow.

[parasharrajat]

Is Anyone working on this? If not, I am up for the task. @roryabraham has laid a good foundation of what needs to be done here #4106 (comment)

[marcaaron]

No and @roryabraham plan sounds good to me.

[MelvinBot]

Triggered auto assignment to @bondydaa (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[stephanieelliott]

Moving this along, @bondydaa sounds like the proposed solution is here: #4106 (comment)

[kidroca]

IMO it's possible to extract this entirely from the action item, and instead of having a menu item/s per each action item have the context menu at the root level. You can use an event associated with the action item in question e.g. on long press - position and provide context to the popup menu

[parasharrajat]

That is a plan all along.

[quinthar]

Does that mean that you are currently working on this? Is this your top focus? When is your estimate that it will be complete?

…

On Wed, Jul 21, 2021, 1:02 PM Rajat Parashar ***@***.***> wrote: That is a plan all along. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#4106 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEMNUWLBHZXYHG3GJEBGGLTY4RTRANCNFSM5APSP5EQ> .

[parasharrajat]

@quinthar I have not started yet. Need a 🟢 signal. But I expect it achieve in 3-4 days from the start date.

[quinthar]

Marc does he have the green light? If not what is blocking it?

…

On Wed, Jul 21, 2021 at 1:19 PM Rajat Parashar ***@***.***> wrote: @quinthar <https://github.com/quinthar> I have not started yet. Need a 🟢 signal. But I expect it achieve in the mid of next week. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4106 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEMNUS6SUGKZ427DXXLZKDTY4TVRANCNFSM5APSP5EQ> .

[marcaaron]

@quinthar I will expedite this.

[parasharrajat]

Hi @marcaaron. PR is ready for review. Thank you.

[marcaaron]

Quick update here. We are seeing great progress with the PR opened by Rajat. It should be on staging soon!

Especially on Android where the chats are opening 0.7 seconds faster 🤯

Separately, I think I will look into further improving the metrics for chat switch time so we can track it in Firebase alongside the other critical metrics that have been recently set up.

Big success ! Great job everyone 🙇

[quinthar]

Can't wait!!

…

On Thu, Jul 29, 2021 at 11:21 AM Marc Glasser ***@***.***> wrote: Quick update here. We are seeing great progress with the PR opened by Rajat. It should be on staging soon! Especially on Android where the chats are opening 0.7 seconds faster 🤯 Separately, I think I will look into further improving the metrics for chat switch time so we can track it in Firebase alongside the other critical metrics that have been recently set up. Big success ! Great job everyone 🙇 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4106 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEMNUUEF4IJT5ZVRT5TYJDT2GL3FANCNFSM5APSP5EQ> .

[MelvinBot]

@parasharrajat, @marcaaron Eep! 4 days overdue now. Issues have feelings too...

[parasharrajat]

PR is merged and we are waiting for the regression period to be over before closing it.

[parasharrajat]

@Expensify/contributor-management any update on this task?

[MelvinBot]

Triggered auto assignment to @Christinadobrzyn (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[roryabraham]

@Christinadobrzyn Seems like maybe we didn't follow the normal process here. If I'm not mistaken, @parasharrajat submitted a PR for this issue without first getting hired on Upwork. So we should create the Upwork job and make sure @parasharrajat is paid for his work.

[parasharrajat]

Sorry for the miscommunication here but there is an active job that needs to be paid out. https://www.upwork.com/jobs/ReportActionContextMenu-slows-down-cell-rendering-time-significantly_%7E01fc756cf8f4b57ccb

[marcaaron]

I gotchu @parasharrajat