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

Better CollectFields performance #680

Merged
merged 4 commits into from
Apr 17, 2019

Conversation

asp24
Copy link
Contributor

@asp24 asp24 commented Apr 17, 2019

CollectFields was increased thanks to following improvements:

  • Pre-allocate mem for method result slice
  • graphql.CollectFields now accept *RequestContext as first arg
  • Early return in shouldIncludeNode if directives empty

Before
BenchmarkSimpleQueryNoArgs-4 100000 20736 ns/op 6628 B/op 120 allocs/op
After
BenchmarkSimpleQueryNoArgs-4 100000 19513 ns/op 6692 B/op 120 allocs/op

In practice, difference is much bigger on complex models

asp24 added 4 commits April 17, 2019 13:10
It was done because RequestContext is a part of executionContext and can
be passed directly without extraction from ctx. This is increasing
performance when model depth is high
@vektah
Copy link
Collaborator

vektah commented Apr 17, 2019

Came into this pretty cynical, but it looks pretty straight forward. Nice!

My last round of perf digging - #465.

I think the next big wins are in eliminating as many goroutines as possible during execution, probably by shipping a dataloader solution integrated with gqlgen that doesnt require goroutines and forcing users to be more explicit around which resolvers need to be concurrent. Going to require a bit of planning.

@vektah vektah merged commit afe33f7 into 99designs:master Apr 17, 2019
@vektah vektah added the v0.9 Fixed in 0.9.0 label May 15, 2019
cgxxv pushed a commit to cgxxv/gqlgen that referenced this pull request Mar 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v0.9 Fixed in 0.9.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants