diff --git a/.changeset/slimy-geckos-invite.md b/.changeset/slimy-geckos-invite.md new file mode 100644 index 000000000..3af5ae547 --- /dev/null +++ b/.changeset/slimy-geckos-invite.md @@ -0,0 +1,7 @@ +--- +"@apollo/query-planner": patch +"@apollo/query-graphs": patch +--- + +Fix query planner heuristic that could lead to ignoring some valid option and yielding a non-optimal query plan. + \ No newline at end of file diff --git a/query-graphs-js/src/graphPath.ts b/query-graphs-js/src/graphPath.ts index 8bec284f5..6b7aad4cc 100644 --- a/query-graphs-js/src/graphPath.ts +++ b/query-graphs-js/src/graphPath.ts @@ -232,15 +232,53 @@ export class GraphPath): { + thisJumps: number, + thatJumps: number + } { + const { vertex, index } = this.findLastCommonVertex(that); + return { + thisJumps: this.subgraphJumpsAtIdx(vertex, index), + thatJumps: that.subgraphJumpsAtIdx(vertex, index), + }; + } + + private findLastCommonVertex(that: GraphPath): { vertex: Vertex, index: number } { + let vertex: Vertex = this.root; + assert(that.root === vertex, () => `Expected both path to start on the same root, but 'this' has root ${vertex} while 'that' has ${that.root}`); + + const minSize = Math.min(this.size, that.size); + let index = 0; + for (; index < minSize; index++) { + const thisEdge = this.edgeAt(index, vertex); + const thatEdge = that.edgeAt(index, vertex); + if (thisEdge !== thatEdge) { + break; + } + if (thisEdge) { + vertex = thisEdge.tail; + } + } + return { vertex, index}; + } + + private subgraphJumpsAtIdx(vertex: Vertex, index: number): number { let jumps = 0; - let v: Vertex = this.root; - for (let i = 0; i < this.size; i++) { + let v: Vertex = vertex; + for (let i = index; i < this.size; i++) { const edge = this.edgeAt(i, v); if (!edge) { continue; } - if (edge.transition.kind === 'KeyResolution' || edge.transition.kind === 'RootTypeResolution') { + if (edge.changesSubgraph()) { ++jumps; } v = edge.tail; @@ -248,6 +286,77 @@ export class GraphPath): boolean { + // We're looking a the specific case were both path are basically equivalent except + // for a single step of type-explosion, so if either the paths don't start and end in the + // same vertex, or if `other` is not exactly 1 more step than `this`, we're done. + if (this.root !== that.root || this.tail !== that.tail || this.size !== that.size - 1) { + return false; + } + + // If that's true, then we get to our comparison. + let thisV: Vertex = this.root; + let thatV: Vertex = that.root; + for (let i = 0; i < this.size; i++) { + let thisEdge = this.edgeAt(i, thisV); + let thatEdge = that.edgeAt(i, thatV); + if (thisEdge !== thatEdge) { + // First difference. If it's not a "type-explosion", that is `that` is a cast from an + // interface to one of the implementation, then we're not in the case we're looking for. + if (!thisEdge || !thatEdge || !isInterfaceType(thatV.type) || thatEdge.transition.kind !== 'DownCast') { + return false; + } + thatEdge = that.edgeAt(i+1, thatEdge.tail); + if (!thatEdge) { + return false; + } + thisV = thisEdge.tail; + thatV = thatEdge.tail; + + // At that point, we want both path to take the "same" key, but because one is starting + // from the interface while the other one from an implementation, they won't be technically + // the "same" edge object. So we check that both are key, to the same subgraph and type, + // and with the same condition. + if (thisEdge.transition.kind !== 'KeyResolution' + || thatEdge.transition.kind !== 'KeyResolution' + || thisEdge.tail.source !== thatEdge.tail.source + || thisV !== thatV + || !thisEdge.conditions!.equals(thatEdge.conditions!) + ) { + return false; + } + + // So far, so good. `thisV` and `thatV` are positioned on the vertex after which the path + // must be equal again. So check that it's true, and if it is, we're good. + // Note that for `this`, the last edge we looked at was `i`, so the next is `i+1`. And + // for `that`, we've skipped over one more edge, so need to use `j+1`. + for (let j = i + 1; j < this.size; j++) { + thisEdge = this.edgeAt(j, thisV); + thatEdge = that.edgeAt(j+1, thatV); + if (thisEdge !== thatEdge) { + return false; + } + if (thisEdge) { + thisV = thisEdge.tail; + thatV = thatEdge!.tail; + } + } + return true; + } + if (thisEdge) { + thisV = thisEdge.tail; + thatV = thatEdge!.tail; + } + } + // If we get here, both path are actually exactly the same. So technically there is not additional + // type explosion, but they are equivalent and we can return `true`. + return true; + } + [Symbol.iterator](): PathIterator { const path = this; return { @@ -1814,6 +1923,35 @@ export function advanceSimultaneousPathsWithOperation( // is unsatisfiable. But as we've only taken type preserving transitions, we cannot get an empty results at this point if we haven't // had one when testing direct transitions above (in which case we have exited the method early). assert(pathWithOperation.length > 0, () => `Unexpected empty options after non-collecting path ${pathWithNonCollecting} for ${operation}`); + + // There is a special case we can deal with now. Namely, suppose we have a case where a query + // is reaching an interface I in a subgraph S1, we query some field of that interface x, and + // say that x is provided in subgraph S2 but by an @interfaceObject for I. + // As we look for direct options for I.x in S1 initially, as we didn't found `x`, we will have tried + // to type-explode I (in say implementation A and B). And in some case doing so is necessary, but + // it may also lead for the type-exploding option to look like: + // [ + // I(S1) -[... on A]-> A(S1) -[key]-> I(S2) -[x] -> Int(S2), + // I(S1) -[... on B]-> B(S1) -[key]-> I(S2) -[x] -> Int(S2), + // ] + // But as we look at indirect option now (still from I in S1), we will note that we can also + // do: + // I(S1) -[key]-> I(S2) -[x] -> Int(S2), + // And while both options are technically valid, the new one really subsume the first one: there + // is no point in type-exploding to take a key to the same exact subgraph if using the key + // on the interface directly works. + // + // So here, we look for that case and remove any type-exploding option that the new path + // render unecessary. + // Do note that we only make that check when the new option is a single-path option, because + // this gets kind of complicated otherwise. + if (pathWithNonCollecting.tailIsInterfaceObject()) { + for (const indirectOption of pathWithOperation) { + if (indirectOption.length === 1) { + options = options.filter((opt) => !opt.every((p) => indirectOption[0].isEquivalentSaveForTypeExplosionTo(p))); + } + } + } options = options.concat(pathWithOperation); } debug.groupEnd(); @@ -1853,6 +1991,7 @@ export function advanceSimultaneousPathsWithOperation( return createLazyOptions(allOptions, subgraphSimultaneousPaths, updatedContext); } + export function createInitialOptions( initialPath: OpGraphPath, initialContext: PathContext, diff --git a/query-planner-js/src/__tests__/buildPlan.test.ts b/query-planner-js/src/__tests__/buildPlan.test.ts index 00fb20d8d..3ccd3e702 100644 --- a/query-planner-js/src/__tests__/buildPlan.test.ts +++ b/query-planner-js/src/__tests__/buildPlan.test.ts @@ -4423,8 +4423,7 @@ describe('__typename handling', () => { t: T @shareable } - type T @key(fields: "id") { - id: ID! + type T { x: Int } ` @@ -4438,7 +4437,7 @@ describe('__typename handling', () => { t: T @shareable } - type T @key(fields: "id") { + type T { id: ID! y: Int } @@ -6170,3 +6169,98 @@ describe('`debug.maxEvaluatedPlans` configuration', () => { }); }); +test('correctly generate plan built from some non-individually optimal branch options', () => { + // The idea of this test is that the query has 2 leaf fields, `t.x` and `t.y`, whose + // options are: + // 1. `t.x`: + // a. S1(get t.x) + // b. S2(get t.id) -> S3(get t.x using key id) + // 2. `t.y`: + // a. S2(get t.id) -> S3(get t.y using key id) + // + // And the idea is that "individually", for just `t.x`, getting it all in `S1` using option a., + // but for the whole plan, using option b. is actually better since it avoid querying `S1` + // entirely (and `S2`/`S2` have to be queried anyway). + // + // Anyway, this test make sure we do correctly generate the plan using 1.b and 2.a, and do + // not ignore 1.b in favor of 1.a in particular (which a bug did at one point). + + const subgraph1 = { + name: 'Subgraph1', + typeDefs: gql` + type Query { + t: T @shareable + } + + type T { + x: Int @shareable + } + ` + } + + const subgraph2 = { + name: 'Subgraph2', + typeDefs: gql` + type Query { + t: T @shareable + } + + type T @key(fields: "id") { + id: ID! + } + ` + } + + const subgraph3 = { + name: 'Subgraph3', + typeDefs: gql` + type T @key(fields: "id") { + id: ID! + x: Int @shareable + y: Int + } + ` + } + + const [api, queryPlanner] = composeAndCreatePlanner(subgraph1, subgraph2, subgraph3); + const operation = operationFromDocument(api, gql` + { + t { + x + y + } + } + `); + + const plan = queryPlanner.buildQueryPlan(operation); + expect(plan).toMatchInlineSnapshot(` + QueryPlan { + Sequence { + Fetch(service: "Subgraph2") { + { + t { + __typename + id + } + } + }, + Flatten(path: "t") { + Fetch(service: "Subgraph3") { + { + ... on T { + __typename + id + } + } => + { + ... on T { + y + x + } + } + }, + }, + }, + } + `); +}); diff --git a/query-planner-js/src/buildPlan.ts b/query-planner-js/src/buildPlan.ts index 166ec9231..e06c78332 100644 --- a/query-planner-js/src/buildPlan.ts +++ b/query-planner-js/src/buildPlan.ts @@ -280,6 +280,94 @@ function selectionIsFullyLocalFromAllVertices( return true; } +/** + * Given 2 paths that are 2 different options to reach the same query leaf field, checks if one can be shown + * to be always "better" (more efficient/optimal) than the other one, and this regardless of any surrounding context (that + * is regardless of what the rest of the query plan would be for any other query leaf field. + * + * Note that this method is used on final options of a given "query path", so all the heuristics done within `GraphPath` + * to avoid unecessary option have already been applied (say, avoiding to consider a path that do 2 successives key jumps + * when there is a 1 jump equivalent, ...), so this focus on what can be done based on the fact that the path considered + * are "finished". + * + * @return -1 if `opt1` is known to be strictly better than `opt2`, 1 if it is `opt2` that is strictly better, and 0 if we + * cannot really guarantee anything (at least "out of context"). + */ +export function compareOptionsComplexityOutOfContext(opt1: SimultaneousPaths, opt2: SimultaneousPaths): number { + // Currently, we only every compare single-path options. We may find smart things to do for multi-path options later, + // but unsure what currently. + if (opt1.length === 1) { + if (opt2.length === 1) { + return compareSinglePathOptionsComplexityOutOfContext(opt1[0], opt2[0]); + } else { + return compareSingleVsMultiPathOptionsComplexityOutOfContext(opt1[0], opt2); + } + } else if (opt2.length === 1) { + return -compareSingleVsMultiPathOptionsComplexityOutOfContext(opt2[0], opt1); + } + return 0; +} + +function compareSinglePathOptionsComplexityOutOfContext(p1: OpGraphPath, p2: OpGraphPath): number { + // Currently, this method only handle the case where we have something like: + // - `p1`: -[t]-> T(A) -[u]-> U(A) -[x] -> Int(A) + // - `p2`: -[t]-> T(A) -[key]-> T(B) -[u]-> U(B) -[x] -> Int(B) + // That is, we have 2 choices that are identical up to the "end", when one stays in the subgraph (p1, which stays in A) + // while the other use a key to another subgraph (p2, going to B). + // + // In such a case, whatever else the a query might be doing, it can never be "worst" + // to use `p1` than to use `p2` because both will force the same "fetch group" up to the + // end, but `p2` may force one more fetch that `p` does not. + // Do note that we say "may" above, because the rest of the plan may well have a forced + // choice like: + // - `other`: -[t]-> T(A) -[key]-> T(B) -[u]-> U(B) -[y] -> Int(B) + // in which case the plan will have the jump from A to B after `t` whether we use `p1` or + // `p2`, but while in that particular case `p1` and `p2` are about comparable in term + // of performance, `p1` is still not worst than `p2` (and in other situtation, `p1` may + // genuinely be better). + // + // Note that this is in many ways just a generalization of a heuristic we use earlier for leaf field. That is, + // we will never get as input to this method something like: + // - `p1`: -[t]-> T(A) -[x] -> Int(A) + // - `p2`: -[t]-> T(A) -[key]-> T(B) -[x] -> Int(B) + // because when the code is asked for option for `x` after ` -[t]-> T(A)`, it notices that `x` + // is a leaf and is in `A`, so it doesn't ever look for alternative path. But this only work for direct + // leaf of an entity. In the example at the beginning, field `u` makes this not working, because when + // we compute choices for `u`, we don't yet know what comes after that, and so we have to take the option + // of going to subgraph `B` into account (it may very be that whatever comes after `u` is not in `A` for + // instance). + if (p1.tail.source !== p2.tail.source) { + const { thisJumps: p1Jumps, thatJumps: p2Jumps } = p1.countSubgraphJumpsAfterLastCommonVertex(p2); + // As described above, we want to known if one of the path has no jumps at all (after the common prefix) while + // the other do have some. + if (p1Jumps === 0 && p2Jumps > 0) { + return -1; + } else if (p1Jumps > 0 && p2Jumps === 0) { + return 1; + } else { + return 0; + } + } + return 0; +} + +function compareSingleVsMultiPathOptionsComplexityOutOfContext(p1: OpGraphPath, p2s: SimultaneousPaths): number { + // This handle the same case than for the single-path only case, but compares the single path against + // each of the option of the multi-path, and only "ignore" the multi-path if all its paths can be ignored. + // Note that this happens less often than the single-path only case, but with @provides on an interface, you can + // have case where one the one side you can get something entirely on the current graph, but the type-exploded case + // has still be generated due to the leaf field not being the one just after "provided" interface. + for (const p2 of p2s) { + // Note: not sure if it is possible for a branch of the multi-path option to subsume the single-path one in practice, but + // if it does, we ignore it because it's not obvious that this is enough to get rid of `p1` (maybe `p1` is provably a bit + // costlier than one of the path of `p2s`, but `p2s` may have many paths and could still be collectively worst than `p1`). + if (compareSinglePathOptionsComplexityOutOfContext(p1, p2) >= 0) { + return 0; + } + } + return -1; +} + class QueryPlanningTraversal { // The stack contains all states that aren't terminal. private bestPlan: [FetchDependencyGraph, OpPathTree, number] | undefined; @@ -338,6 +426,12 @@ class QueryPlanningTraversal { return this.bestPlan; } + private recordClosedBranch(closed: ClosedBranch) { + const maybeTrimmed = this.maybeEliminateStrictlyMoreCostlyPaths(closed); + debug.log(() => `Closed branch has ${maybeTrimmed.length} options (eliminated ${closed.length - maybeTrimmed.length} that could be proved as unecessary)`); + this.closedBranches.push(maybeTrimmed); + } + private handleOpenBranch(selection: Selection, options: SimultaneousPathsWithLazyIndirectPaths[]) { const operation = selection.element; debug.group(() => `Handling open branch: ${operation}`); @@ -376,7 +470,7 @@ class QueryPlanningTraversal { // Do note that we'll only need that `__typename` if there is no other selections inside `foo`, and so we might include // it unecessarally in practice: it's a very minor inefficiency though. if (operation.kind === 'FragmentElement') { - this.closedBranches.push(options.map((o) => ({ paths: o.paths.map(p => terminateWithNonRequestedTypenameField(p))}))); + this.recordClosedBranch(options.map((o) => ({ paths: o.paths.map(p => terminateWithNonRequestedTypenameField(p))}))); } debug.groupEnd(() => `Terminating branch with no possible results`); return; @@ -418,7 +512,7 @@ class QueryPlanningTraversal { // as we should (we add `__typename` for abstract types on the "normal path" and so we add them too to // named fragments; as such, we need them here too). const selectionSet = addTypenameFieldForAbstractTypes(addBackTypenameInAttachments(selection.selectionSet)); - this.closedBranches.push(newOptions.map((opt) => ({ paths: opt.paths, selection: selectionSet }))); + this.recordClosedBranch(newOptions.map((opt) => ({ paths: opt.paths, selection: selectionSet }))); } else { for (const branch of mapOptionsToSelections(selection.selectionSet, newOptions)) { this.stack.push(branch); @@ -426,57 +520,62 @@ class QueryPlanningTraversal { } debug.groupEnd(); } else { - const updated = this.maybeEliminateStrictlyMoreCostlyPaths(newOptions); - this.closedBranches.push(updated); - debug.groupEnd(() => `Branch finished with ${updated.length} options`); - } - } - - // This method should be applied to "final" paths, that is when the tail of the paths is a leaf field. - // TODO: this method was added for cases where we had the following options: - // 1) _ -[f1]-> T1(A) -[f2]-> T2(A) -[f3]-> T3(A) -[f4]-> Int(A) - // 2) _ -[f1]-> T1(A) -[f2]-> T2(A) -[key]-> T2(B) -[f3]-> T3(B) -[f4] -> Int(B) - // where clearly the 2nd option is not necessary (we're in A up to T2 in both case, so staying in A is never - // going to be more expensive that going to B; note that if _other_ branches do jump to B after T2(A) for - // other fieleds, the option 2 might well lead to a plan _as_ efficient as with option 1, but it will - // not be _more_ efficient). - // Anyway, while the implementation does handle this case, I believe it's a bit over-generic and can - // eliminiate options we could want to keep. Double-check that and fix. - private maybeEliminateStrictlyMoreCostlyPaths( - options: SimultaneousPathsWithLazyIndirectPaths[] - ): ClosedBranch { - if (options.length === 1) { - return [{ paths: options[0].paths }]; - } - - const singlePathOptions = options.filter(opt => opt.paths.length === 1); - if (singlePathOptions.length === 0) { - // we can't easily compare multi-path options - return options.map(opt => ({ paths: opt.paths })); - } - - let minJumps = Number.MAX_SAFE_INTEGER; - let withMinJumps: ClosedBranch = []; - for (const option of singlePathOptions) { - const jumps = option.paths[0].subgraphJumps(); - const closedBranch = { paths: option.paths }; - if (jumps < minJumps) { - minJumps = jumps; - withMinJumps = [closedBranch]; - } else if (jumps === minJumps) { - withMinJumps.push(closedBranch); - } + this.recordClosedBranch(newOptions.map((opt) => ({ paths: opt.paths }))); + debug.groupEnd(() => `Branch finished`); } + } - // We then look at multi-path options. We can exclude those if the path with the least amount of jumps is - // more than our minJumps - for (const option of singlePathOptions.filter(opt => opt.paths.length > 1)) { - const jumps = option.paths.reduce((acc, p) => Math.min(acc, p.subgraphJumps()), Number.MAX_SAFE_INTEGER); - if (jumps <= minJumps) { - withMinJumps.push({ paths: option.paths }); + /** + * This method is called on a closed branch, that is on all the possible options found + * to get a particular leaf of the query being planned. And when there is more than one + * option, it tries a last effort at checking an option can be shown to be less efficient + * than another one _whatever the rest of the query plan is_ (that is, whatever the options + * for any other leaf of the query are). + * + * In practice, this compare all pair of options and call the heuristics + * of `compareOptionsComplexityOutOfContext` on them to see if one strictly + * subsume the other (and if that's the case, the subsumed one is ignored). + */ + private maybeEliminateStrictlyMoreCostlyPaths(branch: ClosedBranch): ClosedBranch { + if (branch.length <= 1) { + return branch; + } + + // Copying the branch because we're going to modify in place. + const toHandle = branch.concat(); + + const keptOptions: ClosedPath[] = []; + while (toHandle.length >= 2) { + const first = toHandle[0]; + let shouldKeepFirst = true; + // We compare `first` to every other remaining. But we iterate from end to beginning + // because we may remove in place some of those we iterate on and that makes it safe. + for (let i = toHandle.length - 1 ; i >= 1; i--) { + const other = toHandle[i]; + const cmp = compareOptionsComplexityOutOfContext(first.paths, other.paths); + if (cmp < 0) { + // This means that `first` is always better than `other`. So we eliminate `other`. + toHandle.splice(i, 1); + } else if (cmp > 0) { + // This means that `first` is always worst than `other`. So we eliminate `first` ( + // and we're done with this inner loop). + toHandle.splice(0, 1); + shouldKeepFirst = false; + break; + } } + if (shouldKeepFirst) { + // Means that we found no other option that make first unecessary. We mark first as kept, + // and remove it from our working set (which we know it hasn't yet). + keptOptions.push(first); + toHandle.splice(0, 1); + } + } + // We know toHandle has at most 1 element, but it may have one and we should keep it. + if (toHandle.length > 0) { + keptOptions.push(toHandle[0]); } - return withMinJumps; + return keptOptions; } private newDependencyGraph(): FetchDependencyGraph { @@ -530,6 +629,14 @@ class QueryPlanningTraversal { return false; } + private sortOptionsInClosedBranches() { + this.closedBranches.forEach((branch) => branch.sort((p1, p2) => { + const p1Jumps = Math.max(...p1.paths.map((p) => p.subgraphJumps())); + const p2Jumps = Math.max(...p2.paths.map((p) => p.subgraphJumps())); + return p1Jumps - p2Jumps; + })); + } + private computeBestPlanFromClosedBranches() { if (this.closedBranches.length === 0) { return; @@ -543,6 +650,19 @@ class QueryPlanningTraversal { // that are known to be overriden by other ones. this.pruneClosedBranches(); + // We now sort the options within each branch, putting those with the least amount of subgraph jumps first. + // The idea is that for each branch taken individually, the option with the least jumps is going to be + // the most efficient, and while it is not always the case that the best plan is built for those + // individual bests, they are still statistically more likely to be part of the best plan. So putting + // them first has 2 benefits for the rest of this method: + // 1. if we end up cutting some options of a branch below (due to having too many possible plans), + // we'll cut the last option first (we `pop()`), so better cut what it the least likely to be good. + // 2. when we finally generate the plan, we use the cost of previously computed plans to cut computation + // early when possible (see `generateAllPlansAndFindBest`), so there is a premium in generating good + // plans early (it cuts more computation), and putting those more-likely-to-be-good options first helps + // this. + this.sortOptionsInClosedBranches(); + // We're out of smart ideas for now, so we look at how many plans we'd have to generate, and if it's // "too much", we reduce it to something manageable by arbitrarilly throwing out options. This effectively // means that when a query has too many options, we give up on always finding the "best" @@ -550,7 +670,8 @@ class QueryPlanningTraversal { // TODO: currently, when we need to reduce options, we do so somewhat arbitrarilly. More // precisely, we reduce the branches with the most options first and then drop the last // option of the branch, repeating until we have a reasonable number of plans to consider. - // However, there is likely ways to drop options in a more "intelligent" way. + // The sorting we do about help making this slightly more likely to be a good choice, but + // there is likely more "smarts" we could add to this. // We sort branches by those that have the most options first. this.closedBranches.sort((b1, b2) => b1.length > b2.length ? -1 : (b1.length < b2.length ? 1 : 0)); @@ -628,9 +749,9 @@ class QueryPlanningTraversal { if (!prevCost) { return `Computed plan with cost ${cost}: ${p.tree}`; } else if (cost > prevCost) { - return `Found better with cost ${cost} (previous had cost ${prevCost}: ${p.tree}`; - } else { return `Ignoring plan with cost ${cost} (a better plan with cost ${prevCost} exists): ${p.tree}` + } else { + return `Found better with cost ${cost} (previous had cost ${prevCost}: ${p.tree}`; } }); }, diff --git a/query-planner-js/src/generateAllPlans.ts b/query-planner-js/src/generateAllPlans.ts index 1f7916291..5f9d033a1 100644 --- a/query-planner-js/src/generateAllPlans.ts +++ b/query-planner-js/src/generateAllPlans.ts @@ -4,6 +4,7 @@ type Choices = (T | undefined)[]; type Partial = { partial: P, + partialCost?: number, remaining: Choices[], isRoot: boolean, index?: number, @@ -68,6 +69,12 @@ export function generateAllPlansAndFindBest({ best: P, cost: number, }{ + // Note: we save ourselves the computation of the cost of `initial` (we pass no `partialCost` in this initialization). + // That's because `partialCost` is about exiting early when we've found at least one full plan and the cost of that + // plan is already smaller than the cost of a partial computation. But any plan is going be built from `initial` with + // at least one 'choice' added to it, all plans are guaranteed to be more costly than `initial` anyway. Note that + // save for `initial`, we always compute `partialCost` as the pros of exiting some branches early are large enough + // that it outweight computing some costs unecessarily from time to time. const stack: Partial[] = [{ partial: initial, remaining: toAdd, @@ -78,7 +85,14 @@ export function generateAllPlansAndFindBest({ let min: { best: P, cost: number } | undefined = undefined; while (stack.length > 0) { - const { partial, remaining, isRoot, index } = stack.pop()!; + const { partial, partialCost, remaining, isRoot, index } = stack.pop()!; + + // If we've found some plan already, and the partial we have is already more costly than that, + // then no point continuing with it. + if (min !== undefined && partialCost !== undefined && partialCost >= min.cost) { + continue; + } + const nextChoices = remaining[0]; const otherChoices = remaining.slice(1); @@ -86,20 +100,20 @@ export function generateAllPlansAndFindBest({ const { extracted, updatedChoices, isLast } = extract(pickedIndex, nextChoices); if (!isLast) { - // First, re-insert what correspond to all the choices that dot _not_ pick `extracted`. + // First, re-insert what correspond to all the choices that do _not_ pick `extracted`. insertInStack({ partial, remaining: [updatedChoices].concat(otherChoices), isRoot, index: isRoot && index !== undefined && index < nextChoices.length - 1 ? index + 1 : undefined, + partialCost, }, stack); } const newPartial = addFct(partial, extracted); + const cost = costFct(newPartial); if (otherChoices.length === 0) { - // We have a complete plan. Compute the cost, check if it is best and based on that, - // provide it to `onGenerated` or discard it. - const cost = costFct(newPartial); + // We have a complete plan. If it is best, save it, otherwise, we're done with it. const isNewMin = min === undefined || cost < min.cost; onPlan(newPartial, cost, min?.cost); if (isNewMin) { @@ -111,22 +125,15 @@ export function generateAllPlansAndFindBest({ continue; } - if (min !== undefined) { - // We're not done, but we've already generated a plan with a score, so we check if - // what we have so far is already more costly, and if it is, we skip this branch - // entirely. - const cost = costFct(newPartial); - if (cost >= min.cost) { - continue; - } + if (min === undefined || cost < min.cost) { + insertInStack({ + partial: newPartial, + partialCost: cost, + remaining: otherChoices, + isRoot: false, + index, + }, stack); } - - insertInStack({ - partial: newPartial, - remaining: otherChoices, - isRoot: false, - index - }, stack); } assert(min, 'A plan should have been found');