-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
perf(gatsby-cli): avoid unnecesary rerenders for static messages in CLI #21955
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would love to see a disintction between the two messages
array name, but lgtm otherwise. Merge at will.
Also, this is crazy town.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work @pieh 🥇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsure about the need for memoizedReactElementsForMessages
index++ | ||
) { | ||
const msg = messages[index] | ||
this.memoizedReactElementsForMessages.push( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is React.createElement that expensive? With React.memo
we shouldn't rerender these elements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow that's harsh, 🔥 createElement 🔥
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks awesome! Great work @pieh! 🕵
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)