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(gatsby): rewrite a spread that would break at scale #28910

Merged
merged 1 commit into from
Jan 8, 2021
Merged

Conversation

pvdz
Copy link
Contributor

@pvdz pvdz commented Jan 7, 2021

Do not spread all the nodes of a particular type on a function call. When the set of nodes exceeds 64k elements you'll see stack overflow errors. This PR rewrites it such that the spread is no longer necessary. (The new spread is going to be one or a very low number since you're not going to have thousands of types in a Gatsby site unless you're trying hard for it)

This was blocking images at scale. However, the image stuff no longer goes through this path after #28891 but this function should still not spread potentially thousands of nodes.

I tested a few strategies and then picked the one in this PR because it was the least invasive to the code base while being about equal in performance. Tested against an 8k image benchmark that hit this path six times for every image (before the aforementioned PR). Note: In this test, the type count was always 1 ('File').

// 138s; "master"
const nodes = nodeTypeNames.reduce((acc, typeName) => {
  acc.push(...getNodesByType(typeName))
  return acc
}, [])
result = nodes.filter(Boolean)

// 138s ; not casting the map values to an array first
const nodes = nodeTypeNames.reduce((acc, typeName) => {
  acc.push(...store.getState().nodesByType.get(typeName).values())
  return acc
}, [])
result = nodes.filter(Boolean)

// 135s; not using reduce
let nodes = []
nodeTypeNames.forEach(typeName => {
  nodes.push(...getNodesByType(typeName))
})
result = nodes.filter(Boolean)

// 131s ; concat every step (in this test there is only one type so probably same as next one)
let nodes = [];
nodeTypeNames.forEach(typeName => {
  nodes = nodes.concat(getNodesByType(typeName))
})
result = nodes.filter(Boolean)

// 129s ; concat all sets of nodes once. in this test there was only one type so probably same perf as before
const nodesByType = nodeTypeNames.map(typeName => getNodesByType(typeName))
const nodes = [].concat(...nodesByType)
result = nodes.filter(Boolean)

// 131s ; concat per type and filter per type rather than afterwards (bad test with one type)
const nodesByType = nodeTypeNames.map(typeName => getNodesByType(typeName).filter(Boolean))
const nodes = [].concat(...nodesByType)
result = nodes

// 129s ; concat per type, direct from the source, but oh well
const reduxNodesByType = store.getState().nodesByType
const nodesByType = nodeTypeNames.map(typeName => Array.from(reduxNodesByType.get(typeName).values()))
const nodes = [].concat(...nodesByType)
result = nodes.filter(Boolean)

// 132s ; one push per node
const nodes = []
nodeTypeNames.forEach(typeName => {
  getNodesByType(typeName).forEach(node => node && nodes.push(node))
})
result = nodes

// 129s ; one push per node, direct map access
const reduxNodesByType = store.getState().nodesByType
const nodes = []
nodeTypeNames.forEach(typeName => {
  reduxNodesByType.get(typeName).forEach(node => node && nodes.push(node))
})
result = nodes

// 148s ; one push per node, shallow cloned map access
const nodes = []
nodeTypeNames.forEach(typeName => {
  getNodesByTypeAsMap(typeName).forEach(node => node && nodes.push(node))
})
result = nodes

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Jan 7, 2021
@pvdz pvdz added type: bug An issue or pull request relating to a bug in Gatsby topic: scaling builds and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Jan 7, 2021
@pvdz pvdz merged commit 638ac0a into master Jan 8, 2021
@pvdz pvdz deleted the spreadscale branch January 8, 2021 12:28
pragmaticpat pushed a commit to pragmaticpat/gatsby that referenced this pull request Apr 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug An issue or pull request relating to a bug in Gatsby
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants