Evaluate list fields in parallel #20
Merged
Conversation
This file contains hidden or 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
|
Hi @andreas and thanks for doing this. I've run some tests against both #20 and #21. #21 does not seem to resolving fields in parallel. I've put some logging in my schema in the I've run my load tests again and I'm not seeing any change with the #20 version. However, with individual requests I am definitely seeing a noticeable improvement (stats below). I'm guessing with so many HTTP requests under our load tests I'm just hitting a bottleneck here or on the server at the oither end and no amount of tuning of the GraphQL library is going to solve that. Average Request TimesWhen not under load significant improvement were observed. For 10 repeated tests the average times are: #20: sogko/experiment-parallel-resolve#20#21 |
|
Thanks for confirming @Mleonard87! I think you're right wrt. the latency under load test vs single request. PR #21 is just a refactoring of the existing code on the |
|
Hi @andreas @Mleonard87! Definitely missed out on list evaluation previously, great job catching that 👍 I think I can merge both PR #20 and #21, both are improvements over the original implementation. #21 makes it more consistent with how fields are resolved concurrently and how panics are recovered. Cheers! Notes: Tests: Currently, we don't have benchmark tests as part of the suite; since we have been focusing on correctness and conformance to spec, less on perf. That can be added on a separate PR on this repo before merging to the main repo. (Anybody want to take this up? Probably adapting version of @Mleonard87 tests. (Do you mine sharing your Data race: The /cc @chris-ramon |
This was referenced Jun 11, 2016
sogko
added a commit
that referenced
this pull request
Jun 11, 2016
… from here for perf-related PRs. - simple hello world equivalent - with a list - deeper query with list - query with many root fields and list in each of them - deeper query with many root fields and lists To really bring out probable issues with query perf, a latency is introduced to the data retrieval (previously it was just in-memory data fetch) When testing against branch before PR #20 and #21, this really highlighted the problem with evaluating list fields. In the future, more benchmarks for queries with fragments probably would be useful.
|
Is anyone currently working on this? If not, are you using some other library instead? It seems pretty crucial to parallelize this in order to avoid N+1 queries. |
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.
After looking into the issue reported by @Mleonard87 in graphql-go#106, I noticed that list fields were not being evaluated in parallel. This PR changes the behaviour of
completeListValueto resolve the elements of the list in parallel. A goroutine is spawned per indexjinresultValand assigns the result intocompletedResults[j]. Since the goroutine can panic, a separate channelpanicsis used to communicate those, and repanic in the original goroutine.No tests included, but the example described by @Mleonard87 runs much faster (would be great if you can confirm, @Mleonard87).