Skip to content
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

[Performance] [Audit]: messages with Views nested in Text for inline code are 700% slower to render on Android #4126

Closed
jsamr opened this issue Jul 16, 2021 · 6 comments
Assignees
Labels

Comments

@jsamr
Copy link
Collaborator

jsamr commented Jul 16, 2021

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".

Flamegraph

This flamegraph shows commit c153. The full commit log is available HERE to inspect in Flipper / React Devtools (you will have to unzip it first).

image

Commit c153 refers to one RenderHtml component with 20 words wrapped in inline code. Something like this:

`inline code spanning multiple lines inline code spanning multiple lines inline code spanning multiple lines inline code spanning multiple lines`

Its rendering time is 112ms (yellow bar in chart below). All other messages (green bars), including some messages with as many words are rendered between 8 to 14ms. So that's a minimal overhead of 112 / 14 = 8 or +700%.

image

Proposal: Review InlineCode

Review if the design requirements that have led to implementation of #2527 are worth the rendering cost. You might want to profile this on iOS also.

@jsamr jsamr added the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Jul 16, 2021
@MelvinBot
Copy link

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

@MelvinBot MelvinBot removed the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Jul 16, 2021
@jsamr jsamr changed the title [Performance] [Audit]: messages with views nested in Text for inline code are 700% slower to render on Android [Performance] [Audit]: messages with Views nested in Text for inline code are 700% slower to render on Android Jul 16, 2021
@marcaaron
Copy link
Contributor

Interesting. I'm not sure if this is a mainline case that we need to prioritize for inline code blocks are kind of a power user feature. I think we are more generically concerned with the typical usage scenarios which is probably ... not using any markdown at all. But not too sure. Will let others share their thoughts on this.

700% sounds pretty bad but we can also test the performance here...

Benchmarking idea:

  1. Create two chats. 1 with no inline comments and one with several multi line inline code comments.
  2. Compare the time to switch between both on Android.

Does it take an excessive amount of time to render the chat with inline code blocks?

@marcaaron marcaaron changed the title [Performance] [Audit]: messages with Views nested in Text for inline code are 700% slower to render on Android [HOLD] [Performance] [Audit]: messages with Views nested in Text for inline code are 700% slower to render on Android Aug 3, 2021
@MelvinBot MelvinBot added the Monthly KSv2 label Aug 4, 2021
@laurenreidexpensify laurenreidexpensify added Daily KSv2 Monthly KSv2 and removed Monthly KSv2 Daily KSv2 labels Aug 17, 2021
@laurenreidexpensify
Copy link
Contributor

This is still on hold

@MelvinBot MelvinBot removed the Overdue label Sep 17, 2021
@laurenreidexpensify
Copy link
Contributor

Will review this when N6 hold is off

@laurenreidexpensify laurenreidexpensify changed the title [HOLD] [Performance] [Audit]: messages with Views nested in Text for inline code are 700% slower to render on Android [Performance] [Audit]: messages with Views nested in Text for inline code are 700% slower to render on Android Oct 22, 2021
@laurenreidexpensify
Copy link
Contributor

laurenreidexpensify commented Oct 22, 2021

Am going to close this one, as our finding work flow detailed here all issues should be reported in Slack before being added to GitHub as issues. @jsamr can you please report this in the Slack channel if you would like to discuss it further. Thank you.

@parasharrajat
Copy link
Member

@laurenreidexpensify This was created months back. back than there was no such policy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants