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

perf(gatsby): fix performance regression with query dependency cleaning #28032

Merged
merged 3 commits into from
Nov 16, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ Object {
"byConnection": Map {},
"byNode": Map {},
"deletedQueries": Set {},
"queryNodes": Map {},
"trackedComponents": Map {
"/Users/username/dev/site/src/templates/my-sweet-new-page.js" => Object {
"componentPath": "/Users/username/dev/site/src/templates/my-sweet-new-page.js",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,11 @@ Object {
},
},
"deletedQueries": Set {},
"queryNodes": Map {
"/hi/" => Set {
"SuperCoolNode",
},
},
"trackedComponents": Map {},
"trackedQueries": Map {},
}
Expand Down
1 change: 1 addition & 0 deletions packages/gatsby/src/redux/reducers/__tests__/queries.ts
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ it(`has expected initial state`, () => {
"byConnection": Map {},
"byNode": Map {},
"deletedQueries": Set {},
"queryNodes": Map {},
"trackedComponents": Map {},
"trackedQueries": Map {},
}
Expand Down
101 changes: 76 additions & 25 deletions packages/gatsby/src/redux/reducers/queries.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ const initialState = (): IGatsbyState["queries"] => {
return {
byNode: new Map<NodeId, Set<QueryId>>(),
byConnection: new Map<ConnectionName, Set<QueryId>>(),
queryNodes: new Map<QueryId, Set<NodeId>>(),
trackedQueries: new Map<QueryId, IQueryState>(),
trackedComponents: new Map<ComponentPath, IComponentState>(),
deletedQueries: new Set<QueryId>(),
Expand Down Expand Up @@ -88,12 +89,8 @@ export function queriesReducer(
for (const component of state.trackedComponents.values()) {
component.pages.delete(queryId)
}
for (const nodeQueries of state.byNode.values()) {
nodeQueries.delete(queryId)
}
for (const connectionQueries of state.byConnection.values()) {
connectionQueries.delete(queryId)
}
state = clearNodeDependencies(state, queryId)
state = clearConnectionDependencies(state, queryId)
state.trackedQueries.delete(queryId)
}
state.deletedQueries.clear()
Expand All @@ -110,12 +107,12 @@ export function queriesReducer(
}
if (component.query !== query) {
// Invalidate all pages associated with a component when query text changes
component.pages.forEach(queryId => {
for (const queryId of component.pages) {
const query = state.trackedQueries.get(queryId)
if (query) {
query.dirty = setFlag(query.dirty, FLAG_DIRTY_TEXT)
}
})
}
component.query = query
}
return state
Expand Down Expand Up @@ -144,27 +141,18 @@ export function queriesReducer(
case `CREATE_COMPONENT_DEPENDENCY`: {
const { path: queryId, nodeId, connection } = action.payload
if (nodeId) {
const queryIds = state.byNode.get(nodeId) ?? new Set<QueryId>()
queryIds.add(queryId)
state.byNode.set(nodeId, queryIds)
state = addNodeDependency(state, queryId, nodeId)
}
if (connection) {
const queryIds =
state.byConnection.get(connection) ?? new Set<QueryId>()
queryIds.add(queryId)
state.byConnection.set(connection, queryIds)
state = addConnectionDependency(state, queryId, connection)
}
return state
}
case `QUERY_START`: {
// Reset data dependencies as they will be updated when running the query
const { path } = action.payload
state.byNode.forEach(queryIds => {
queryIds.delete(path)
})
state.byConnection.forEach(queryIds => {
queryIds.delete(path)
})
state = clearNodeDependencies(state, path)
state = clearConnectionDependencies(state, path)
return state
}
case `CREATE_NODE`:
Expand All @@ -177,18 +165,18 @@ export function queriesReducer(
const queriesByConnection =
state.byConnection.get(node.internal.type) ?? []

queriesByNode.forEach(queryId => {
for (const queryId of queriesByNode) {
const query = state.trackedQueries.get(queryId)
if (query) {
query.dirty = setFlag(query.dirty, FLAG_DIRTY_DATA)
}
})
queriesByConnection.forEach(queryId => {
}
for (const queryId of queriesByConnection) {
const query = state.trackedQueries.get(queryId)
if (query) {
query.dirty = setFlag(query.dirty, FLAG_DIRTY_DATA)
}
})
}
return state
}
case `PAGE_QUERY_RUN`: {
Expand All @@ -213,6 +201,69 @@ export function hasFlag(allFlags: number, flag: number): boolean {
return allFlags >= 0 && (allFlags & flag) > 0
}

function addNodeDependency(
state: IGatsbyState["queries"],
queryId: QueryId,
nodeId: NodeId
): IGatsbyState["queries"] {
// Perf: using two-side maps.
// Without additional `queryNodes` map we would have to loop through
// all existing nodes in `clearNodeDependencies` to delete node <-> query dependency
let nodeQueries = state.byNode.get(nodeId)
if (!nodeQueries) {
nodeQueries = new Set<QueryId>()
state.byNode.set(nodeId, nodeQueries)
}
let queryNodes = state.queryNodes.get(queryId)
if (!queryNodes) {
queryNodes = new Set<NodeId>()
state.queryNodes.set(queryId, queryNodes)
}
nodeQueries.add(queryId)
queryNodes.add(nodeId)
return state
}

function addConnectionDependency(
state: IGatsbyState["queries"],
queryId: QueryId,
connection: ConnectionName
): IGatsbyState["queries"] {
// Note: not using two-side maps for connections as associated overhead
// for small number of elements is greater then benefits, so no perf. gains
let queryIds = state.byConnection.get(connection)
if (!queryIds) {
queryIds = new Set()
state.byConnection.set(connection, queryIds)
}
queryIds.add(queryId)
return state
}

function clearNodeDependencies(
state: IGatsbyState["queries"],
queryId: QueryId
): IGatsbyState["queries"] {
const queryNodeIds = state.queryNodes.get(queryId) ?? new Set()
for (const nodeId of queryNodeIds) {
const nodeQueries = state.byNode.get(nodeId)
if (nodeQueries) {
nodeQueries.delete(queryId)
}
Comment on lines +249 to +252
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const nodeQueries = state.byNode.get(nodeId)
if (nodeQueries) {
nodeQueries.delete(queryId)
}
state.byNode.get(nodeId)?.delete(queryId)

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 was my first attempt. But it gives an Eslint error.

}
return state
}

function clearConnectionDependencies(
state: IGatsbyState["queries"],
queryId: QueryId
): IGatsbyState["queries"] {
for (const [, queryIds] of state.byConnection) {
queryIds.delete(queryId)
}
return state
}

function registerQuery(
state: IGatsbyState["queries"],
queryId: QueryId
Expand Down
1 change: 1 addition & 0 deletions packages/gatsby/src/redux/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,7 @@ export interface IGatsbyState {
queries: {
byNode: Map<Identifier, Set<Identifier>>
byConnection: Map<string, Set<Identifier>>
queryNodes: Map<Identifier, Set<Identifier>>
trackedQueries: Map<Identifier, IQueryState>
trackedComponents: Map<string, IComponentState>
deletedQueries: Set<Identifier>
Expand Down