Skip to content

Conversation

andreas
Copy link

@andreas andreas commented Jun 9, 2016

This is another take on doing concurrent evaluation of fields. Not necessarily better, but maybe it can serve as inspiration. The goroutines assign directly into the result map, which (unfortunately) needs to be protected by a mutex, as maps are not thread-safe. A separate channel panics is used for re-raising panics.

@andreas andreas changed the title Simplify concurrent evaluation of fields Alternative concurrent evaluation of fields Jun 9, 2016
@sogko sogko merged commit 14fac79 into sogko:sogko/experiment-parallel-resolve Jun 11, 2016
@sogko
Copy link
Owner

sogko commented Jun 11, 2016

Hey @andreas

Thanks for the PR, appreciate it very much 👍

Regarding the use of mutex for finalResults map, I think it's fine. The implementation reads easier now and consistent with completeListValue.

Cheers!

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.
@Nebulai
Copy link

Nebulai commented Jun 16, 2016

Hey @andreas @sogko,

Sorry for lurking, but any idea when parallel resolves might make it into master?
We're looking to implement GraphQL in the company I work at and I've been experimenting with the Golang version. Some colleagues want to go with the JavaScript implementation however and their main reasoning is that go-graphql doesn't have parallel resolves yet.

I tried compiling the experiment-parallel-resolve branch unsuccessfully:

# graphql-go-parallel
../graphql-go-parallel/executor.go:183: undefined: ast.OperationTypeMutation

../graphql-go-parallel/executor.go:197: undefined: ast.OperationTypeQuery

../graphql-go-parallel/executor.go:199: undefined: ast.OperationTypeMutation

../graphql-go-parallel/executor.go:204: cannot use operation (type ast.Definition) as type ast.Node in array or slice literal:
    ast.Definition does not implement ast.Node (missing GetKind method)

../graphql-go-parallel/executor.go:209: too many arguments in call to gqlerrors.NewError

../graphql-go-parallel/executor.go:212: undefined: ast.OperationTypeSubscription

../graphql-go-parallel/executor.go:217: cannot use operation (type ast.Definition) as type ast.Node in array or slice literal:
    ast.Definition does not implement ast.Node (missing GetKind method)

../graphql-go-parallel/executor.go:222: too many arguments in call to gqlerrors.NewError

../graphql-go-parallel/executor.go:228: cannot use operation (type ast.Definition) as type ast.Node in array or slice literal:
    ast.Definition does not implement ast.Node (missing GetKind method)

../graphql-go-parallel/executor.go:233: too many arguments in call to gqlerrors.NewError

../graphql-go-parallel/executor.go:233: too many errors


Any idea what I'm doing wrong? Give me some pointers and maybe I can help solve it.

@andreas
Copy link
Author

andreas commented Jun 16, 2016

@Nebulai, the following works for me:

git clone https://github.com/sogko/graphql.git
cd graphql
git checkout origin/sogko/experiment-parallel-resolve
go build

How did you get that error?

@Nebulai
Copy link

Nebulai commented Jun 17, 2016

@andreas Got it working now

I had an old version of go-graphql. Pulled the latest one and just had this one error:

./graphql.go:37: cannot use p.RequestString (type string) as type []byte in field value

Changed line 37 to how it looks in the go-graphql master and that did the trick 👍

@andreas andreas deleted the simplify-concurrent-field-evaluation branch November 25, 2017 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants