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

fix(gatsby): lower memory pressure in SSR #30793

Merged
merged 2 commits into from
Apr 9, 2021
Merged

fix(gatsby): lower memory pressure in SSR #30793

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b081e91
fix(gatsby): lower memory pressure in SSR
vladar Apr 9, 2021
414678c
Remove redundant worker pool
vladar Apr 9, 2021
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
5 changes: 1 addition & 4 deletions packages/gatsby/src/commands/build.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import db from "../db"
import { store } from "../redux"
import * as appDataUtil from "../utils/app-data"
import { flush as flushPendingPageDataWrites } from "../utils/page-data"
import * as WorkerPool from "../utils/worker/pool"
import {
structureWebpackErrors,
reportWebpackWarnings,
Expand Down Expand Up @@ -79,7 +78,7 @@ module.exports = async function build(program: IBuildArgs): Promise<void> {
const buildSpan = buildActivity.span
buildSpan.setTag(`directory`, program.directory)

const { gatsbyNodeGraphQLFunction } = await bootstrap({
const { gatsbyNodeGraphQLFunction, workerPool } = await bootstrap({
program,
parentSpan: buildSpan,
})
Expand Down Expand Up @@ -137,8 +136,6 @@ module.exports = async function build(program: IBuildArgs): Promise<void> {
buildActivityTimer.end()
}

const workerPool = WorkerPool.create()

const webpackCompilationHash = stats.hash
if (
webpackCompilationHash !== store.getState().webpackCompilationHash ||
Expand Down
91 changes: 51 additions & 40 deletions packages/gatsby/src/utils/worker/render-html.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,34 +312,38 @@ export const renderHTMLProd = async ({
}
}

await Bluebird.map(paths, async pagePath => {
try {
const pageData = await readPageData(publicDir, pagePath)
const resourcesForTemplate = await getResourcesForTemplate(pageData)

const { html, unsafeBuiltinsUsage } = htmlComponentRenderer.default({
pagePath,
pageData,
...resourcesForTemplate,
})

if (unsafeBuiltinsUsage.length > 0) {
unsafeBuiltinsUsageByPagePath[pagePath] = unsafeBuiltinsUsage
}

return fs.outputFile(getPageHtmlFilePath(publicDir, pagePath), html)
} catch (e) {
if (e.unsafeBuiltinsUsage && e.unsafeBuiltinsUsage.length > 0) {
unsafeBuiltinsUsageByPagePath[pagePath] = e.unsafeBuiltinsUsage
}
// add some context to error so we can display more helpful message
e.context = {
path: pagePath,
unsafeBuiltinsUsageByPagePath,
await Bluebird.map(
paths,
async pagePath => {
try {
const pageData = await readPageData(publicDir, pagePath)
const resourcesForTemplate = await getResourcesForTemplate(pageData)

const { html, unsafeBuiltinsUsage } = htmlComponentRenderer.default({
pagePath,
pageData,
...resourcesForTemplate,
})

if (unsafeBuiltinsUsage.length > 0) {
unsafeBuiltinsUsageByPagePath[pagePath] = unsafeBuiltinsUsage
}

return fs.outputFile(getPageHtmlFilePath(publicDir, pagePath), html)
} catch (e) {
if (e.unsafeBuiltinsUsage && e.unsafeBuiltinsUsage.length > 0) {
unsafeBuiltinsUsageByPagePath[pagePath] = e.unsafeBuiltinsUsage
}
// add some context to error so we can display more helpful message
e.context = {
path: pagePath,
unsafeBuiltinsUsageByPagePath,
}
throw e
}
throw e
}
})
},
{ concurrency: 2 }
Copy link
Contributor

Choose a reason for hiding this comment

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

wouldn't we want the concurrency to match the size of the jest worker pool?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is concurrency within one worker. So this effectively sets total concurrency across all workers to workerPoolSize * 2. I was hesitating if we should set { concurrency: 1 } or { concurrency: 2 } here but 2 seems to keep memory usage as low as 1, so settled on value 2.

To clarify your question - do you mean changing it to 1 here? Because setting it to workerPoolSize will effectively make it workerPoolSize * workerPoolSize which doesn't make much sense to me unless I am missing something.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh my bad. I didn't read this closely and thought this was in the main node process. Yeah 2 makes sense then. SSR is pure CPU so reading in any more page-data.json files wouldn't speed up anything and 1 would very slightly slow it down as the next SSR has to wait for the read to finish.

)

return { unsafeBuiltinsUsageByPagePath }
}
Expand Down Expand Up @@ -372,18 +376,25 @@ export const renderHTMLDev = async ({
lastSessionId = sessionId
}

return Bluebird.map(paths, async pagePath => {
try {
const htmlString = htmlComponentRenderer.default({
pagePath,
})
return fs.outputFile(getPageHtmlFilePath(outputDir, pagePath), htmlString)
} catch (e) {
// add some context to error so we can display more helpful message
e.context = {
path: pagePath,
return Bluebird.map(
paths,
async pagePath => {
try {
const htmlString = htmlComponentRenderer.default({
pagePath,
})
return fs.outputFile(
getPageHtmlFilePath(outputDir, pagePath),
htmlString
)
} catch (e) {
// add some context to error so we can display more helpful message
e.context = {
path: pagePath,
}
throw e
}
throw e
}
})
},
{ concurrency: 2 }
)
}