forked from wundergraph/graphql-go-tools
-
Notifications
You must be signed in to change notification settings - Fork 4
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
[TT-9864] Optimize the creation usage of AST documents #431
Merged
buraksezer
merged 6 commits into
master
from
feat/TT-9864/Optimize-the-creation-usage-of-AST-documents
Jun 14, 2024
Merged
[TT-9864] Optimize the creation usage of AST documents #431
buraksezer
merged 6 commits into
master
from
feat/TT-9864/Optimize-the-creation-usage-of-AST-documents
Jun 14, 2024
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
Quality Gate passedIssues Measures |
Latest benchmark results can be found here: # Pooling ast.Document instances
graphql git:(feat/TT-9864/Optimize-the-creation-usage-of-AST-documents) ✗ go test -bench=BenchmarkExecutionEngineV2_Execute_AstDocumentPool -run='^$' -v -memprofile memprofile.out -count=10 .
Alloc = 1 MiB TotalAlloc = 1 MiB Sys = 9 MiB NumGC = 0
goos: darwin
goarch: arm64
pkg: github.com/TykTechnologies/graphql-go-tools/pkg/graphql
BenchmarkExecutionEngineV2_Execute_AstDocumentPool
BenchmarkExecutionEngineV2_Execute_AstDocumentPool-8 79106 13289 ns/op 38904 B/op 118 allocs/op
BenchmarkExecutionEngineV2_Execute_AstDocumentPool-8 91957 13402 ns/op 38852 B/op 118 allocs/op
BenchmarkExecutionEngineV2_Execute_AstDocumentPool-8 93105 12736 ns/op 38819 B/op 118 allocs/op
BenchmarkExecutionEngineV2_Execute_AstDocumentPool-8 93937 12798 ns/op 38781 B/op 118 allocs/op
BenchmarkExecutionEngineV2_Execute_AstDocumentPool-8 87145 13045 ns/op 38806 B/op 118 allocs/op
BenchmarkExecutionEngineV2_Execute_AstDocumentPool-8 91832 12914 ns/op 38773 B/op 118 allocs/op
BenchmarkExecutionEngineV2_Execute_AstDocumentPool-8 94764 12832 ns/op 38797 B/op 118 allocs/op
BenchmarkExecutionEngineV2_Execute_AstDocumentPool-8 84831 13630 ns/op 38942 B/op 118 allocs/op
BenchmarkExecutionEngineV2_Execute_AstDocumentPool-8 95127 12783 ns/op 38821 B/op 118 allocs/op
BenchmarkExecutionEngineV2_Execute_AstDocumentPool-8 94060 13051 ns/op 38846 B/op 118 allocs/op
PASS
Alloc = 2 MiB TotalAlloc = 37317 MiB Sys = 19 MiB NumGC = 18325
ok github.com/TykTechnologies/graphql-go-tools/pkg/graphql 13.616s
# Without pooling
➜ graphql git:(master) ✗ go test -bench=BenchmarkExecutionEngineV2_Execute_AstDocumentPool -run='^$' -v -memprofile memprofile.out -count=10 .
Alloc = 1 MiB TotalAlloc = 1 MiB Sys = 9 MiB NumGC = 0
goos: darwin
goarch: arm64
pkg: github.com/TykTechnologies/graphql-go-tools/pkg/graphql
BenchmarkExecutionEngineV2_Execute_AstDocumentPool
BenchmarkExecutionEngineV2_Execute_AstDocumentPool-8 31033 38686 ns/op 176441 B/op 165 allocs/op
BenchmarkExecutionEngineV2_Execute_AstDocumentPool-8 31431 38183 ns/op 176437 B/op 165 allocs/op
BenchmarkExecutionEngineV2_Execute_AstDocumentPool-8 31693 38475 ns/op 176469 B/op 165 allocs/op
BenchmarkExecutionEngineV2_Execute_AstDocumentPool-8 31638 38381 ns/op 176481 B/op 165 allocs/op
BenchmarkExecutionEngineV2_Execute_AstDocumentPool-8 31202 38643 ns/op 176496 B/op 165 allocs/op
BenchmarkExecutionEngineV2_Execute_AstDocumentPool-8 31650 37853 ns/op 176421 B/op 165 allocs/op
BenchmarkExecutionEngineV2_Execute_AstDocumentPool-8 30669 38295 ns/op 176468 B/op 165 allocs/op
BenchmarkExecutionEngineV2_Execute_AstDocumentPool-8 32060 39800 ns/op 176517 B/op 165 allocs/op
BenchmarkExecutionEngineV2_Execute_AstDocumentPool-8 30771 38303 ns/op 176439 B/op 165 allocs/op
BenchmarkExecutionEngineV2_Execute_AstDocumentPool-8 31297 38701 ns/op 176467 B/op 165 allocs/op
PASS
Alloc = 2 MiB TotalAlloc = 69767 MiB Sys = 51 MiB NumGC = 36602
ok github.com/TykTechnologies/graphql-go-tools/pkg/graphql 16.436s
# benchstat output
➜ ~ benchstat old.txt new.txt
goos: darwin
goarch: arm64
pkg: github.com/TykTechnologies/graphql-go-tools/pkg/graphql
│ old.txt │ new.txt │
│ sec/op │ sec/op vs base │
ExecutionEngineV2_Execute_AstDocumentPool-8 38.43µ ± 1% 12.98µ ± 3% -66.22% (p=0.000 n=10)
│ old.txt │ new.txt │
│ B/op │ B/op vs base │
ExecutionEngineV2_Execute_AstDocumentPool-8 172.33Ki ± 0% 37.91Ki ± 0% -78.00% (p=0.000 n=10)
│ old.txt │ new.txt │
│ allocs/op │ allocs/op vs base │
ExecutionEngineV2_Execute_AstDocumentPool-8 165.0 ± 0% 118.0 ± 0% -28.48% (p=0.000 n=10) |
kofoworola
approved these changes
Jun 14, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seen the benchmark result, nice!
pvormste
approved these changes
Jun 14, 2024
buraksezer
deleted the
feat/TT-9864/Optimize-the-creation-usage-of-AST-documents
branch
June 14, 2024 11:12
buraksezer
added a commit
to TykTechnologies/tyk
that referenced
this pull request
Jun 17, 2024
### **User description** PR for https://tyktech.atlassian.net/browse/TT-9864. See TykTechnologies/graphql-go-tools#431 for details. ___ ### **PR Type** Enhancement, Dependencies ___ ### **Description** - Added cleanup for allocated resources in the `HandleReverseProxy` method in `internal/graphengine/engine_v2.go`. - Updated `graphql-go-tools` dependency version in `go.mod`. - Updated checksum for `graphql-go-tools` dependency in `go.sum`. ___ ### **Changes walkthrough** 📝 <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Enhancement </strong></td><td><table> <tr> <td> <details> <summary><strong>engine_v2.go</strong><dd><code>Add resource cleanup in `HandleReverseProxy` method.</code> </dd></summary> <hr> internal/graphengine/engine_v2.go <li>Added cleanup for allocated resources in <code>HandleReverseProxy</code> method.<br> </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6345/files#diff-b1eaa954c9836f395e1d49090e85c739e3878747c8bd748f556fc5a53ff7b191">+2/-0</a> </td> </tr> </table></td></tr><tr><td><strong>Dependencies </strong></td><td><table> <tr> <td> <details> <summary><strong>go.mod</strong><dd><code>Update `graphql-go-tools` dependency version.</code> </dd></summary> <hr> go.mod - Updated `graphql-go-tools` dependency version. </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6345/files#diff-33ef32bf6c23acb95f5902d7097b7a1d5128ca061167ec0716715b0b9eeaa5f6">+1/-1</a> </td> </tr> <tr> <td> <details> <summary><strong>go.sum</strong><dd><code>Update checksum for `graphql-go-tools` dependency.</code> </dd></summary> <hr> go.sum - Updated checksum for `graphql-go-tools` dependency. </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6345/files#diff-3295df7234525439d778f1b282d146a4f1ff6b415248aaac074e8042d9f42d63">+2/-2</a> </td> </tr> </table></td></tr></tr></tbody></table> ___ > 💡 **PR-Agent usage**: >Comment `/help` on the PR to get a list of all available PR-Agent tools and their descriptions
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.
PR for TT-9864.
After reading a lot of code, trying different solutions and measuring them, I decided to go with the simplest solution.
I used
sync.Pool
for reusingast.Document
instances and added aCleanup
method toRequest
struct to recycleast.Document
structs if it's possible.The library caches the plans for queries but before caching or deciding to use a plan from the cache, it creates a new
ast.Document
, parses the query, normalizes and validates it against the schema. If a plan is cached, resetting and puttingast.Document
to the pool can lead to subtle bugs because the cached plan still uses it for the incoming queries.I added a boolean field to
Request
struct to mark points out the request will be processed by a plan from the cache. If so, we don't need theast.Document
in theRequest
struct anymore and we can recycle it.Cleanup
method does this job. I plan to useCleanup
method from Tyk GW. I tried to run the CI pipeline with a dummy PR and it worked. See this: https://github.com/TykTechnologies/tyk/pull/6345/files#Because
Cleanup
method is only called from Tyk GW, it should not has any side effects on the tests in the library.ast.NewDocument
method has been called from different places in the library but recyclingast.Document
in GraphQL data source and federation code leads to bugs and breaks the tests.