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] Conduct React Native Performance Audit #3957

Closed
jsamr opened this issue Jul 9, 2021 · 11 comments
Closed

[Performance] Conduct React Native Performance Audit #3957

jsamr opened this issue Jul 9, 2021 · 11 comments
Assignees
Labels
Engineering Improvement Item broken or needs improvement. Weekly KSv2

Comments

@jsamr
Copy link
Collaborator

jsamr commented Jul 9, 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 is a follow-up on a conversation I had with @puneetlath. I propose a React performance audit by investigating best practices in load-heavy scenarios. I am planing to use tools such as:

  • React Devtools via Flipper for flamegraphs at critical render-points (such as switching conversations);
  • React Native Performance and its Flipper plugin;
  • Why-did-I-render to make sure tree updates are parsimonious and optimized;
  • Native tooling for memory and CPU usage;

Main objectives:

  • Identify bottlenecks and area of improvements;
  • App footprint assessments.

Deliverable:

The conclusions and analysis will be delivered in the form of a technical report.

Additional info

For best results, I'd like detailed steps to reproduce scenarios which are known to cause performances issues.

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

Triggered auto assignment to @sophiepintoraetz (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 9, 2021
@sophiepintoraetz
Copy link
Contributor

Hey @jsamr - can you please provide some more context on:

- Identify bottlenecks and area of improvements;
Are there going to be recommendations on actions to take for improvements? Who needs to implement them?

- The conclusions and analysis will be delivered in the form of a technical report.
How often will this happen? Who receives this report? How do you ensure that any of the improvements have been implemented? Could we not create a GH assignment, much like we've done here, for any tasks that need to be completed?

@sophiepintoraetz sophiepintoraetz added Weekly KSv2 Improvement Item broken or needs improvement. labels Jul 13, 2021
@jsamr
Copy link
Collaborator Author

jsamr commented Jul 13, 2021

Hi @sophiepintoraetz! Puneet suggested that I open an issue so that he can link an upwork job (I'm a freelancer).

Are there going to be recommendations on actions to take for improvements? Who needs to implement them?

There will probably be! But who will implement them is not my call. I think all of this will be worked out by Puneet directly, or by whoever should be considered the referral, @marcaaron perhaps?

How often will this happen? Who receives this report?

This should be a one-shot work; all the other questions are rather internal so I'll let your team figure it out :-)

@quinthar
Copy link

This sounds a bit ambiguous and open-ended,, and thus unlikely to produce any actual optimizations for many days or even weeks, so this can't be our top proposal for solving the current performance emergency.

However, it feels like we are kind of desperate right now and grasping at straws, so I am in total support of getting started on this right away even though I'm a little unclear on what exactly it's going to come out of it.

@quinthar
Copy link

Can you confirm that you are going to run this on all four platforms, and produce a prioritized list of recommendations that are sufficiently specific that we can turn them into projects immediately after?

@jsamr
Copy link
Collaborator Author

jsamr commented Jul 13, 2021

@quinthar

This sounds a bit ambiguous and open-ended

I agree it's somehow generic and exploratory ; the idea was to investigate performance in well-defined scenarios and pinpoint identifiable bottlenecks. I see three potential causes:

  • JavaScript / native bridge;
  • Extreneous re-renders;
  • Long render times (which often come up to too many items to render, a bridge bottleneck, slow I/O operations).

Can you confirm that you are going to run this on all four platform

Yes, although I would assume that electron and web can be conflated.

produce a prioritized list of recommendations that are sufficiently specific that we can turn them into projects immediately after

Sure. I'll state which code area is at stake and to which part of React Native architecture it belongs.

@sophiepintoraetz
Copy link
Contributor

Sounds like more discussion to be had - but I feel like @puneetlath would be a better triager here / assigning to engineering to field the discussion.

@MelvinBot
Copy link

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

@marcaaron
Copy link
Contributor

Oh yeah so to avoid further confusion, we've discussed this a bit already and asked @jsamr to create this issue.
Gonna assign @puneetlath since he has the most context :)

@marcaaron marcaaron assigned puneetlath and marcaaron and unassigned MariaHCD Jul 14, 2021
@marcaaron marcaaron changed the title Performance audit [Performance] Conduct React Native Performance Audit Jul 14, 2021
@jsamr
Copy link
Collaborator Author

jsamr commented Jul 15, 2021

Performance report, scenario "Rendering Individual chat messages"

To inspect the profiling session, unzip and open the attached report with flipper or React Devtools. Go to Profiler (top right of the screen), and use the arrow up button to import the JSON session file . Also, make sure that you're not filtering commits below 1ms (use the gear menu → profiler). REPORT

Remark: This scenario conflates a bit with "Switching from one chat to another", but the later will be on heavy-load conditions and focus on FlatList performances and other metrics.

Platform: Android (findings are related to react internals, and should be platform-independent)

Reproduction

  1. Open a discussion (concierge for example).
  2. Get back to the component displaying lists of discussions.
  3. Start profiling.
  4. Open another discussion.
  5. Wait for the list of message to load.
  6. Stop profiling.

You'll notice the report has nearly 300 commits for a small discussion list of 14 report actions! This is huge and should be a matter of concern and focus. More specific findings in separate issues coming very soon. The total render time was 6s (profiling did most certainly add overhead).

Because there are a lot of findings and I don't want the discussion to spread too much, I will add references to distinct findings as they come.

@puneetlath
Copy link
Contributor

It seems like the initial investigation here is complete and all further discussion is happening in the individual issues. I'm going to go ahead and pay this and close it out. Thanks @jsamr!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering Improvement Item broken or needs improvement. Weekly KSv2
Projects
None yet
Development

No branches or pull requests

7 participants