chore(federation): borrow selection keys #6074
Conversation
goto-bus-stop
requested review from
dariuszkuc,
sachindshinde,
SimonSapin,
lrlna,
TylerBloom and
duckki
as code owners
|
@goto-bus-stop, please consider creating a changeset entry in |
|
CI performance tests
|
| } | ||
|
|
||
| impl HasSelectionKey for FragmentSpread { | ||
| fn key(&self) -> SelectionKey<'_> { | ||
| if is_deferred_selection(&self.directives) { |
There was a problem hiding this comment.
This is a tradeoff right now: we call key() much more frequently, and for inline fragments and fragment spreads it has to search through the directive list. In many cases it'll be free (because the directive list is None), but in some cases it will have to walk the list and do a few string comparisons. It could potentially be optimized by storing a .has_defer boolean in DirectiveList, but I didn't see it on flame graphs yet, so that could be overengineering it.
| @@ -206,7 +206,7 @@ impl SelectionSet { | |||
| )); | |||
| }; | |||
| fields | |||
| .entry(other_key) | |||
| .entry(other_key.to_owned_key()) | |||
There was a problem hiding this comment.
This is not easy to rewrite to borrowed keys, because it creates intermediate maps grouping selections that have the same key. We can probably rewrite merging without the intermediate maps entirely later. Note the previous version was also cloning keys here, so it's not a regression.
| pub(crate) fn schema(&self) -> &ValidFederationSchema { | ||
| &self.schema | ||
| } |
There was a problem hiding this comment.
Something to consider: We could derive these via the derive_getters crate. This would as a single transitive dependency to the router (i.e. just the added crate).
| /// Add selections from another selection map to this one. If there are key collisions, the | ||
| /// selections are *overwritten*. | ||
| pub(crate) fn extend(&mut self, other: SelectionMap) { | ||
| for selection in other.into_values() { | ||
| self.insert(selection); | ||
| } | ||
| } | ||
|
|
||
| /// Add selections from another selection map to this one. If there are key collisions, the | ||
| /// selections are *overwritten*. | ||
| pub(crate) fn extend_ref(&mut self, other: &SelectionMap) { | ||
| for selection in other.values() { | ||
| self.insert(selection.clone()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Is there a reason to have this over implementing the extend trait directly?
|
I'll update this to use |
goto-bus-stop
force-pushed
the
renee/selection-map-fleeting-key
branch
from
d9e239f to
2613cd1
Compare
✅ Docs Preview ReadyNo new or changed pages found. |
goto-bus-stop
force-pushed
the
renee/selection-map-fleeting-key
branch
from
2613cd1 to
9c9702d
Compare
|
Rebased, tried to rewrite commit history to make a little more sense, will write a new PR description and fix linting before undrafting for review. |
This has the same name as InlineFragment::can_rebase_on, which will be a problem when I merge the two structures. It's only used in one place: computing the parent type of the inline fragment's subselection when it is being rebased. I inlined the function and I think it's more readable this way as well.
goto-bus-stop
force-pushed
the
renee/selection-map-fleeting-key
branch
from
e7432d8 to
354beb2
Compare
|
Very nice cleanup! |
This PR changes the implementation of
SelectionMapso it does not have owned key data.I'm opening this already so I can share it but I would like to improve the PR text a bit after our standup 😛 I don't think we'll have time to review this in the next days anyways!
Previously, to use
SelectionMap, each selection type provided akey() -> SelectionKeymethod, returning some owned data, in practice twoArcs. Keys are copied frequently. However, key data must always already exist in the selection itself, so this is quite wasteful.Selection keys now borrow from their Selection. They are
Copy, so there is no overhead1 in creating or dropping them. They are meant to be created whenever they're necessary, not stored.Because keys are now "free" to create, there is no need to cache them in
Field,FragmentSpread, andInlineFragmentstructures. As a result, there is now also no need for theField/FieldDatasplit. TheField/FieldDatadata structures existed to disallow arbitrary mutable access toFieldDatathat may affect the cached key in theFieldwrapper. This limitation is lifted, as keys are now only used bySelectionMap: you can freely mutate aFieldor aFieldSelection, as long as it is not in aSelectionMap.SelectionMaponly hands out mutable access throughFieldSelectionValue, which enforces the mutability constraints.I found this to improve performance across the board by 1-3%. That's not super impressive, but not bad either for something that affects every plan. I think this is also a stepping stone towards having mutable selections/selection sets so we won't need to copy so much.
Review guidance
There's a lot of code changes here. I tried to group the commit history into grokkable chunks. Note I didn't keep it compiling between commits necessarily 🙈
SelectionKeyshashbrown'sHashTableand a Vec instead of IndexMap. The implementation is similar to IndexMap, except that keys are derived as necessary.SelectionKeyto be borrowediter()anditer_mut()iterators from SelectionMap: de5ca95(SelectionKey<'a>, SelectionValue<'a>), i.e. an immutable and a mutable reference to the same selection. The methods are unnecessary as you can trivially derive the key from the value.OwnedSelectionKeytype that's used in selection set merging, the way that's written requires an owned key structure, that can be adjusted in the future but I decided to keep it like this also because we recently had to revert a change to this codeInlineFragment::can_rebase_onandInlineFragmentData::can_rebase_onso the names won't collide after the merge 68f3eceFieldandFieldDatainto a single typeField: 4e43243InlineFragment: c2da90cFragmentSpread: 1cf8157Footnotes
I mean, it's still some memory being moved around, but it's not very substantial. ↩