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

Ink introduces significant overhead to the build time #15505

Closed
me4502 opened this issue Jul 8, 2019 · 25 comments · Fixed by #19866
Closed

Ink introduces significant overhead to the build time #15505

me4502 opened this issue Jul 8, 2019 · 25 comments · Fixed by #19866
Assignees

Comments

@me4502
Copy link
Contributor

me4502 commented Jul 8, 2019

Description

While profiling some performance issues, I noticed that Ink's render method takes around 60 seconds of our build due to writing to the terminal socket.

This is mostly an issue with heavy progressbar usage, such as during the run queries step. There appears to be some debouncing in the profile, however this doesn't prevent the slowdown. It may be worth either limiting it to 2 updates per second, or providing a flag to disable ink?

Steps to reproduce

Connect a profiler, have a lot of pages, and look for Ink in the profile output.

Expected result

The terminal should not introduce a significant overhead.

Actual result

The terminal introduces 60 seconds of overhead (Increases run queries from 300 to 350 seconds).

Environment

System:
OS: macOS 10.15
CPU: (12) x64 Intel(R) Core(TM) i9-8950HK CPU @ 2.90GHz
Shell: 3.2.57 - /bin/bash
Binaries:
Node: 11.15.0 - ~/.nvm/versions/node/v11.15.0/bin/node
Yarn: 1.16.0 - ~/.nvm/versions/node/v11.15.0/bin/yarn
npm: 6.7.0 - ~/.nvm/versions/node/v11.15.0/bin/npm
Languages:
Python: 2.7.16 - /usr/local/bin/python
Browsers:
Chrome: 75.0.3770.100
Safari: 13.0

@vadimdemedes
Copy link

Hey @me4502, creator of Ink here 👋 Do you have a public repo where I could reproduce this problem and validate that my fix would work?

@me4502
Copy link
Contributor Author

me4502 commented Jul 11, 2019

I don't have a public repo sorry, but assuming this is just about the query progressbar size...

https://github.com/gatsbyjs/gatsby/tree/master/benchmarks/create-pages with 75k pages should produce the same results.

@vadimdemedes
Copy link

Thanks! I'm on vacation right now, but I will look into it next week ;)

@vadimdemedes
Copy link

I ran the benchmark you've mentioned with three different settings for throttling output for 75k pages and haven't noticed a significant performance regression yet:

  • 1 frame per second, build took 207s
  • 60 frames per second, build took 213s
  • no throttling, build took 213s

Will keep digging and keep you posted.

@me4502
Copy link
Contributor Author

me4502 commented Jul 29, 2019

In our case we've got a lot of pages that cause images to be rendered with sharp, so there are two progress bars going at once. It could be related to this?

@vadimdemedes
Copy link

Could be. I'm not familiar with sharp, could you link me to it and how are you setting it up?

@me4502
Copy link
Contributor Author

me4502 commented Jul 29, 2019

https://www.gatsbyjs.org/packages/gatsby-plugin-sharp/ This is the Gatsby plugin that handles it, basically a lot of the pages have queries on them that use images, meaning sharp spins up extra jobs that also have a progress bar.

@vadimdemedes
Copy link

Could you try disabling progress bars altogether and see if it improves your build time? You can do that at node_modules/gatsby/node_modules/gatsby-cli/lib/reporter/reporters/ink/reporter.js (line 36) by replacing const showProgress = _isTty.default; with const showProgress = false;.

@me4502
Copy link
Contributor Author

me4502 commented Jul 29, 2019

So after some extensive testing, these are the results:

Ink (Avg): 661s
Ink w/o Progress (Avg): 583s
Yurnalist (Avg): 503s

@vadimdemedes
Copy link

Quick update, I've been looking into it whole evening, and I've came to conclusion that frequency of output rendering won't affect performance, because the heavy part is happening when output is generated in Ink, not when it's rendered. I have some ideas for optimization, but it will take time to implement and test those. Until then, I'd recommend sticking with Yurnalist reporter, if slowdown is unacceptable to you.

@me4502
Copy link
Contributor Author

me4502 commented Jul 31, 2019

Thanks a lot :)

I've setup a patch to use Yurnalist for the meantime.

@vadimdemedes
Copy link

I think you can skip the patch and just run gatsby build with CI=true environment variable to use Yurnalist, like this: CI=true gatsby build. This will force it to ignore Ink reporter.

@vadimdemedes
Copy link

I don't like promising things, but I just wanted to let you know there's some good progress on improving performance in Ink and hope to have something ready for you soon ;)

@vadimdemedes
Copy link

vadimdemedes commented Aug 11, 2019

