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

Refactor BalanceCoinRow into FastBalanceCoinRow and FastCoinIcon #3313

Merged
merged 75 commits into from Jun 17, 2022

Conversation

terrysahaidak
Copy link
Contributor

@terrysahaidak terrysahaidak commented May 9, 2022

Fixes RNBW-3350

What changed (plus any additional context for devs)

Refactored BalanceCoinRow into its own family of components - Fast components - FastBalanceCoinRow. Fast components are meant to be used in Lists, especially in Recyclable lists.

Also here I made some optimizations to hooks and selectors that build the wallet screen asset list. Now it's called only when it receives new data. But also it's created on the WalletScreen only and we pass data to the next screens so we don't call the same hook 3 times in different places.

Another change - I removed stagging for the assets list since we don't need it anymore. The render of the asset list items is fast enough.

What "FastComponent" means

The "Fast" part means that the component is as fast to render/update as it can be. We are trying to use as less of dynamic and heavy stuff as possible. Because of that, we don't use the Design System for layout except for Text.

So in order to make a Fast Component, you should make sure that:

  • we don't use selectors inside of the component
  • we receive data from the props
  • we calculate data before the render
  • we use as less views as possible to achieve the design
  • we don't render things that are not visible and hiding them with opacity
  • we use raw views and StyleSheet API
  • we don't use useContext (useTheme. useNavigation etc) for these
  • we pass props down
  • the component is as flat as possible
  • the component is as simple as possible
  • the component is not "reusable" - if you need to reuse it in a different place - make a new one
  • we don't waste any CPU time

Before doing any refactoring - measure the mount and update time with RenderProfiler.
Also, I noticed that refactoring old stuff that is used in many places and "reusable" is not worth it and it's easier to just create a brand new component instead.

We're using RenderProfiler to measure mount and update time. Since it's RecyclerViewList the mount - is when we show a full list of assets, the update is when we scroll since it's passing new props instead of remounting the items.

All the tests were done on Samsung A12 2021 in Release but with the Profiling version of RN (a tiny bit slower).

Before results:

[10:40:56] I | ReactNativeJS ▶︎ mount: BalanceCoinRow (5) - 52.28ms (max: 67.51ms; min: 46.45ms)

[10:40:59] I | ReactNativeJS ▶︎ update: BalanceCoinRow (1) - 4.94ms (max: 4.94ms; min: 4.94ms)

[10:41:00] I | ReactNativeJS ▶︎ update: BalanceCoinRow (1) - 0.57ms (max: 0.57ms; min: 0.57ms)

[10:41:08] I | ReactNativeJS ▶︎ update: BalanceCoinRow (44) - 4.89ms (max: 40.48ms; min: 0.04ms)

[10:41:09] I | ReactNativeJS ▶︎ update: BalanceCoinRow (1) - 0.56ms (max: 0.56ms; min: 0.56ms)

[10:41:10] I | ReactNativeJS ▶︎ update: BalanceCoinRow (5) - 0.04ms (max: 0.05ms; min: 0.04ms)

[10:41:12] I | ReactNativeJS ▶︎ mount: BalanceCoinRow (11) - 45.42ms (max: 58.86ms; min: 40.19ms)

[10:41:14] I | ReactNativeJS ▶︎ mount: BalanceCoinRow (11) - 44.99ms (max: 59.23ms; min: 40.02ms)

[10:41:14] I | ReactNativeJS ▶︎ update: BalanceCoinRow (85) - 2.82ms (max: 30.92ms; min: 0.04ms)

[10:41:16] I | ReactNativeJS ▶︎ update: BalanceCoinRow (27) - 5.94ms (max: 18.02ms; min: 3.56ms)

[10:41:23] I | ReactNativeJS ▶︎ mount: BalanceCoinRow (11) - 61.14ms (max: 155.05ms; min: 48.26ms)

[10:41:30] I | ReactNativeJS ▶︎ update: BalanceCoinRow (197) - 21.88ms (max: 267.78ms; min: 0.60ms)

[10:41:30] I | ReactNativeJS ▶︎ update: BalanceCoinRow (1) - 3.47ms (max: 3.47ms; min: 3.47ms)

[10:41:36] I | ReactNativeJS ▶︎ update: BalanceCoinRow (64) - 24.34ms (max: 47.29ms; min: 3.83ms)

[10:41:41] I | ReactNativeJS ▶︎ update: BalanceCoinRow (106) - 25.81ms (max: 161.06ms; min: 0.58ms)

[10:41:44] I | ReactNativeJS ▶︎ update: BalanceCoinRow (90) - 24.23ms (max: 57.63ms; min: 5.63ms)

