[HOLD for payment 2021-09-06] [HOLD for payment 2021-09-01] [Performance] [Audit]: using react-native-render-html composite architecture could speed up render time of short report actions by 32%

[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!

---

Upwork job link: https://www.upwork.com/jobs/~0157cdae32a7b23e69
This report is part of #3957, scenario "Rendering Individual chat messages".

Flamegraph

Basically, using the explicit composite architecture means that all components on top of RenderHtmlSource, starting with TRenderEngineProvider, would be put either at the root of the ReportScreen, or near the root of the application instead of being replicated for each report action.

In the above flamegraph, the rendering time of those composite components is 7.3ms - 5ms = 2.3ms. If those component were factored out, it would mean a net gain of 2.3 / 7.3 = 32%.

Proposal: implement the explicit composite architecture

See https://meliorence.github.io/react-native-render-html/docs/flow/rendering#composite-rendering-architecture

[MelvinBot]

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

[marcaaron]

32% sounds like a lot but also is this 32% of 7ms total or per each comment in the chat? (I think it must be the latter)

Maybe there is a way to dumb this down, but it's hard for me to wrap my head around what this means for the perspective of loading a chat that has something like 50-100 messages.

So, let's say we are rendering 100 when the chat loads and there is 730ms of time spent here - that would then become 230ms? Seems pretty significant if it was happening in sequence, but we are also rendering in parallel? Is there some easy way to tell how much user time this will save once implemented?

[jsamr]

@marcaaron

is this 32% of 7ms total or per each comment in the chat?

Per message indeed.

So, let's say we are rendering 100 when the chat loads and there is 730ms of time spent here - that would then become 230ms?

It would be my understanding that it would save up 230ms in this scenario; not sure I'm understanding the parallel thing?

Is there some easy way to tell how much user time this will save once implemented?

I think your 230ms estimate is reasonable for a 100 messages scenario!

[jsamr]

PS: I used beta-8 branch for this flamegraph; I previously noticed a bug in alpha-10 (main branch) where RenderHtml was rendered twice. Did not reproduce in beta-8 thus did not bother report 😅

[marcaaron]

Got it. I think my question makes no sense because the actual rendering is not happening asynchronously and when we're talking about "rendering time of those composite components" only one can be rendering at a time because JavaScript.

[jsamr]

@marcaaron I'm ready to have a look into this one this week. Ping me when a Job is ready on Upwork!

[MelvinBot]

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

[michaelhaxhiu]

I'll work on posting this job to upwork shortly. Let me double check with @puneetlath and @marcaaron a few things and drop a note here after.

[michaelhaxhiu]

Okie dokie. @jsamr we'd like to add benchmarking to this request to help show the before/after-effects of the enhancement. Do you have any recommendations for that aspect?

[jsamr]

@michaelhaxhiu That's perfectly reasonable. What I propose is to create a separate git repository for benchmarking explicit composite vs implicit composite with the react profiler. What I intend to do is display 100 small distinct HTML snippets in a ScrollView an measure the total render time of this view. What I like with this approach is it's reproducible and transparent.

I could also do the profiler measurement within Expensify app. However, I'm not sure how to handle lags caused by Onyx in such case, which might interfere with the results.

[marcaaron]

I could also do the profiler measurement within Expensify app. However, I'm not sure how to handle lags caused by Onyx in such case, which might interfere with the results.

To clarify, are you concerned that lags with Onyx will obscure the benefit? To be honest, a separate benchmark repository doesn't sound as useful to me as measurements done within our app. I may be able to help here. I have a pretty decent manual test that will reveal whether something is improving report render time or not.

[jsamr]

@marcaaron Yes I was a bit worry of that; I'd by happy to see your manual testing strategy; I was also thinking I might sample the render time of each RenderHTML component instead of each cell of the flat list; and compare before / after. I would need a list with lots of items preferably. In our initial rationale, we reasoned about 100 items, but now I am thinking that windowSize (default 21) FlatList prop will limit the number of rendered items, while the benefits could still be sensible when a user scrolls rapidly. Do you think I should set this window size to 100 while benchmarking?

[marcaaron]

I'd by happy to see your manual testing strategy

It's tedious but effective and I don't have anything better yet but... I build the release native app then time when the FlatList onLayout is called (I know, not perfect) and log this to our log server. Then switch two chats back and forth about 10 times or so, average them out, and compare before and after.

Do you think I should set this window size to 100 while benchmarking?

I would maybe do two tests. One where we keep the FlatList props as they are and one where we try to optimize for scrolling performance?

In my limited experience so far it really seems like initialNumToRender + time to render a cell has the greatest impact on time to layout for the FlatList. We are mainly trying to optimize the chat switch time over scrolling performance at the moment, but this will certainly change after we are happy with the initial load time.

[michaelhaxhiu]

Job is posted on Upwork. @jsamr feel free to apply and I'll accept you asap - https://www.upwork.com/jobs/~0157cdae32a7b23e69

[jsamr]

@marcaaron Sorry for my delayed response! NP for the manual testing; I'm ready to tackle this one next week!

[michaelhaxhiu]

@jsamr is hired for this job in Upwork

[MelvinBot]

Triggered auto assignment to @marcaaron (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[michaelhaxhiu]

Removing the Help Wanted label as this is already assigned.

[MelvinBot]

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

[marcaaron]

Just testing out the draft branch now with manual testing. Seems like it's about 20ms faster on average to switch chats with the new branch (so pretty small difference). Unsure if there's some other test we want to do here like the one suggested here.

[michaelhaxhiu]

@marcaaron should we start the 7 day counter for payment, or is this going to be re-opened for additional related work?

[michaelhaxhiu]

Going to send payment, as no regressions have been identified. Sorry for the wait @jsamr it's been a busy few weeks on this side :)

[jsamr]

@michaelhaxhiu no worries!