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

fix: use cpuCount for all parallel parts #30548

Merged
merged 2 commits into from
Apr 22, 2021
Merged

Conversation

wardpeet
Copy link
Contributor

Description

Not every part of Gatsby is using cpuCoreCount and we should always do -1 to make sure the main process is left alone.

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Mar 30, 2021
@wardpeet wardpeet added type: maintenance An issue or pull request describing a change that isn't a bug, feature or documentation change and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Mar 30, 2021
@axe312ger
Copy link
Collaborator

More cores used on minify? I like!

@@ -12,7 +12,7 @@ sharp.simd(true)
// Handle Sharp's concurrency based on the Gatsby CPU count
// See: http://sharp.pixelplumbing.com/en/stable/api-utility/#concurrency
// See: https://www.gatsbyjs.org/docs/multi-core-builds/
sharp.concurrency(cpuCoreCount())
sharp.concurrency(cpuCoreCount() - 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does concurrency make sense for the manifest plugin at all? It is for a single icon resizing, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah — it wouldn't ever use more than one core

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Problem here is that sharp is a singleton so it will be shared with gatsby-plugin-sharp :'(

Copy link
Contributor

Choose a reason for hiding this comment

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

But gatsby-plugin-sharp now has concurrency 1 (after this PR: #28575):

// Concurrency is handled in gatsby-worker queue instead
sharp.concurrency(1)

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder what is the actual behavior currently if you have both plugins 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh interesting :D I'll set it to one than. Basically last one in gatsby-config wins

@vladar
Copy link
Contributor

vladar commented Apr 9, 2021

cpuCoreCount() - 1 can be equal to 0 with one core and also when GATSBY_CPU_COUNT env variable is set to 1

@wardpeet wardpeet marked this pull request as ready for review April 19, 2021 13:44
Copy link
Contributor

@vladar vladar left a comment

Choose a reason for hiding this comment

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

👍

@wardpeet wardpeet merged commit 9dbb772 into master Apr 22, 2021
@wardpeet wardpeet deleted the fix/cpucount-parallel branch April 22, 2021 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: maintenance An issue or pull request describing a change that isn't a bug, feature or documentation change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants