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

use goroutine about processing each array elements #316

Merged
merged 2 commits into from
Aug 28, 2018

Conversation

vvakame
Copy link
Collaborator

@vvakame vvakame commented Aug 24, 2018

refs #313
#301 makes ResolverContext immutable.
We can use concurrent processing safety.
cc/ @dvic

I have:

  • Added tests covering the bug / feature (see testing)
  • Updated any relevant documentation (see docs)

performance regression test is so hard to writing... 🤔

{{.arr}} = append({{.arr}}, func() graphql.Marshaler {
{{ .next }}
}())
go func({{.index}} int) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should probably avoid creating a new gofunc if:

  • there is only one element in the array
  • the child field doesn't have a resolver (accessing properties is cheap and should happen sequentially)

@@ -212,28 +212,37 @@ func (f *Field) doWriteJson(val string, remainingMods []string, astType *ast.Typ
}
var arr = "arr" + strconv.Itoa(depth)
var index = "idx" + strconv.Itoa(depth)
var wg = "wg" + strconv.Itoa(depth)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this probably doesn't need to be nested, and can be shared by the whole field.

@vektah
Copy link
Collaborator

vektah commented Aug 26, 2018

Don't worry about testing performance. It shouldn't be too hard to test that { foo { bar { baz } } } runs all baz concurrently, without any of them needing to complete.

@vvakame
Copy link
Collaborator Author

vvakame commented Aug 27, 2018

updated!

return {{.arr}}`, map[string]interface{}{
"val": val,
"arr": arr,
"index": index,
"wg": wg,
"top": depth == 1,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably can just go into WriteJson and be prefixed on? Doing it here in the recursive part is a bit hard to reason about.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is little bit difficult.
We should exec wg.Wait() before return statement.
https://github.com/99designs/gqlgen/pull/316/files#diff-3995ea1c8b08158971f7c289f7f7d50aR256

@vektah
Copy link
Collaborator

vektah commented Aug 27, 2018

This really needs some tests :(

@vvakame
Copy link
Collaborator Author

vvakame commented Aug 27, 2018

integration/ is good test case. It caught my bug in this PR 😺

@vektah vektah merged commit 988b367 into master Aug 28, 2018
@vektah vektah deleted the feat-concurrent-each-element branch August 28, 2018 00:34
cgxxv pushed a commit to cgxxv/gqlgen that referenced this pull request Mar 25, 2022
…element

use goroutine about processing each array elements
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.

None yet

2 participants