perf(gatsby-cli): avoid unnecesary rerenders for static messages in CLI #21955
Conversation
sidharthachatterjee
changed the title
| index++ | ||
| ) { | ||
| const msg = messages[index] | ||
| this.memoizedReactElementsForMessages.push( |
There was a problem hiding this comment.
Is React.createElement that expensive? With React.memo we shouldn't rerender these elements.
There was a problem hiding this comment.
Yes, React.memo avoids re-rendering, but createElement is still super expensive (at least for now, while we use development react version).
With just React.memo my "stress test" https://github.com/pieh/ink-stress-test still takes 5s to log 1000 messages. With memoized stuff it take 1.5s
On the left - with memoizedReactElementsForMessages and memo, on the right just with memo
I can upload .cpuprofile files if you want to take closer look ;)
There was a problem hiding this comment.
Notice on flamegraph on the right how much work our message.map is taking compared to finishSyncRender which "commit" changes to CLI output - the finishSyncRender in general is constant here - just the the render function of CLI component is massively more expensinve because of React.createElement (well at least for now - maybe swapping to production react build would make it a lot better, but this needs more engineering work - need webpack or rollup to bundle that stuff up and setup package build and hopefully watch correctly - so this is just effort vs impact calculation here ;) )
There was a problem hiding this comment.
I also do realize that doing what I'm doing is not really the "react" way, but it's pragmatic quick solution to a problem.
There are additional things we could explore, but those might need reaching out to ink maintainer about allowing <Static> component to not have to keep getting all past static elements - that way we could avoid mapping over all past messages on each re-render etc - but this seems even more involved and return of investment (time/research) might be questionable
There was a problem hiding this comment.
Wow that's harsh, 🔥 createElement 🔥
There was a problem hiding this comment.
Note, that this might be greatly impacted by using dev version of react which does a lot more validation stuff than production one (tho I didn't research that enough to give impact estimates of switching to prod react version - still want to do it - it's just more work over doing this hack)
|
@vadimdemedes curious on your take here — is there a way to avoid passing in old elements to |
|
Hey folks, I'm working on Ink 3 nowadays and performance improvements is one of the most important things I'm focusing on. Among those improvements, a new <Static items={[array, of, items, to, render])>
{item => <ComponentThatRendersItem item={item}/>}
</Static>Under the hood it ensures that items that have already been rendered and written to stdout are no longer present in In a simple benchmark I made which adds 1000 items to There are many other things I have planned to improve performance of Ink, but I thought you'd be interested to hear this sooner. |
|
Thanks for the update @vadimdemedes! V3 sounds exciting and I would love to revert this hack when it lands ;) Will also be interested in checking it out when you will reach alpha/beta versions |
Previous implementation of `<Static>` component was performing poorly for apps with thousands of items to render. Ink's rendering process slowed down proportionally to the number of children in `<Static>` component, because React has to call `React.createElement()` for each child, even if it was already rendered. See gatsbyjs/gatsby#21955 (comment) for more context. This change changes the API of a `<Static>` component to call `React.createElement()` and render only new items, ignoring the items that were already rendered. The new API is looking very similar to virtual list libraries. You pass a list of items to render via `items` prop and specify a function that is responsible for rendering each item, the rest is handled for you. ```jsx <Static items={['a', 'b', 'c']}> {item => <Box key={item}>Item: {item}</Box>} </Static> ```
Previous implementation of `<Static>` component was performing poorly for apps with thousands of items to render. Ink's rendering process slowed down proportionally to the number of children in `<Static>` component, because React has to call `React.createElement()` for each child, even if it was already rendered. See gatsbyjs/gatsby#21955 (comment) for more context. This change changes the API of a `<Static>` component to call `React.createElement()` and render only new items, ignoring the items that were already rendered. The new API is looking very similar to virtual list libraries. You pass a list of items to render via `items` prop and specify a function that is responsible for rendering each item, the rest is handled for you. ```jsx <Static items={['a', 'b', 'c']}> {item => <Box key={item}>Item: {item}</Box>} </Static> ``` This change also removes the need to traverse entire node tree on every render to find where `<Static>` is located and extract its contents. Reconciler is going to save a reference to `<Static>` node as soon as it encounters one, saving precious CPU cycles and thus making rendering less expensive.
Previous implementation of `<Static>` component was performing poorly for apps with thousands of items to render. Ink's rendering process slowed down proportionally to the number of children in `<Static>` component, because React has to call `React.createElement()` for each child, even if it was already rendered. See gatsbyjs/gatsby#21955 (comment) for more context. This change changes the API of a `<Static>` component to call `React.createElement()` and render only new items, ignoring the items that were already rendered. The new API is looking very similar to virtual list libraries. You pass a list of items to render via `items` prop and specify a function that is responsible for rendering each item, the rest is handled for you. ```jsx <Static items={['a', 'b', 'c']}> {item => <Box key={item}>Item: {item}</Box>} </Static> ``` This change also removes the need to traverse entire node tree on every render to find where `<Static>` is located and extract its contents. Reconciler is going to save a reference to `<Static>` node as soon as it encounters one, saving precious CPU cycles and thus making rendering less expensive.
* Implement a new <Static> component Previous implementation of `<Static>` component was performing poorly for apps with thousands of items to render. Ink's rendering process slowed down proportionally to the number of children in `<Static>` component, because React has to call `React.createElement()` for each child, even if it was already rendered. See gatsbyjs/gatsby#21955 (comment) for more context. This change changes the API of a `<Static>` component to call `React.createElement()` and render only new items, ignoring the items that were already rendered. The new API is looking very similar to virtual list libraries. You pass a list of items to render via `items` prop and specify a function that is responsible for rendering each item, the rest is handled for you. ```jsx <Static items={['a', 'b', 'c']}> {item => <Box key={item}>Item: {item}</Box>} </Static> ``` This change also removes the need to traverse entire node tree on every render to find where `<Static>` is located and extract its contents. Reconciler is going to save a reference to `<Static>` node as soon as it encounters one, saving precious CPU cycles and thus making rendering less expensive. * Update readme.md Co-authored-by: Sindre Sorhus <sindresorhus@gmail.com> Co-authored-by: Sindre Sorhus <sindresorhus@gmail.com>
* Implement a new <Static> component Previous implementation of `<Static>` component was performing poorly for apps with thousands of items to render. Ink's rendering process slowed down proportionally to the number of children in `<Static>` component, because React has to call `React.createElement()` for each child, even if it was already rendered. See gatsbyjs/gatsby#21955 (comment) for more context. This change changes the API of a `<Static>` component to call `React.createElement()` and render only new items, ignoring the items that were already rendered. The new API is looking very similar to virtual list libraries. You pass a list of items to render via `items` prop and specify a function that is responsible for rendering each item, the rest is handled for you. ```jsx <Static items={['a', 'b', 'c']}> {item => <Box key={item}>Item: {item}</Box>} </Static> ``` This change also removes the need to traverse entire node tree on every render to find where `<Static>` is located and extract its contents. Reconciler is going to save a reference to `<Static>` node as soon as it encounters one, saving precious CPU cycles and thus making rendering less expensive. * Update readme.md Co-authored-by: Sindre Sorhus <sindresorhus@gmail.com> Co-authored-by: Sindre Sorhus <sindresorhus@gmail.com>
This was tested on https://github.com/pieh/ink-stress-test/blob/master/gatsby-node.js
and it dropped from ~5-6s to ~1s locally
There is more we can do for CLI perf - particularly we do use development react build, which has additional perf overhead. (tho with those changes impact is greatly minimilized - at least for my testing sample https://github.com/pieh/ink-stress-test)