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

Extra queries slow rebuild on file change when using gatsby-source-filesystem #20787

Closed
wodenx opened this issue Jan 22, 2020 · 6 comments · Fixed by #20798
Closed

Extra queries slow rebuild on file change when using gatsby-source-filesystem #20787

wodenx opened this issue Jan 22, 2020 · 6 comments · Fixed by #20798
Assignees
Labels
status: confirmed Issue with steps to reproduce the bug that’s been verified by at least one reviewer. type: bug An issue or pull request relating to a bug in Gatsby

Comments

@wodenx
Copy link

wodenx commented Jan 22, 2020

Description

Since v2.18.23, changes to files which are watched by gatsby-source-filesystem trigger rebuilds which , after time, involve execution of hundreds of queries and poor rebuild performance.

Steps to reproduce

See this example

Expected result

See this example

Actual result

See this example

Environment

System:
OS: macOS Mojave 10.14.6
CPU: (8) x64 Intel(R) Core(TM) i5-8279U CPU @ 2.40GHz
Shell: 3.2.57 - /bin/bash
Binaries:
Node: 10.16.0 - /usr/local/bin/node
Yarn: 1.21.1 - ~/.yarn/bin/yarn
npm: 6.9.0 - /usr/local/bin/npm
Languages:
Python: 2.7.10 - /usr/bin/python
Browsers:
Chrome: 79.0.3945.130
Safari: 13.0.4
npmPackages:
gatsby: ^2.19.1 => 2.19.1
gatsby-plugin-react-helmet: ^3.1.16 => 3.1.16
gatsby-source-filesystem: ^2.1.40 => 2.1.40
gatsby-transformer-code: ^0.1.0 => 0.1.0
npmGlobalPackages:
gatsby-dev-cli: 2.5.43

@vladar
Copy link
Contributor

vladar commented Jan 22, 2020

Can you confirm that it is not happening with version 2.18.22? CC @pvdz

@pvdz
Copy link
Contributor

pvdz commented Jan 22, 2020

From the repro repo it looks like he bisected it. I'll have a look to see whether I can repro. Perhaps the queue is shared / retained which might lead to incorrect events being picked up.

@pvdz
Copy link
Contributor

pvdz commented Jan 22, 2020

Ok, I can repro (at least between .22 and .23).

  • In that repro repo, the layout.js needs to be renamed to uppercase L for case sensitive file systems or the build will fail
  • Set the version of gatsby to 2.18.23 in package.json
  • Remove node_modules if not already and run npm install
  • Run gatsby develop
  • Add a space to any query (this will trigger a rebuild where run queries also runs), like in src/data/pages/f006/index.jsx
  • After two or three times you'll notice that the progress is reporting 3/1 queries
  • Stop develop
  • Set the version of gatsby to 2.18.23 in package.json
  • Remove node_modules if not already and run npm install (run npm ls gatsby to verify version)
  • Run gatsby develop
  • Repeat steps above and notice that the progress bar stays put at 1/1

I'll look into it. Thanks for the report and the repro :)

@pvdz pvdz added the type: bug An issue or pull request relating to a bug in Gatsby label Jan 22, 2020
@pvdz pvdz self-assigned this Jan 22, 2020
@pvdz pvdz added the status: confirmed Issue with steps to reproduce the bug that’s been verified by at least one reviewer. label Jan 22, 2020
@pvdz
Copy link
Contributor

pvdz commented Jan 22, 2020

  • Bisect blame confirmed to be https://github.com/gatsbyjs/gatsby/pull/20606/files?utf8=%E2%9C%93&diff=split&w=1
  • Queue is not retained
    • Queue is retained, I was looking at the wrong thing
  • Queue does fire the task_finish as many times as indicated, all for the same page
  • Every time, processBatch is only called once, with one job
  • The task_finish always fires n times for the currently built page (so if you start at page1 and then update page2, it will fire twice for page2)
  • Queue is retained after all and seems to be unwinding all its events again
  • As observed by @pieh the problem is that processBatch now attached a new task_finish event handler for every call to processBatch. That's fine on a fresh queue, but not when it's kept in memory, like for develop.

Solution: do not rebind the same event :) Will create a PR.

pvdz added a commit that referenced this issue Jan 22, 2020
The queue was retained for develop mode which led to the same event handlers being registered over and over again, which led to strange artifacts in the output.

With this change the events are aggressively deregistered from the queue when it finishes or fails.

Fixes #20787
gatsbybot pushed a commit that referenced this issue Jan 23, 2020
The queue was retained for develop mode which led to the same event handlers being registered over and over again, which led to strange artifacts in the output.

With this change the events are aggressively deregistered from the queue when it finishes or fails.

Fixes #20787
@pvdz
Copy link
Contributor

pvdz commented Jan 24, 2020

FYI: this may have been a red herring. While the count was definitely broken, it seems develop had another problem with redundant build steps being triggered. This should be fixed after #20836 merges.

@sidharthachatterjee
Copy link
Contributor

Fix was published in [email protected] @wodenx

Thanks for the report!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: confirmed Issue with steps to reproduce the bug that’s been verified by at least one reviewer. type: bug An issue or pull request relating to a bug in Gatsby
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants