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

perf(gatsby-cli): throttle progress bar, build much faster this way #19866

Merged
merged 2 commits into from
Nov 28, 2019

Conversation

pvdz
Copy link
Contributor

@pvdz pvdz commented Nov 28, 2019

This change explicitly throttles the progress bar updates (tick and total) to be clamped to 10 fps max.

This drops the govbook benchmark from 210 seconds down to 140 seconds. Makes the build about as fast as with CI=true and GATSBY_LOGGER=yurnalist.

(Note: Many CI's automatically enable this so you won't see a difference there. But many people are unaware of the CI flag and this will make a huge difference for larger sites, or sites with many images.)

Fixes #15505
Fixes #17452
Fixes #17966
Fixes #18801

Relates to #17873
Relates to vadimdemedes/ink#212

…his way

This change explicitly throttles the progress bar updates (`tick` and `total`) to be clamped to 10 fps max.

This drops the govbook benchmark from 210 seconds down to 140 seconds.

Fixes #15505
Fixes #17452
Fixes #17966
Fixes #18801

Relates to #17873
Relates to vadimdemedes/ink#212
@pvdz pvdz requested a review from a team as a code owner November 28, 2019 19:25
Copy link
Contributor

@pieh pieh left a comment

Choose a reason for hiding this comment

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

Can we make sure in progressActivity.done() to flush any potential pending updates to make sure when activity finishes, the progress / total are set correctly?

Something like updateProgress(force: true), which will be no-op unless something is pending

packages/gatsby-cli/src/reporter/index.js Outdated Show resolved Hide resolved
@pvdz
Copy link
Contributor Author

pvdz commented Nov 28, 2019

Done. Will now force flush pending updates before resolving the done()

Copy link
Contributor

@pieh pieh left a comment

Choose a reason for hiding this comment

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

LGTM

@pvdz pvdz added the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label Nov 28, 2019
@gatsbybot gatsbybot merged commit c1764a3 into master Nov 28, 2019
@delete-merged-branch delete-merged-branch bot deleted the ink-perf branch November 28, 2019 20:51
@pvdz
Copy link
Contributor Author

pvdz commented Nov 28, 2019

Hmmm, I did not intend to skip cloud checks. The merge on green label seems to skip them. Good to know. I think it's fine for this one.

@vladar
Copy link
Contributor

vladar commented Dec 2, 2019

Published in gatsby-cli 2.8.15

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: merge on green Gatsbot will merge these PRs automatically when all tests passes
Projects
None yet
5 participants