Hey @me4502, I rewrote the entire process of building output in Ink to be much more efficient and would love it if you could try it out in your project. Here's how:

  1. Ensure all dependencies are installed
  2. Run npm install 'vadimdemedes/ink#experimental-reconciler'
  3. Confirm the experimental reconciler exists and you've installed the correct version by running ls node_modules/ink/build/experimental
  4. Open node_modules/gatsby/node_modules/gatsby-cli/lib/reporter/reporters/ink/index.js and replace lines 17-19 (only use the "After" version):
// Before
(0, _ink.render)(_react.default.createElement(_reporter.default, {
  ref: inkReporter
}));

// After
(0, _ink.render)(_react.default.createElement(_reporter.default, {
  ref: inkReporter
}), {experimental: true);
  1. Run build

@me4502
Copy link
Contributor Author

me4502 commented Aug 15, 2019

Thanks :)

So I've ran it again and these are the results:

Yurnalist: Done building in 463 sec
Ink (with fix): Done building in 506 sec
Ink: Done building in 532 sec

A Gatsby update since the last one appears to have sped things up across the board a bit, but overall the gap between Yurnalist and Ink has definitely closed a lot :)

@vadimdemedes
Copy link

This is awesome news! I still have some improvements in mind, would appreciate it, if you could continue helping to measure the impact of these changes :)

@me4502
Copy link
Contributor Author

me4502 commented Aug 15, 2019

Sounds good, thanks! :) I can definitely keep testing as it other changes come in

@gatsbot gatsbot bot added the stale? Issue that may be closed soon due to the original author not responding any more. label Sep 5, 2019
@gatsbot
Copy link

gatsbot bot commented Sep 5, 2019

Hiya!

This issue has gone quiet. Spooky quiet. 👻

We get a lot of issues, so we currently close issues after 30 days of inactivity. It’s been at least 20 days since the last update here.

If we missed this issue or if you want to keep it open, please reply here. You can also add the label "not stale" to keep this issue open!

As a friendly reminder: the best way to see this issue, or any other, fixed is to open a Pull Request. Check out gatsby.dev/contribute for more information about opening PRs, triaging issues, and contributing!

Thanks for being a part of the Gatsby community! 💪💜

@brettinternet
Copy link
Contributor

I'm happy I found the CI=true option to remove CLI animations for dumb output in tools like Jenkins which were printing out a new line for every animation.

Screenshot_1

Does it make sense to include a --no-progress option in gatsby build? Should I open a separate ticket for a feature request?

@sidharthachatterjee
Copy link
Contributor

Just leaving a note here: this issue is a blocker for removing the yurnalist implementation

@wardpeet
Copy link
Contributor

@brettinternet Please create a new issue with the --no-progress flag, I think it makes sense so people don't have to think about the CI env flag.

We try to detect non TTY inputs like jenkins but some products are bad at faking it so we also disable it on CI builds. Most ci vendors enable the CI=true flag.

@simonjoom
Copy link

simonjoom commented Oct 18, 2019

yeah got the same problem too much slow with ink,
for me i use yurnalist, no need much more animations anyway.

To use yurnalist just remove ink from your dependencies, gatsby won't use Ink it if he doesn't find Ink

i ve' got less 30sec in build than ink do with yurnalist!!

tips: If you use lot of console.log in your code, there is an effect that ink will slow down much more the build process

@vadimdemedes
Copy link

@simonjoom Could you try the solution described here #15505 (comment), but instead of npm install ink#experimental-reconciler execute npm upgrade ink.

@pvdz
Copy link
Contributor

pvdz commented Nov 28, 2019

@vadimdemedes I saw you mention somewhere you wanted a repro; you can check out https://github.com/thechicagoreporter/govbook , npm install, gatsby build
Currently that builds in about 250 sec for me on master. If I enable Ink's experimental mode and bump to 2.6.0 that drops a bit, to 205 sec. If I use CI=true or GATSBY_LOGGER=yurnalist then the build finishes in 135 sec.

We need to fix this perf. I'm going to take measures on Gatsby's side. What can we do to help Ink?

@pvdz pvdz self-assigned this Nov 28, 2019
pvdz added a commit that referenced this issue Nov 28, 2019
…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
gatsbybot pushed a commit that referenced this issue Nov 28, 2019
…19866)

* perf(gatsby-cli): throttle progress bar, build significantly faster this 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

* Force flush on done() and use local var for total
@pvdz
Copy link
Contributor

pvdz commented Nov 28, 2019

The overhead of Ink should now be resolved. Please report new issues if you still see them after #19866

Thanks everyone for your input!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants