Refactor selection set implementation to be immutable #2387
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
👷 Deploy request for apollo-federation-docs pending review.Visit the deploys page to approve it
|
🦋 Changeset detectedLatest commit: 11954af 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
selection-set-refactor
branch
from
5f584cf
to
d65fcb8
Compare
The current (before this commit) implementation of `SelectionSet` is inherently mutable: its `SelectionSet.add` method mutate the selection set it's called on. Which is at odds with the general query planner algorithm, which is the main user of `SelectionSet`, but is fundamentally based on dealing with immutable data (as we explore various "path" options and the possible plans, most things are shared (at least partly)). This mismatch between the query planner that really want immutable selection sets but an implementation that is mutable leads to less than optimal code complexity and performance. More specificially, if we want to use a `SelectionSet` into 2 separate branch of the algorithm, we usually have to do defensive deep copies, which is inefficient. And to mitigate those inefficiencies, we sometimes don't do those copies, but this lead to code that fragile and easy to break and has lead to bug previously (where we save a copy, but then a branch mistakenly mutated and thus impacted another branch mistakenly). A few tricks had been added to the implementation to help mitigate the risks (the `freeze` behaviour, and the copy-on-write in `FetchGroup`), but they add complexity and aren't always optimal performance wise. This commit thus rework the implementation/api of `SelectionSet` to make it an immutable data structure. Doing so makes the code a lot safer and easy to reason about. And it also provide some performance benefits: compared to current `main` over a set of production schema/query, this seem to provide around 20% improvement on average and up to almost 50% improvment on some specific cases (disclaimer: those benchmark are fairly unscientific so those numbers should be taken with a grain of salt, but the numbers are clear enough that this is a net measurable gain).
pcmanus
force-pushed
the
selection-set-refactor
branch
from
d65fcb8
to
ae71a71
Compare
Co-authored-by: Trevor Scheer <trevor.scheer@gmail.com>
Co-authored-by: Trevor Scheer <trevor.scheer@gmail.com>
clenfest
reviewed
Mar 15, 2023
| @@ -672,10 +631,14 @@ class FetchGroup { | |||
| readonly rootKind: SchemaRootKind, | |||
| readonly parentType: CompositeType, | |||
| readonly isEntityFetch: boolean, | |||
| private _selection: LazySelectionSet, | |||
| private _inputs?: LazySelectionSet, | |||
| private _selection: MutableSelectionSet<{ conditions: Conditions}>, | |||
There was a problem hiding this comment.
It's probably past time to change this to named parameters, especially when there are so many optionals.
There was a problem hiding this comment.
It's a private constructor and the public methods that calls it do use named parameters. Moving to named parameters here means separately declaring all those field and having trivial assignments for each. That'd be fine but it's a lot of noise and I like it the way it is.
| } | ||
|
|
||
|
|
||
| static of(selectionSet: SelectionSet): MutableSelectionSet { |
There was a problem hiding this comment.
It's indeed not used anymore, but I'd like to keep it both for symmetry with empty and because it make sense API wise and has a high change of being use soon enough (and it really isn't, it's hardly a maintenance burden).
trevor-scheer
approved these changes
Mar 15, 2023
Outside of minor typos/updates, the bulk of this change is switching how we collect used variables to be more efficient/avoid generating useless garbage.
pcmanus
pushed a commit
to pcmanus/federation
that referenced
this pull request
Mar 16, 2023
It turns out that we have had (since fed 2.0 at least, maybe earlier) a
slightly weird behaviour (a bug really) in the validation of selection
sets which impacts what we accept for the `fields` arg of `@key`,
`@requires` and `@provides`. Namely, assuming a field `t` returnin an
object type, we were accepting a selection like:
```
{
t {
a
b
}
t
}
```
even though the 2nd occurence of `t` is kind of incorrect according to
the graphQL spec since `t` should always have a sub-selection. But
the reason it works is that the code was merging the 1st and 2nd
occurrence of `t` before any validation was run, so internally the
selection is handled as just:
```
{
t {
a
b
}
}
```
Now, an assertion was added in apollographql#2387 that is triggered by the example
above, and that means that some `@key`, `@provides` or `@requires` that
were accepted (and were mostly correctly working) in currently released
versions would start erroring in 2.4 because of this.
To be clear, the historical behaviour is kind of wrong here, and we
should consider fixing it at some point. However, hard-failing on
upgrades is not very nice: we should probably introduce a warning
for a few versions before genuinely making this an error. Further,
the current assertion does not provide a very user friendly message.
In the meantime, this PR restore the status quo.
clenfest
pushed a commit
that referenced
this pull request
Mar 16, 2023
It turns out that we have had (since fed 2.0 at least, maybe earlier) a
slightly weird behaviour (a bug really) in the validation of selection
sets which impacts what we accept for the `fields` arg of `@key`,
`@requires` and `@provides`. Namely, assuming a field `t` returnin an
object type, we were accepting a selection like:
```
{
t {
a
b
}
t
}
```
even though the 2nd occurence of `t` is kind of incorrect according to
the graphQL spec since `t` should always have a sub-selection. But
the reason it works is that the code was merging the 1st and 2nd
occurrence of `t` before any validation was run, so internally the
selection is handled as just:
```
{
t {
a
b
}
}
```
Now, an assertion was added in #2387 that is triggered by the example
above, and that means that some `@key`, `@provides` or `@requires` that
were accepted (and were mostly correctly working) in currently released
versions would start erroring in 2.4 because of this.
To be clear, the historical behaviour is kind of wrong here, and we
should consider fixing it at some point. However, hard-failing on
upgrades is not very nice: we should probably introduce a warning
for a few versions before genuinely making this an error. Further,
the current assertion does not provide a very user friendly message.
In the meantime, this PR restore the status quo.
pcmanus
added a commit
to pcmanus/federation
that referenced
this pull request
Aug 10, 2023
The previously committed [apollographql#2713](apollographql#2713) fixed an issue introduced by [apollographql#2387](apollographql#2387), ensuring that querying the same field with different directives applications was not merged, similar to what was/is done for fragments. But the exact behaviour slightly differs between fields and fragments when it comes to `@defer` in that for fragments, we never merge 2 similar fragments where both have `@defer`, which we do merge for fields. Or to put it more concretely, in the following query: ```graphq query Test($skipField: Boolean!) { x { ... on X @defer { a } ... on X @defer { b } } } ``` the 2 `... on X @defer` are not merged, resulting in 2 deferred sections that can run in parallel. But following [apollographql#2713](apollographql#2713), query: ```graphq query Test($skipField: Boolean!) { x @defer { a } x @defer { b } } ``` _will_ merge both `x @defer`, resulting in a single deferred section. This fix changes that later behaviour so that the 2 `x @defer` are not merged and result in 2 deferred sections, consistently with both 1) the case of fragments and 2) the behaviour prior to [apollographql#2387](apollographql#2387).
jeffjakub
pushed a commit
that referenced
this pull request
Aug 15, 2023
…ly (#2720) The previously committed [#2713](#2713) fixed an issue introduced by [#2387](#2387), ensuring that querying the same field with different directives applications was not merged, similar to what was/is done for fragments. But the exact behaviour slightly differs between fields and fragments when it comes to `@defer` in that for fragments, we never merge 2 similar fragments where both have `@defer`, which we do merge for fields. Or to put it more concretely, in the following query: ```graphq query Test($skipField: Boolean!) { x { ... on X @defer { a } ... on X @defer { b } } } ``` the 2 `... on X @defer` are not merged, resulting in 2 deferred sections that can run in parallel. But following [#2713](#2713), query: ```graphq query Test($skipField: Boolean!) { x @defer { a } x @defer { b } } ``` _will_ merge both `x @defer`, resulting in a single deferred section. This fix changes that later behaviour so that the 2 `x @defer` are not merged and result in 2 deferred sections, consistently with both 1) the case of fragments and 2) the behaviour prior to [#2387](#2387).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The current (before this commit) implementation of
SelectionSetis inherently mutable: itsSelectionSet.addmethod mutate the selection set it's called on. Which is at odds with the general query planner algorithm, which is the main user ofSelectionSet, but is fundamentally based on dealing with immutable data (as we explore various "path" options and the possible plans, most things are shared (at least partly)).This mismatch between the query planner that really want immutable selection sets but an implementation that is mutable leads to less than optimal code complexity and performance. More specificially, if we want to use a
SelectionSetinto 2 separate branch of the algorithm, we usually have to do defensive deep copies, which is inefficient. And to mitigate those inefficiencies, we sometimes don't do those copies, but this lead to code that fragile and easy to break and has lead to bug previously (where we save a copy, but then a branch mistakenly mutated and thus impacted another branch mistakenly). A few tricks had been added to the implementation to help mitigate the risks (thefreezebehaviour, and the copy-on-write inFetchGroup), but they add complexity and aren't always optimal performance wise.This commit thus rework the implementation/api of
SelectionSetto make it an immutable data structure. Doing so makes the code a lot safer and easy to reason about. And it also provide some performance benefits: compared to currentmainover a set of production schema/query, this seem to provide around 20% improvement on average and up to almost 50% improvment on some specific cases for the computation of query plans (disclaimer: those benchmark are fairly unscientific so those numbers should be taken with a grain of salt, but the numbers are clear enough that this is a net measurable gain).