Fix potential inefficient planning due to __typename #2137
Conversation
👷 Deploy request for apollo-federation-docs pending review.Visit the deploys page to approve it
|
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
|
Note: I'm not absolutely in love with my solution here in principle: the "attachement" thing feels a bit of a hack. But it's the simplest solution that I could convince myself would work, and it's relatively simple and "contained", so figure this might do, at least until some better idea comes in. |
| @@ -86,7 +107,6 @@ export class Field<TArgs extends {[key: string]: any} = {[key: string]: any}> ex | |||
| readonly alias?: string | |||
| ) { | |||
| super(definition.schema(), variablesInArguments(args)); | |||
| this.validate(); | |||
There was a problem hiding this comment.
Note to reviewers: somewhat unrelated minor optimisation. While starting to look at this issue, I fired up profiling to see if anything specific showed up. While nothing egregious showed up, this validation came up higher than it should have, and upon reflexion, doing that validation in the constructor was a bit heavy. And since we have a validate() method on SelectionSet that we call when needed (but less aggressively), simply made it so this Field.validate was called only at the same time (outside of performance, this even feel a bit more expected anyway).
| @@ -1261,7 +1287,11 @@ export class SelectionSet extends Freezable<SelectionSet> { | |||
| clone(): SelectionSet { | |||
| const cloned = new SelectionSet(this.parentType); | |||
| for (const selection of this.selections()) { | |||
| cloned.add(selection.clone()); | |||
| const clonedSelection = selection.clone(); | |||
There was a problem hiding this comment.
Notes to reviewer: another minor stuff that profiling showed up. I could probably have put those into a separate commit, but I was lazy. Sorry.
| operationName, | ||
| ); | ||
|
|
||
| operation = operation.optimize(fragments); |
There was a problem hiding this comment.
Note to reviewers: a very minor optimistic refactor. The optimize method was called in each of operationForEntitiesFetch and operationForQueryFetch which was both less DRY, but also less readable in the sense that we always want to call that method, and it has no reason to depend on the kind of operation we deal with.
The code from apollographql#2137 is optimize some use of __typename when other fields are queried alongside it. But when __typename had only inline fragments alongside it, the logic was flawed and was discarding any potential instance of the optimization done somewhere more nested. This fixes that problem.
The optimization of apollographql#2137 is meant to have no impact on the generated plans: it's an optimization that ensure the query planner does not needlessly evaluate ineffient options just because __typename happens to be resolvable from any subgraph. But this make testing that optimization a bit more complicated. To work around this, this patch adds a new behaviour to the query planner whereby along the generation of the plan, it also exposes some statistics on the plan generation. As of this commit, the only thing exposed is the number of plan that were evaluated under the hood, as that is what we care to check here (but it is also one of the main contributor to time spent query planning, so arguably an important statistic). There was probably more hacky ways to test this optimization, but it while this statistic is currently only use for testing, this feels like something that could have other uses and be useful to enrich later.
The code from #2137 is optimizing some use of __typename when other fields are queried alongside it. But when __typename had only inline fragments alongside it, the logic was flawed and was discarding any potential instance of the optimization done somewhere more nested. This fixes that problem. Additionally, this patch adds a test for the optimization of #2137, but to do that, this patch adds a new behaviour to the query planner whereby along the generation of the plan, it also exposes some statistics on the plan generation. As of this commit, the only thing exposed is the number of plan that were evaluated under the hood, as that is what we care to check here (but it is also one of the main contributor to time spent query planning, so arguably an important statistic).
This PR fixes a query planning performance issue discovered with some user data. Namely, planning for some particular queries was taking a very noticeable amount of time (~30 sec on my somewhat fancy laptop for reference).
Upon inspection, it was due to some large amount of plans being built and compared, but the only reason there were many plans considered was due to the consideration of multiple options for the
__typenamefield in a number of places. Obviously, since__typenamecan be requested equally from any subgraph (as long as it knows the type), evaluating all those options was particularly wasteful.The attaching commit avoids that problem by essentially making the query planning "ignore"
__typenameas it computes all the possible choices/plans to consider, and then add it back where it should at the very end when finalising the plan.On the particular example mentioned above, this reduces the number of plans to consider/evaluate by the query planner to just one, and so query planner is reduced to some ~300ms (again, on my laptop, and it's a fairly large query/plan).
Testing this, I randomly run into a (very) minor bug with aliasing
__typename, in that if one was to query__typenametwice in the same selection by using an alias, the aliased version was discarded. This PR also opportunistically fixes this. Tbc, I can't think of a reason why anyone would request such a thing but it's valid graphQL and was definitively not handled properly so...