-
Notifications
You must be signed in to change notification settings - Fork 843
parallelize resolvers #106
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
Comments
It seems both methods are the exact same thing. |
@bigblind You are right, the executor in @solher Would like to share the issues that you are facing? (May suggest opening a separate issue?) |
@sogko Is it not in the specs how parallelization should work with queries? It's important that this be implemented properly. From what I understand is that query fields should be parallelized; however, mutation fields should not. |
@bbuck The spec does not really specify if you should / should not execute a request concurrently / parallelise it. It just dictates that the order of evaluation. (Relevant section: So you can, in effect, choose to implement the executor to run a query and even a mutation concurrently, as long as you can guarantee that the evaluation order for a mutation is done serially as the user has specified. For non-mutation (i.e. normal query), the order of evaluation can be non-deterministic (which allows for concurrent evaluation easily). (http://facebook.github.io/graphql/#sec-Normal-evaluation) So, right now, the library does not evaluate fields concurrently for either query and mutation, but it conforms to the spec re: evaluation order. Relevant test for serial evaluation for mutation: Line 122 in 344abcc
Cheers! |
@sogko Nothing really complicated here. When I find a list of 1000 nodes and their associated nodes, 1000 DB queries of 1ms are executed non concurrently so I get the result in 1 sec where I get it in 10 ms with the same rest app haha. However, I'm still surprised that a simple "Hello world" on a root query takes an average of 5ms when a simple rest return would do it in 100 microsec. I obviously get that graphql adds a ton of logic and that it's perfectly normal to have an overhead but I would not expect it to be more than 10 times slower. Is that only the amount of reflection that must be used that slows that much everything down ? |
Any news ? 👍 |
Quick update: I've created an experimental branch for resolving fields concurrently: It is currently based on the It satisfies current set of unit tests (which is great), but still need to spend some time to discover edge cases that I might have missed. Cheers! /cc @chris-ramon |
Requesting reviews/comments for PR #132 which implements a concurrent field resolver. (It is based off the 0.5.0 PR, so there will be a lot of unrelated commits. I've isolated the relevant commits on the description). Any help is appreciated! Cheers! |
@sogko - This is in relation to this and #123 I haven't done anything in the way of a code review but I've upgraded a system we've got to 0.4.8 and then to your sogko/experiment-parallel-resolve branch. There were no code changes required and everything worked great! We are currently in the process of testing three identical GraphQL backends - Go, Java and Node - to determine what we want to use going forwards. As part of this we are running load tests. Our app spends a lot of time communicating with a third party server over HTTP (offsite). We make an initial call to it to get a list of patients and then for each patient we need to do an additional lookup via the HTTP call to resolve a particular ID field. Given that there is a resolve fired for each patient in a list to get this ID which in turn makes an HTTP request I was expecting to see big wins with the use of parallel resolvers. However, I am not seeing much difference in regards to performance here. The project I am using is open source and can be found here. The query I am using is below - this returns a list of patients and each patient has a resolve function that makes an HTTP call (click to see the code) to get the POST
{
patients {
id,
firstname,
surname,
address,
phone,
dob,
email,
gender,
allergies {
name,
date
},
ehrId,
surgical
}
} Go - GraphQL-0.4.8
Go - GraphQL-0.5.0
Go - GraphQL-0.5.0 (with Parallel Resolve)
By the way - these times still beat the Java implementation (16.49s with a lot of errors being generated) and the Node implementation (21.20s) |
Just for the record this issue was closed since we support concurrent resolvers, an example was added on how to achieve it: https://github.com/graphql-go/graphql/blob/master/examples/concurrent-resolvers/main.go — thanks a lot for the great feedback! 👍 |
Graphql-js runs resolvers in parallel, and Go makes this easy to do as well, but unless I'm overlooking something in the code, both
executeFieldsSerially
andexecuteFields
do the work serially. I'm willing to provide a PR for this.The text was updated successfully, but these errors were encountered: