-
Notifications
You must be signed in to change notification settings - Fork 46
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Validation is extremely slow for large queries with fragment spreads due to lack of implementers_map
caching
#862
Comments
That's a not great oversight. thanks for the report! One approach we could consider here is to add a cache to |
I think using the As a more concrete idea for caching on the schema itself so that it's reusable between different queries, we might have something like the below. If we cache something inside struct Schema {
// .. all the current stuff ..
/// Implementers cache: this must only be accessed through Valid<Schema>.
implementers_map: OnceLock<HashMap<Name, Implementers>>,
}
impl Valid<Schema> {
// This could have the API that SchemaWithCache has today, that populates the cache on demand.
fn implementers_of(&self, name: Name) {}
}
// & then the stuff to support invalidating the cache:
struct Valid<T: Invalidate>(T);
impl<T> Valid<T> {
fn into_inner(self) -> T {
self.0.invalidate()
}
}
trait Invalidate {
// default implementation for types that can be used with Valid<> but do not require invalidation
fn invalidate(self) -> Self { self }
}
impl Invalidate for Schema {
fn invalidate(mut self) -> Self {
self.implementers_map.take();
self
}
}
// this can use the default implementation.
impl Invalidate for ExecutableDocument {} |
@goto-bus-stop I was thinking about this over the weekend, another option is to not cache it but instead have it as a living thing in the schema, that gets built along the schema. Of course it's a bit more complex, again because of mutable schemas and updating this mapping as it is built / modified. But it's a very common structure to access in GraphQL schemas in general, so it's tempting to just make it a "first class" kind of thing. Thoughts? |
I'm not sure if that would work with the current design, which encourages directly mutating the schema structs. Wouldn't users have to manually update the implementers map, or apollo-compiler to provide specific methods to modify the schema? |
Gah good point, I thought this was happening through |
I'll work on this |
Thank you @goto-bus-stop, sounds great 🙇 |
Fixes #862. I had proposed a different approach that would let us cache the implementers map for as long as the schema is immutable, so it could be reused for different query validations. That approach had an issue that made `Valid::assume_valid_ref` very, very subtle to use, so we are not doing that right now. This is a less ideal version but it does solve the immediate problem. We already pass around an `OperationValidationConfig` structure and this seems like a nice, non-invasive place to put the implementers cache.
I've been investigating slow parsing time in our application. It appears to be due to
validate_fragment_spread_type
recomputing a large map of implementers everytime it is called. For large schemas, this creates a very large hashmap everytime. Combined with larger queries this can have a very large effect and we're seeing parse times in the seconds.Instead, the
implementers_map
can be computed once either through aSchemaWithCache
structure, which is used elsewhere already, or by passing along a reference to the pre-computed map during validation. It would be great if this map could be computed only once per schema rather than once per query.I'm happy to contribute a fix if you are opened to it!
The text was updated successfully, but these errors were encountered: