Faster approach to code for reusing fragments in fetches #2604
Conversation
👷 Deploy request for apollo-federation-docs pending review.Visit the deploys page to approve it
|
🦋 Changeset detectedLatest commit: 7df8461 The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
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. |
pcmanus
force-pushed
the
fragment-improvements
branch
3 times, most recently
from
7945a40
to
fb24aeb
Compare
pcmanus
force-pushed
the
fragment-improvements
branch
from
fb24aeb
to
a41376f
Compare
|
I'll note that as mentioned on apollographql/router#3172, while the motivation of this patch started as a performance improvement for the fragment reuse code, it also conceptually simplify said code, making easier to maintain. And it happens to fix the issue at apollographql/router#3172, so this is technically a bug fix. |
This commit somehwat "inverse" the way fragment reuse is tried. Before this commit, fragment reuse was tried depth-first, meaning that we tried reusing fragment on the leaf of the result set and then up on every selection. Unfortunately, doing so creates a number of subtlety that force the checks done on every selection to be somewhat costly, which adds up. This commit change the process a bit to test for fragment reuse "at the top level" first (and so always fully expanded selections), and recurse down for the part that didn't matched anything. In practice, doing so simplify things a bit due to always dealing with expanded selections, and this allow to the checks simpler and more efficient. Overall, this commit usually improves the time spend for trying fragment reuse during query planning (sometimes substantially), hence lowering query planning time.
pcmanus
force-pushed
the
fragment-improvements
branch
from
a41376f
to
35f4a0a
Compare
internals-js/src/operations.ts
Outdated
| // Computes for every fragment, which other fragments use it (so the reverse of it's dependencies, the other fragment it uses). | ||
| function computeFragmentsReverseDependencies(fragments: NamedFragments): SetMultiMap<string, string> { | ||
| const reverseDeps = new SetMultiMap<string, string>(); | ||
| for (const fragment of fragments.definitions()) { | ||
| for (const dependency of fragment.fragmentUsages().keys()) { | ||
| reverseDeps.add(dependency, fragment.name); | ||
| } | ||
| } | ||
| return reverseDeps; | ||
| } | ||
|
|
There was a problem hiding this comment.
The name of this function confused me a bit since nothing is being reversed. Maybe computeFragmentsDependents instead?
There was a problem hiding this comment.
Right, computedFragmentDependents is better. Changed to that.
.changeset/brown-bats-float.md
Outdated
| "@apollo/gateway": patch | ||
| --- | ||
|
|
||
| Improves the performance of the code used to try to reuse query named fragments, thus improving query planning |
There was a problem hiding this comment.
Add a note about the other issues being fixed?
There was a problem hiding this comment.
Good point. Re-worked the message to include them.
This commit somehwat "inverse" the way fragment reuse is tried. Before this commit, fragment reuse was tried depth-first, meaning that we tried reusing fragment on the leaf of the result set and then up on every selection. Unfortunately, doing so creates a number of subtlety that force the checks done on every selection to be somewhat costly, which adds up.
This commit change the process a bit to test for fragment reuse "at the top level" first (and so always fully expanded selections), and recurse down for the part that didn't matched anything. In practice, doing so simplify things a bit due to always dealing with expanded selections, and this allow to the checks simpler and more efficient.
Overall, this commit usually improves the time spend for trying fragment reuse during query planning (sometimes substantially), hence lowering query planning time.