perf[react-native]: Memoized Blocks Component to free up UI thread. #3814
Conversation
|
|
View your CI Pipeline Execution ↗ for commit bb5b126.
☁️ Nx Cloud last updated this comment at |
yash-builder
changed the title
| )); | ||
|
|
||
| // Optional: Give the component a display name for better debugging | ||
| RenderBlock.displayName = 'RenderBlock'; |
There was a problem hiding this comment.
🎯 suggestion:
Should we append block.id to the displayName as a identifier ?
There was a problem hiding this comment.
We can surely do that. But in this scenario I used this identifier for inspecting the component in React Devtools
| }; | ||
|
|
||
| // Moved outside and memoized | ||
| const RenderBlock = memo(({ |
There was a problem hiding this comment.
❔question:
We have used memo over Block component ( to get RenderBlock ) and then composed the RenderBlock with React.useCallback. Could you explain the logic here and how it works internally ?
- what happens if we use
memo/useCallbackin both places ?
There was a problem hiding this comment.
Great question!
- If Only
memoIs Used:
RenderBlockwill avoid re-renders, butFlatListmay still recreate the renderItem function on every render. This may lead to potential performance bottleneck. - If Only
useCallbackIs Used:
TherenderItemfunction will be stable, butRenderBlockwill re-render if its props are shallowly different, even when not required.
There was a problem hiding this comment.
Very well done! I'm Rewriting here what I sent you in Slack so we don't lose track of it.
One important thing is not to use overrides/* unless as a last resort because the file will quickly go out of sync with the original Blocks.lite.tsx, forcing us to maintain multiple copies of the same logic.
Here is a Loom showing how I'd tackle the ~3-4 changes in your PR and incorporate them into Mitosis plugins instead:
https://www.loom.com/share/94f1767796bd487bb48be41d9046e01f
Also, you need to add a changesets entry, which you can do by following https://github.com/BuilderIO/builder/blob/main/packages/sdks/PUBLISHING.md
packages/sdks/mitosis.config.cjs
Outdated
| code: { | ||
| post: (code, json) => { | ||
| if (json.name === 'Blocks') { | ||
| return code.replace( | ||
| 'export default Blocks', | ||
| 'export default memo(Blocks)' | ||
| ); | ||
| } | ||
| return code; | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Initially in this PR, you were memoizing Block, but now you're memoizing Blocks instead.
Was this intended? Do we still get the same performance benefits from memoizing the parent component instead of the individual items?
There was a problem hiding this comment.
Hey Sami, In the earlier PR also we were memoizing Blocks.tsx and not Block.tsx
There was a problem hiding this comment.
Ah indeed, you memoized Blocks.tsx in your original PR commit.
but you were also creating a memoized version of Block called RenderBlock : https://github.com/BuilderIO/builder/pull/3814/commits/e264c120fd38851208875a9915b55e26e952528f#diff-b9cbe94aa51d49deda0[…]fd13d38a1f56e5641441f5R20
Which is now gone from this PR. Was it unnecessary?
There was a problem hiding this comment.
Ah my bad I missed this part. Also one question I want to ask by using mitosis how can I write code outside the Blocks function but inside the Blocks.tsx
There was a problem hiding this comment.
in my Loom I explan that in this case, you probably want to add export default Memo(Block) to Block.tsx, and remove this RenderBlock from Blocks.tsx to achieve the same outcome
There was a problem hiding this comment.
gotcha!
Asking out of curiosity. Is there a way where we can declare variables/functions outside Functional/Class component using mitosis.config.js?
There was a problem hiding this comment.
Yes actually, there's a preComponent hook in Mitosis whose code will be printed right after import statements (and therefore, outside React components)
There was a problem hiding this comment.
I see large-reactive-state.spec.ts is failing for React Native now. This is an integration stress-test for SDK performance: it renders thousands of interactive elements on the same page, performs multiple state updates and makes sure it all gets done in a reasonable time frame.
It's a bit surprising that your PR is causing it to fail. Can you investigate this failure and make sure there aren't any unintended performance drawbacks to this solution?
Hmm...that's weird let me take a look into it |
There was a problem hiding this comment.
Code looks good! You're still missing the changesets entry https://github.com/BuilderIO/builder/blob/main/packages/sdks/PUBLISHING.md#1--add-changeset
Description
Suspense:Added
React.Suspenseto defer the rendering of theContentcomponent until it is fully loaded.Memoization:React.memoto memoize computationally expensive operations within theBlockscomponent. To prevents unnecessary recalculations of processed blocks during re-renders, reducing the load on the UI thread.Screenshot
Before:
After:
