Skip to content
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

fix(compiler): cache the implementers map per executable document #863

Merged
merged 3 commits into from
Jun 5, 2024

Conversation

goto-bus-stop
Copy link
Member

@goto-bus-stop goto-bus-stop commented Jun 5, 2024

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 had an OperationValidationConfig structure that contains contextual information about the current operation. This extends that idea to have an ExecutableValidationContext for the whole document, and "child" OperationValidationContexts for each operation.

The included benchmark goes from 200ms of validation time to 2ms.

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.
@goto-bus-stop goto-bus-stop added the apollo-compiler issues/PRs pertaining to semantic analysis & validation label Jun 5, 2024
@goto-bus-stop goto-bus-stop self-assigned this Jun 5, 2024
@goto-bus-stop goto-bus-stop changed the title fix(compiler): cache the implementers map per operation fix(compiler): cache the implementers map per executable document Jun 5, 2024
@goto-bus-stop goto-bus-stop merged commit 09eb68c into main Jun 5, 2024
12 checks passed
@goto-bus-stop goto-bus-stop deleted the renee/implementers-per-validation branch June 5, 2024 10:20
@goto-bus-stop goto-bus-stop mentioned this pull request Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apollo-compiler issues/PRs pertaining to semantic analysis & validation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Validation is extremely slow for large queries with fragment spreads due to lack of implementers_map caching
2 participants