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

parallelize resolvers #106

Closed
bigblind opened this issue Jan 20, 2016 · 10 comments · Fixed by #394
Closed

parallelize resolvers #106

bigblind opened this issue Jan 20, 2016 · 10 comments · Fixed by #394

Comments

@bigblind
Copy link

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 and executeFields do the work serially. I'm willing to provide a PR for this.

@solher
Copy link

solher commented Feb 7, 2016

It seems both methods are the exact same thing.
Maybe that would explain why I get huge perf issues ?
Could somebody confirm/infirm ?

@sogko
Copy link
Member

sogko commented Feb 11, 2016

@bigblind You are right, the executor in graphql-go currently does not take full advantage of being able to resolve fields concurrently. The library is still at its infancy and and was more concerned about getting it right, conforming to specs, over optimising for performance at first. PR is definitely welcomed 👍🏻

@solher Would like to share the issues that you are facing? (May suggest opening a separate issue?)

@bbuck
Copy link
Collaborator

bbuck commented Feb 11, 2016

@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.

@sogko
Copy link
Member

sogko commented Feb 11, 2016

@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:
http://facebook.github.io/graphql/#sec-Serial-execution)

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:

func TestMutations_ExecutionOrdering_EvaluatesMutationsSerially(t *testing.T) {

Cheers!

@solher
Copy link

solher commented Feb 11, 2016

@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 ?

@solher
Copy link

solher commented Feb 23, 2016

Any news ? 👍

@sogko
Copy link
Member

sogko commented May 10, 2016

Quick update:

I've created an experimental branch for resolving fields concurrently:
https://github.com/sogko/graphql/tree/sogko/experiment-parallel-resolve

It is currently based on the v0.4.18 branch (pending merge, being reviewed).

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

@sogko
Copy link
Member

sogko commented Jun 1, 2016

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!

@switchtrue
Copy link
Contributor

@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 ehrId field.

POST
{
  patients {
    id,
    firstname,
    surname,
    address,
    phone,
    dob,
    email,
    gender,
    allergies {
      name,
      date
    },
    ehrId,
    surgical
  }
}

Go - GraphQL-0.4.8

mike@Michaels-MacBook-Pro-2 ~/g/wrk> ./wrk -t12 -c400 -d150s -s gql_t2.lua --timeout 30s "http://10.100.13.235:3001/graphql"
Running 2m test @ http://10.100.13.235:3001/graphql
  12 threads and 400 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency    15.96s     1.35s   21.54s    67.48%
    Req/Sec     5.63      6.94    60.00     91.25%
  2165 requests in 2.50m, 14.35MB read
  Socket errors: connect 155, read 0, write 0, timeout 0
Requests/sec:     14.43
Transfer/sec:     97.96KB

Go - GraphQL-0.5.0

mike@Michaels-MacBook-Pro-2 ~/g/wrk> ./wrk -t12 -c400 -d150s -s gql_t2.lua --timeout 30s "http://10.100.13.235:3001/graphql"
Running 2m test @ http://10.100.13.235:3001/graphql
  12 threads and 400 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency    14.43s     2.55s   23.73s    62.76%
    Req/Sec     3.57      4.45    39.00     81.05%
  1960 requests in 2.50m, 12.99MB read
  Socket errors: connect 155, read 183, write 0, timeout 0
Requests/sec:     13.06
Transfer/sec:     88.69KB

Go - GraphQL-0.5.0 (with Parallel Resolve)

mike@Michaels-MacBook-Pro-2 ~/g/wrk> ./wrk -t12 -c400 -d150s -s gql_t2.lua --timeout 30s "http://10.100.13.235:3001/graphql"
Running 2m test @ http://10.100.13.235:3001/graphql
  12 threads and 400 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency    16.02s     1.97s   24.11s    67.78%
    Req/Sec     3.91      4.91    49.00     78.26%
  2151 requests in 2.50m, 14.26MB read
  Socket errors: connect 155, read 0, write 0, timeout 0
Requests/sec:     14.34
Transfer/sec:     97.35KB

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)

@chris-ramon
Copy link
Member

chris-ramon commented Sep 22, 2018

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! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants