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

graphql: cache GraphQL validation #3759

Merged
merged 3 commits into from
Jul 26, 2022
Merged

Conversation

kamilkisiela
Copy link
Contributor

@kamilkisiela kamilkisiela commented Jul 21, 2022

I ran the same query ~55 times and got these results:

No caching (58 samples)

p95:      47.75µs
p99:      73.83µs
average:  24.85µs
median:   20.13µs

Caching (56 samples)
    
p95:       4.63µs
p99:       4.92µs
average:   2.08µs
median:    1.67µs

Part of #3077

graphql/src/execution/query.rs Outdated Show resolved Hide resolved
@kamilkisiela kamilkisiela force-pushed the kamil-graphql-validation-perf branch from 9abe431 to 7da9eb1 Compare July 21, 2022 13:51
@kamilkisiela
Copy link
Contributor Author

Applied suggested changes, ran cargo fmt and squashed commits

@leoyvens
Copy link
Collaborator

Still doesn't build.

@kamilkisiela kamilkisiela force-pushed the kamil-graphql-validation-perf branch from bf5b62d to 1ac64f3 Compare July 22, 2022 12:14
@kamilkisiela kamilkisiela force-pushed the kamil-graphql-validation-perf branch from 1ac64f3 to 31a6405 Compare July 22, 2022 12:16
@leoyvens
Copy link
Collaborator

Lock lifetimes are tricky. I've built on your changes so that the lock is only held when reading or inserting to the cache, but importantly not held when actually validating the query. As you had noticed this requires a bit of cloning.

@leoyvens
Copy link
Collaborator

@kamilkisiela could you review and re-run the benchmarks?

@kamilkisiela
Copy link
Contributor Author

@leoyvens thank you. I re-run the benchmarks and it looks faster now.

@leoyvens leoyvens merged commit b77bf9d into master Jul 26, 2022
@leoyvens leoyvens deleted the kamil-graphql-validation-perf branch July 26, 2022 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants