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.

Sign up for GitHub

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

Jump to bottom

perf(gatsby-cli): avoid unnecesary rerenders for static messages in CLI #21955

Merged
merged 3 commits into from
Mar 9, 2020
Merged

perf(gatsby-cli): avoid unnecesary rerenders for static messages in CLI #21955

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4ad9379
perf(gatsby-cli): avoid unnecesary work for static messages on CLI re…
pieh Mar 4, 2020
aafaad5
Merge remote-tracking branch 'upstream/master' into cli-ink-static-perf
Mar 4, 2020
c44f6f6
make long and distinct name for array of memoized elements - are we j…
pieh Mar 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 27 additions & 9 deletions packages/gatsby-cli/src/reporter/loggers/ink/cli.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ class CLI extends React.Component {
hasError: false,
}

memoizedReactElementsForMessages = []

componentDidCatch(error, info) {
trackBuildError(`INK`, {
error: {
Expand Down Expand Up @@ -51,6 +53,30 @@ class CLI extends React.Component {
)
}

/*
Only operation on messages array is to push new message into it. Once
message is there it can't change. Because of that we can do single
transform from message object to react element and store it.
This will avoid calling React.createElement completely for every message
that can't change.
*/
if (messages.length > this.memoizedReactElementsForMessages.length) {
for (
let index = this.memoizedReactElementsForMessages.length;
index < messages.length;
index++
) {
const msg = messages[index]
this.memoizedReactElementsForMessages.push(
Copy link
Contributor

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.

Copy link
Contributor Author

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

Screenshot 2020-03-04 at 15 28 45
Screenshot 2020-03-04 at 15 31 10

I can upload .cpuprofile files if you want to take closer look ;)

Copy link
Contributor Author

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 ;) )

Copy link
Contributor Author

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

Copy link
Contributor

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 🔥

Copy link
Contributor Author

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)

msg.level === `ERROR` ? (
<Error details={msg} key={index} />
) : (
<Message key={index} {...msg} />
)
)
}
}

const spinners = []
const progressBars = []
if (showProgress) {
Expand All @@ -71,15 +97,7 @@ class CLI extends React.Component {
return (
<Box flexDirection="column">
<Box flexDirection="column">
<Static>
{messages.map((msg, index) =>
msg.level === `ERROR` ? (
<Error details={msg} key={index} />
) : (
<Message key={index} {...msg} />
)
)}
</Static>
<Static>{this.memoizedReactElementsForMessages}</Static>

{spinners.map(activity => (
<Spinner key={activity.id} {...activity} />
Expand Down
4 changes: 2 additions & 2 deletions packages/gatsby-cli/src/reporter/loggers/ink/components/error.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ const DocsLink = ({ docsUrl }) => {
)
}

const Error = ({ details }) => (
const Error = React.memo(({ details }) => (
// const stackLength = get(details, `stack.length`, 0

<Box marginY={1} flexDirection="column">
Expand Down Expand Up @@ -75,6 +75,6 @@ const Error = ({ details }) => (
</Box>
)} */}
</Box>
)
))

export default Error
4 changes: 2 additions & 2 deletions packages/gatsby-cli/src/reporter/loggers/ink/components/messages.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ const getLabel = level => {
}
}

export const Message = ({ level, text, duration, statusText }) => {
export const Message = React.memo(({ level, text, duration, statusText }) => {
let message = text
if (duration) {
message += ` - ${duration.toFixed(3)}s`
Expand All @@ -53,4 +53,4 @@ export const Message = ({ level, text, duration, statusText }) => {
{message}
</Box>
)
}
})