From 860aace9904e787f9bf05aad94be5b5920f10543 Mon Sep 17 00:00:00 2001 From: "Sachin D. Shinde" Date: Mon, 8 Jul 2024 14:46:15 -0700 Subject: [PATCH] Avoid sparsely-populated arrays in query graph cache (#3066) For condition resolution, we use a cache that supports caching data per query graph node or edge. This cache's code was pre-allocating arrays that would almost always be sparsely populated, and for large schemas this overhead is significant. This PR changes these arrays into maps. --- .changeset/friendly-tables-mix.md | 7 +++ composition-js/src/validate.ts | 2 +- query-graphs-js/src/conditionsCaching.ts | 6 +-- query-graphs-js/src/conditionsValidation.ts | 2 +- query-graphs-js/src/graphviz.ts | 2 +- .../src/nonTrivialEdgePrecomputing.ts | 2 +- query-graphs-js/src/querygraph.ts | 47 ++++++++++--------- query-planner-js/src/buildPlan.ts | 1 - 8 files changed, 40 insertions(+), 29 deletions(-) create mode 100644 .changeset/friendly-tables-mix.md diff --git a/.changeset/friendly-tables-mix.md b/.changeset/friendly-tables-mix.md new file mode 100644 index 000000000..9b3fa471e --- /dev/null +++ b/.changeset/friendly-tables-mix.md @@ -0,0 +1,7 @@ +--- +"@apollo/composition": patch +"@apollo/query-planner": patch +"@apollo/query-graphs": patch +--- + +Query graph caches now use maps instead of sparsely-populated arrays for per-subgraph data. diff --git a/composition-js/src/validate.ts b/composition-js/src/validate.ts index 4a62a4158..82a404822 100644 --- a/composition-js/src/validate.ts +++ b/composition-js/src/validate.ts @@ -705,7 +705,7 @@ class ValidationTraversal { conditionResolver: this.conditionResolver, overrideConditions: new Map(), }))); - this.previousVisits = new QueryGraphState(supergraphAPI); + this.previousVisits = new QueryGraphState(); this.context = new ValidationContext( supergraphSchema, subgraphNameToGraphEnumValue, diff --git a/query-graphs-js/src/conditionsCaching.ts b/query-graphs-js/src/conditionsCaching.ts index 051663e0c..f0cde4f8b 100644 --- a/query-graphs-js/src/conditionsCaching.ts +++ b/query-graphs-js/src/conditionsCaching.ts @@ -1,9 +1,9 @@ import { SelectionSet, assert } from "@apollo/federation-internals"; import { ConditionResolution, ConditionResolver, ExcludedConditions, ExcludedDestinations, sameExcludedDestinations } from "./graphPath"; import { PathContext } from "./pathContext"; -import { Edge, QueryGraph, QueryGraphState } from "./querygraph"; +import { Edge, QueryGraphState } from "./querygraph"; -export function cachingConditionResolver(graph: QueryGraph, resolver: ConditionResolver): ConditionResolver { +export function cachingConditionResolver(resolver: ConditionResolver): ConditionResolver { // For every edge having a condition, we cache the resolution its conditions when possible. // We save resolution with the set of excluded edges that were used to compute it: the reason we do this is // that excluded edges impact the resolution, so we should only used a cached value if we know the excluded @@ -13,7 +13,7 @@ export function cachingConditionResolver(graph: QueryGraph, resolver: ConditionR // when we have no excluded edges, we'd only ever use the cache for the first key of every type. However, // as the algorithm always try keys in the same order (the order of the edges in the query graph), including // the excluded edges we see on the first ever call is actually the proper thing to do. - const cache = new QueryGraphState(graph); + const cache = new QueryGraphState(); return (edge: Edge, context: PathContext, excludedDestinations: ExcludedDestinations, excludedConditions: ExcludedConditions, extraConditions?: SelectionSet) => { assert(edge.conditions || extraConditions, 'Should not have been called for edge without conditions'); diff --git a/query-graphs-js/src/conditionsValidation.ts b/query-graphs-js/src/conditionsValidation.ts index 406a5961c..7bae900ad 100644 --- a/query-graphs-js/src/conditionsValidation.ts +++ b/query-graphs-js/src/conditionsValidation.ts @@ -121,5 +121,5 @@ export function simpleValidationConditionResolver({ // condition is validated. Note that we use a cost of 1 for all conditions as we don't care about efficiency. return { satisfied: true, cost: 1 }; }; - return withCaching ? cachingConditionResolver(queryGraph, resolver) : resolver; + return withCaching ? cachingConditionResolver(resolver) : resolver; } diff --git a/query-graphs-js/src/graphviz.ts b/query-graphs-js/src/graphviz.ts index 38437c46e..c1b2ad74a 100644 --- a/query-graphs-js/src/graphviz.ts +++ b/query-graphs-js/src/graphviz.ts @@ -100,7 +100,7 @@ function addToVizGraph(graph: QueryGraph, vizGraph: GraphBaseModel, noTerminal: } return vizGraph; } - const state = new QueryGraphState(graph); + const state = new QueryGraphState(); const onEdge = function (edge: Edge): boolean { const head = edge.head; const tail = edge.tail; diff --git a/query-graphs-js/src/nonTrivialEdgePrecomputing.ts b/query-graphs-js/src/nonTrivialEdgePrecomputing.ts index e63976778..55a93de80 100644 --- a/query-graphs-js/src/nonTrivialEdgePrecomputing.ts +++ b/query-graphs-js/src/nonTrivialEdgePrecomputing.ts @@ -2,7 +2,7 @@ import { assert } from "@apollo/federation-internals"; import { Edge, QueryGraph, QueryGraphState, simpleTraversal } from "./querygraph"; export function preComputeNonTrivialFollowupEdges(graph: QueryGraph): (previousEdge: Edge) => readonly Edge[] { - const state = new QueryGraphState(graph); + const state = new QueryGraphState(); simpleTraversal(graph, () => {}, (edge) => { const followupEdges = graph.outEdges(edge.tail); state.setEdgeState(edge, computeNonTrivialFollowups(edge, followupEdges)); diff --git a/query-graphs-js/src/querygraph.ts b/query-graphs-js/src/querygraph.ts index fe716de8b..1d98286c1 100644 --- a/query-graphs-js/src/querygraph.ts +++ b/query-graphs-js/src/querygraph.ts @@ -476,16 +476,8 @@ export class QueryGraph { */ export class QueryGraphState { // Store some "user" state for each vertex (accessed by index) - private readonly verticesStates: (VertexState | undefined)[]; - private readonly adjacenciesStates: (EdgeState | undefined)[][]; - - /** - * Creates a new `QueryGraphState` to associate state to the vertices and/or edges of `graph`. - */ - constructor(readonly graph: QueryGraph) { - this.verticesStates = new Array(graph.verticesCount()); - this.adjacenciesStates = new Array(graph.verticesCount()); - } + private readonly verticesStates: Map = new Map(); + private readonly adjacenciesStates: Map> = new Map(); /** * Associates the provided state to the provided vertex. @@ -496,7 +488,7 @@ export class QueryGraphState { * @param state - the state/value to associate to `vertex`. */ setVertexState(vertex: Vertex, state: VertexState) { - this.verticesStates[vertex.index] = state; + this.verticesStates.set(vertex.index, state); } /** @@ -507,7 +499,7 @@ export class QueryGraphState { * `QueryGraphState` was created (and its behavior is undefined if it isn't). */ removeVertexState(vertex: Vertex) { - this.verticesStates[vertex.index] = undefined; + this.verticesStates.delete(vertex.index); } /** @@ -519,7 +511,7 @@ export class QueryGraphState { * @return the state associated to `vertex`, if any. */ getVertexState(vertex: Vertex): VertexState | undefined { - return this.verticesStates[vertex.index]; + return this.verticesStates.get(vertex.index); } /** @@ -531,10 +523,12 @@ export class QueryGraphState { * @param state - the state/value to associate to `edge`. */ setEdgeState(edge: Edge, state: EdgeState) { - if (!this.adjacenciesStates[edge.head.index]) { - this.adjacenciesStates[edge.head.index] = new Array(this.graph.outEdgesCount(edge.head)); + let edgeMap = this.adjacenciesStates.get(edge.head.index) + if (!edgeMap) { + edgeMap = new Map(); + this.adjacenciesStates.set(edge.head.index, edgeMap); } - this.adjacenciesStates[edge.head.index][edge.index] = state; + edgeMap.set(edge.index, state); } /** @@ -545,7 +539,13 @@ export class QueryGraphState { * `QueryGraphState` was created (and its behavior is undefined if it isn't). */ removeEdgeState(edge: Edge) { - this.adjacenciesStates[edge.head.index][edge.index] = undefined; + const edgeMap = this.adjacenciesStates.get(edge.head.index); + if (edgeMap) { + edgeMap.delete(edge.index); + if (edgeMap.size === 0) { + this.adjacenciesStates.delete(edge.head.index); + } + } } /** @@ -557,16 +557,21 @@ export class QueryGraphState { * @return the state associated to `edge`, if any. */ getEdgeState(edge: Edge): EdgeState | undefined { - const forEdge = this.adjacenciesStates[edge.head.index]; - return forEdge ? forEdge[edge.index] : undefined; + return this.adjacenciesStates.get(edge.head.index)?.get(edge.index); } toDebugString( vertexMapper: (s: VertexState) => string, edgeMapper: (e: EdgeState) => string ): string { - const vs = this.verticesStates.map((state, idx) => ` ${idx}: ${!state ? "" : vertexMapper(state)}`).join("\n"); - const es = this.adjacenciesStates.map((adj, vIdx) => adj.map((state, eIdx) => ` ${vIdx}[${eIdx}]: ${!state ? "" : edgeMapper(state)}`).join("\n")).join("\n"); + const vs = Array.from(this.verticesStates.entries()).sort(([a], [b]) => a - b).map(([idx, state]) => + ` ${idx}: ${!state ? "" : vertexMapper(state)}` + ).join("\n"); + const es = Array.from(this.adjacenciesStates.entries()).sort(([a], [b]) => a - b).map(([vIdx, adj]) => + Array.from(adj.entries()).sort(([a], [b]) => a - b).map(([eIdx, state]) => + ` ${vIdx}[${eIdx}]: ${!state ? "" : edgeMapper(state)}` + ).join("\n") + ).join("\n"); return `vertices = {${vs}\n}, edges = {${es}\n}`; } } diff --git a/query-planner-js/src/buildPlan.ts b/query-planner-js/src/buildPlan.ts index 66cfd986d..6a27faaa1 100644 --- a/query-planner-js/src/buildPlan.ts +++ b/query-planner-js/src/buildPlan.ts @@ -400,7 +400,6 @@ class QueryPlanningTraversal { this.isTopLevel = isRootVertex(root); this.optionsLimit = parameters.config.debug?.pathsLimit; this.conditionResolver = cachingConditionResolver( - federatedQueryGraph, (edge, context, excludedEdges, excludedConditions, extras) => this.resolveConditionPlan(edge, context, excludedEdges, excludedConditions, extras), );