Pool goroutines

[edsrzf]

This is a draft proposal/approach for pooling goroutines. It's not ready to merge, but available for early feedback on the general approach before I go through the effort of trying to add tests, docs, and add further configurability.

There are two problems I'd like to solve:

• Limiting concurrency available for the execution of a single operation.
• Allow a better mechanism for to trigger dataloaders that doesn't rely on timeouts (somewhat related to Optimized batch loading without blocking #518, definitely related to Architecture using timeouts to call fetch? vektah/dataloaden#14). Not solved by pooling goroutines, but I have thoughts around how we might build on top of the pool. If we get to the point that we're happy with this, I'll draft up a proposal issue.

There may also be performance benefits to pooling goroutines, but I haven't measured yet. Judging from #465 it looks like there's potential.

The solution is relatively simple. It adds a new Pool type which can be configured with a goroutine limit. The limit defaults to 0, in which case there is no limit, but goroutines are still pooled and reused. RequestContext now has a Pool field that's used for kicking off goroutines, so goroutines are pooled per-request.

I have:

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

[coveralls]

Coverage increased (+0.2%) to 62.034% when pulling 85a2c4d on edsrzf:goroutine-pool into 7e643fd on 99designs:master.

[vektah]

I had a similar experiment a while ago to benchmark the difference, one pool for everything though. Most of the performance savings were from reusing stacks, and avoiding calls to morestack (it wasn't a large performance saving)

I think go already does some pooling of goroutines, but they don't hold onto the grown stacks. I get the feeling as this is a separate pool for each request it probably won't have much of a performance impact on but it's worth measuring.

Limiting concurrency can be done very cheaply using atomic. I guess this hinges on what you want to do with dataloading, which probably needs to be moved into gqlgen.

I've wondered for a long time if dataloaders are actually doing too much, caching can happen more naturally in the graph based on Id+type, perhaps collecting batches becomes much simpler addressing it on it's own, with some interface changes gqlgen probably doesn't need goroutines per item.

[edsrzf]

While limiting concurrency would be kind of nice, I'm mostly after a solution to the dataloader issues.

I'm curious whether you've put much thought into this:

with some interface changes gqlgen probably doesn't need goroutines per item

I'm keen to find a solution that most people are happy with and willing to do some work toward it.

[vektah]

I'm curious whether you've put much thought into this:

with some interface changes gqlgen probably doesn't need goroutines per item

A little, not enough. My rough thought process goes like this:

• we could simply make resolvers work on slices, but that interface would be terrible most of the time where single loading is fine.
• could use an @batch annotation to make some resolvers as a batch, but still means repeating the "fetch N users by n ids" call all the places that could return users
• for the relay node and apollo federation interfaces what we really want is a way to "fetch N entities by N ids + types"

I'm not really sure how the relationship between the graph and the fetching interface works though.
eg given the ability to lookup users

type ChatMessage {
   author: User
}

What should chat message implement? can we use directives?

• could be thunks?
• could be returning a ref to id + type. is there some crossover with federated external references?
• could be goroutines + pooling, but getting smarter about where concurrency actually happens and remove most of them

Definitely more questions than answers.

[bugzpodder]

we could simply make resolvers work on slices, but that interface would be terrible most of the time where single loading is fine.

My approach was to just separate out the resolver into two resolveLoader and resolveField steps. resolveLoader loads the data and assign it to the field in the api struct. resolveField just returns whatever that's in the field. It works well now that resolvers in gqlgen returns a slice of pointers.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[edsrzf]

I'm still keen to see some of the underlying issues here addressed, but it seems like this isn't the solution.