[10:42:01] I | ReactNativeJS ▶︎ update: BalanceCoinRow (76) - 0.12ms (max: 1.57ms; min: 0.04ms)

[10:42:51] I | ReactNativeJS ▶︎ update: BalanceCoinRow (6) - 1.49ms (max: 1.71ms; min: 1.12ms)

[10:42:54] I | ReactNativeJS ▶︎ update: BalanceCoinRow (83) - 21.03ms (max: 54.97ms; min: 0.61ms)

[10:42:55] I | ReactNativeJS ▶︎ update: BalanceCoinRow (1) - 6.29ms (max: 6.29ms; min: 6.29ms)

[10:42:59] I | ReactNativeJS ▶︎ update: BalanceCoinRow (79) - 24.52ms (max: 46.80ms; min: 5.60ms)

[10:43:01] I | ReactNativeJS ▶︎ update: BalanceCoinRow (38) - 0.47ms (max: 10.31ms; min: 0.04ms)
[17:59:48] I | ReactNativeJS ▶︎ mount: CoinIcon (5) - 14.55ms (max: 24.36ms; min: 10.97ms)

[17:59:51] I | ReactNativeJS ▶︎ mount: CoinIcon (4) - 13.82ms (max: 21.50ms; min: 10.52ms)

[17:59:51] I | ReactNativeJS ▶︎ mount: CoinIcon (11) - 11.30ms (max: 13.12ms; min: 9.77ms)

[17:59:59] I | ReactNativeJS ▶︎ mount: CoinIcon (17) - 10.64ms (max: 12.88ms; min: 9.19ms)

[18:00:00] I | ReactNativeJS ▶︎ update: CoinIcon (62) - 1.34ms (max: 6.08ms; min: 0.04ms)

After results:

[10:29:52] I | ReactNativeJS ▶︎ mount: FastBalanceCoinRow (5) - 20.35ms (max: 21.94ms; min: 18.41ms)

[10:29:55] I | ReactNativeJS ▶︎ update: FastBalanceCoinRow (3) - 1.71ms (max: 1.78ms; min: 1.63ms)

[10:30:02] I | ReactNativeJS ▶︎ update: FastBalanceCoinRow (17) - 2.80ms (max: 7.70ms; min: 0.23ms)

[10:30:07] I | ReactNativeJS ▶︎ update: FastBalanceCoinRow (15) - 1.81ms (max: 6.59ms; min: 0.03ms)

[10:30:07] I | ReactNativeJS ▶︎ mount: FastBalanceCoinRow (11) - 15.98ms (max: 18.47ms; min: 12.98ms)

[10:30:07] I | ReactNativeJS ▶︎ update: FastBalanceCoinRow (10) - 1.87ms (max: 2.47ms; min: 0.83ms)

[10:30:08] I | ReactNativeJS ▶︎ update: FastBalanceCoinRow (16) - 0.04ms (max: 0.05ms; min: 0.03ms)

[10:30:08] I | ReactNativeJS ▶︎ mount: FastBalanceCoinRow (17) - 17.18ms (max: 34.08ms; min: 14.58ms)

[10:30:09] I | ReactNativeJS ▶︎ update: FastBalanceCoinRow (17) - 1.70ms (max: 1.83ms; min: 0.68ms)

[10:30:12] I | ReactNativeJS ▶︎ mount: FastBalanceCoinRow (7) - 20.42ms (max: 22.80ms; min: 17.99ms)

[10:30:12] I | ReactNativeJS ▶︎ mount: FastBalanceCoinRow (1) - 18.73ms (max: 18.73ms; min: 18.73ms)

[10:30:13] I | ReactNativeJS ▶︎ update: FastBalanceCoinRow (8) - 1.87ms (max: 2.28ms; min: 0.89ms)

[10:30:13] I | ReactNativeJS ▶︎ mount: FastBalanceCoinRow (6) - 21.32ms (max: 22.47ms; min: 19.77ms)

[10:30:14] I | ReactNativeJS ▶︎ mount: FastBalanceCoinRow (1) - 19.15ms (max: 19.15ms; min: 19.15ms)

[10:30:15] I | ReactNativeJS ▶︎ update: FastBalanceCoinRow (81) - 5.83ms (max: 14.57ms; min: 0.63ms)

[10:30:17] I | ReactNativeJS ▶︎ update: FastBalanceCoinRow (68) - 6.99ms (max: 28.48ms; min: 1.68ms)

[10:30:20] I | ReactNativeJS ▶︎ update: FastBalanceCoinRow (137) - 6.64ms (max: 32.52ms; min: 1.59ms)

[10:30:23] I | ReactNativeJS ▶︎ update: FastBalanceCoinRow (125) - 6.65ms (max: 26.71ms; min: 1.66ms)

[10:30:25] I | ReactNativeJS ▶︎ update: FastBalanceCoinRow (57) - 7.17ms (max: 23.26ms; min: 1.60ms)

[10:30:28] I | ReactNativeJS ▶︎ update: FastBalanceCoinRow (68) - 6.07ms (max: 12.95ms; min: 1.51ms)
[18:08:16] I | ReactNativeJS ▶︎ mount: FastCoinIcon (5) - 6.01ms (max: 7.19ms; min: 4.98ms)

[18:08:18] I | ReactNativeJS ▶︎ update: FastCoinIcon (3) - 1.43ms (max: 1.44ms; min: 1.43ms)

[18:08:26] I | ReactNativeJS ▶︎ update: FastCoinIcon (8) - 0.05ms (max: 0.06ms; min: 0.05ms)

[18:08:29] I | ReactNativeJS ▶︎ update: FastCoinIcon (4) - 0.04ms (max: 0.04ms; min: 0.04ms)

[18:08:35] I | ReactNativeJS ▶︎ mount: FastCoinIcon (11) - 5.29ms (max: 6.39ms; min: 4.78ms)

[18:08:35] I | ReactNativeJS ▶︎ update: FastCoinIcon (1) - 1.72ms (max: 1.72ms; min: 1.72ms)

[18:08:36] I | ReactNativeJS ▶︎ update: FastCoinIcon (4) - 1.37ms (max: 1.78ms; min: 0.43ms)

[18:08:36] I | ReactNativeJS ▶︎ mount: FastCoinIcon (17) - 5.25ms (max: 8.48ms; min: 4.37ms)

[18:08:36] I | ReactNativeJS ▶︎ update: FastCoinIcon (6) - 1.51ms (max: 1.59ms; min: 1.44ms)

[18:08:36] I | ReactNativeJS ▶︎ update: FastCoinIcon (2) - 1.96ms (max: 2.16ms; min: 1.77ms)

[18:08:36] I | ReactNativeJS ▶︎ update: FastCoinIcon (4) - 2.22ms (max: 2.65ms; min: 1.70ms)

[18:08:37] I | ReactNativeJS ▶︎ update: FastCoinIcon (1) - 2.50ms (max: 2.50ms; min: 2.50ms)

[18:08:37] I | ReactNativeJS ▶︎ update: FastCoinIcon (9) - 1.88ms (max: 3.19ms; min: 0.52ms)

PoW (screenshots / screen recordings)

Before:

REC_20220518_094024_2.mp4

After:

REC_20220518_091440_3.mp4

Dev checklist for QA: what to test

Final checklist

  • Assigned individual reviewers?
  • Added labels?
  • Added e2e tests? if not please specify why
  • If you added new files, did you update the CODEOWNERS file?

@terrysahaidak
Copy link
Contributor Author

thanks, @tchayen, should be fixed now!

Copy link
Member

@estebanmino estebanmino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

built android and ios, looking good

@BrodyHughes
Copy link
Member

/preview

@terrysahaidak
Copy link
Contributor Author

/preview

@tchayen tchayen marked this pull request as draft June 16, 2022 08:46
@tchayen tchayen marked this pull request as ready for review June 16, 2022 08:46
@tchayen
Copy link
Contributor

tchayen commented Jun 16, 2022

Uh I am sorry Terry. I was looking at one of my branches and then closed browser tab and accidentally made your branch draft instead of mine 😬

@BrodyHughes BrodyHughes requested review from BrodyHughes and removed request for ibrahimtaveras00 June 16, 2022 14:45
Copy link
Member

@BrodyHughes BrodyHughes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hide / unhide & pin / unpin logic seems broken. I let Terry know via DM!

@terrysahaidak
Copy link
Contributor Author

/preview

Copy link
Member

@BrodyHughes BrodyHughes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! No app breaking behavior. Great performance improvements.

@terrysahaidak terrysahaidak merged commit b9c1ffc into develop Jun 17, 2022
@terrysahaidak terrysahaidak deleted the @terrysahaidak/fast-balance-coin-row branch June 17, 2022 07:48
@skylarbarrera skylarbarrera mentioned this pull request Jun 23, 2022
4 tasks
@terrysahaidak terrysahaidak mentioned this pull request Jul 4, 2022
4 tasks
@linear linear bot mentioned this pull request Jul 6, 2022
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
android android public beta needs dev review Includes code review AND testing out the branch performance performance related improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants