Gqlgen: Optimized batch loading without blocking

[atombender]

I'm trying to understand how gqlgen can optimize batch loading lazily/concurrently, given that all resolvers are synchronous — i.e. they return the value itself, instead of a thunk (future).

I was reading through the code example for data loading to try to understand how dataloaden works. In the its code, I noticed that Load(key) simply calls LoadThunk(key)(). Then I noticed this line in the readme: "This method will block for a short amount of time, waiting for any other similar requests to come in". Is this how gqlgen assumes it will work?

If you look at the graphql-go project, all resolvers can return either values or thunks. This means the entire GraphQL tree can be resolved to thunks, which can then queue up all the keys they need so that when the thunks are called, all the objects can be fetched at once.

Can someone fill me in on how gqlgen is supposed to handle this? A resolver like this:

func (r *rootQueryResolver) Things(ctx context.Context, uids []*string, limit *int) ([]*models.Thing, error) {
    // ...
}

...cannot defer any work at all; it has to return. The only way to batch this is to execute all the resolvers concurrently in goroutines, then have some kind of queue that keeps track of when they've all resolved their keys — or just wait for an arbitrary interval. That seems wrong to me, and all the attendant synchronization sounds like it would be super inefficient.

[vektah]

The only way to batch this is to execute all the resolvers concurrently in goroutines [... and] just wait for an arbitrary interval.

This is what gqlgen does, we eliminate a bunch of places where creating goroutines is unnessicary (binding to fields directly, or methods that dont take a context) and goroutines are cheap enough - benchmarks. There are still a few optimization targets here.

There are optimizations we can make on both sides:

• a. pooling goroutines to avoid morestack (see https://github.com/99designs/gqlgen/pull/465)
• b. notifying dataloaden when all goroutines have been dispatched and the scheduler has had a chance to run. This is enough to avoid the sleep.

Overarching goal for gqlgen is for clean, readable, safe code. I dont think trading clean stacktraces and return values for extra milliseconds (way less if b is actioned)

[atombender]

Thanks. Since you're the one who's done benchmarks, I'm not going to argue until I could possibly offer contrary evidence, but I followed that benchmark link, and it's not actually doing any concurrent batch loading. It's just measuring a single resolver that returns a static value.

I did some simulated benchmarking here. If a GraphQL returns 1000 objects, each with 3 joined objects, the gqlgen architecture will spawn 1000 * 3 goroutines, as I understand it. With a test that iterates over 3000 slice elements in goroutines (using waitgroups and mutexes), I'm getting roughly a 100x performance slowdown with goroutine. It's presumably not the goroutines, but the locking. Since a data loader will need locking for its cache, it might come down to the same numbers, of course. I'm not clear how much locking is needed inside the gqlgen resolver code itself.

But I'm also skeptical that you can achieve sane batch loading through arbitrary deadlines. That sounds like asking for unpredictable performance; as I understand the architecture, one little GC blip and your query could explode into N individual queries because the keys haven't been registered yet.

I'm not sure that thunks affect "cleanness", safety or stack traces? If anything, thunks seem simpler to me than spawning and waiting for goroutines.

[vektah]

I'm not sure that thunks affect "cleanness", safety or stack traces? If anything, thunks seem simpler to me than spawning and waiting for goroutines.

By safe I mostly mean having a single correct resolver signature that can be put into the interface. Go's type system isnt powerful enough to allow multiple different return types and maintain type safety.

When a thunk panics its stack is from the thing dispatching thunks, when a goroutine panics the stack is what you expect, each resolver that lead to this error.

I'm reluctant dive into any work here without looking more broadly at what should be included in gqlgen, id caching and query panning are both interesting, how much of the dataloader should be moved in.

That said if you are hitting performance issues and have some benchmarks to share, and have the time to come up with some clean solutions work in this area is appreciated. Do keep in mind there is a very large refactor coming in the next branch. Dont start anything on master.

[atombender]

By safe I mostly mean having a single correct resolver signature that can be put into the interface.

The simplest solution (which at this point would of course break existing users) would be to always return a function, which is not particularly annoying for non-thunk loaders:

func (r *orderResolver) Items(ctx context.Context, obj *Order) (func() ([]Item, error), error) {
    return func() ([]Item, error) {
        // ...
    }, nil
}

When a thunk panics its stack is from the thing dispatching thunks, when a goroutine panics the stack is what you expect, each resolver that lead to this error.

Yet both will have a stack trace pointing within gqlgen's resolving hierarchy. I honestly don't see the difference, but maybe I'm not seeing something.

That said if you are hitting performance issues and have some benchmarks to share

Well, I'm merely evaluating gqlgen at this point. I've started writing app that uses graphql-go/graphql, but I like the "schema first" approach and generating code, since it means both the GraphQL code and the Go types can be generated from the same time. I'm not overly concerned with performance for this app, but I don't like the added complexity. Using goroutines instead of thunks adds a lot of synchronization at the data loading level. I've already written my batch loading using the graph-gophers/dataloader library, which I'm quite happy with. It's simple and easy to understand.

Having looked at a few of these GraphQL libraries, I'm of the opinion that a server-side GraphQL implementation like gqlgen should absolutely have batch loading built in, because it is in the nature of GraphQL to express many fetches in a single query. An app that doesn't need batch loading can choose not to batch-load, but those will in the minority.

[vektah]

related #520

[leonth]

Hi, I'm evaluating gqlgen as well for our internal use.

notifying dataloaden when all goroutines have been dispatched and the scheduler has had a chance to run. This is enough to avoid the sleep.

I agree and this is what other implementations that do not use promises e.g. PHP are doing.

By safe I mostly mean having a single correct resolver signature that can be put into the interface. Go's type system isnt powerful enough to allow multiple different return types and maintain type safety.

Since gqlgen itself already requires code generation step, if the dataloading mechanism (dataloaden) can also be generated at the same time (possibly in the same configuration file etc) then this should be less of an issue.

Having looked at a few of these GraphQL libraries, I'm of the opinion that a server-side GraphQL implementation like gqlgen should absolutely have batch loading built in, because it is in the nature of GraphQL to express many fetches in a single query.

+1. This is also what the PHP implementation above does, as well as other solid implementations like Sangria do.

[bugzpodder]

@atombender @leonth @steebchen @a-h
I have prototyped something that worked for me, with less reliance on blocking. https://github.com/99designs/gqlgen/issues/790 If you have any feedback please let me know.

[atombender]

Thanks, @bugzpodder. Unfortunately, I decided to not use gqlgen because of this issue; I found graphql-go's resolver design to be cleaner and I'm using that. So I'm not in a position to try out your PR anymore.

[stale[bot]]

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.

[frederikhors]

no stale

[stale[bot]]

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.

[atombender]

Not stale.

[stale[bot]]

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.

[frederikhors]

Please can we disable Stale?

[macnibblet]

@vektah Any ideas on how to handle this? It's becoming a growing concern for us.

[happenslol]

I'd be interested in tackling this issue if there is a way to implement this that we can agree on. It seems the main problem here is that it's not possible to delay fetching of fields when the resolver is called, and I can see 2 possible solutions for this:

1. Add a directive (like @batch) which will generate code that lets you return ids or hand you a function with a batcher as an argument, or
2. Add configuration options in gqlgen.yml which will change the return type of the function to something of your choosing, and then require you to implement a resolver which gets passed the collected returns of that "tick".

I would personally prefer 2., since it keeps implementation specifics out of the schema. Please tell me if you would consider one of these solutions, or if you have any thoughts.

[macnibblet]

I think the second solution sounds very clean, the only issue I remember discussing with someone is the stack traces if something goes wrong gets exceedingly messy but as far as I can think at the moment that's the only downside.

[happenslol]

the stack traces if something goes wrong gets exceedingly messy

How so? With the proposed solution, fetching a field that is marked as batched would simply be divided into two phases - fetching the IDs, and fetching the data using the IDs. If one of those steps goes wrong, you would get the exact same stack traces as if you had a single resolver.
Just to clarify what this would look like from the library users' perspective:

1. Mark a field as batched in gqlgen.yml

models:
  Foo:
    model: ...
    batch:
        # Defining both model and type allows
        # us to define custom ID types.
        - field: field1
          model: string
        - field: field2
          model: github.com/99designs/gqlgen/graphql.Int64

1. 2 Resolvers are generated for you instead of one:

interface FooResolver {
  // The ID resolver lets us return only the ID type we defined, and defer
  // loading of the actual data.
  Field1ID(ctx context.Context, obj *model.Foo) (string, error)
  Field2ID(ctx context.Context, obj *model.Foo) (graphql.Int64, error)

  // The second part is the data resolver, which hands us IDs and expects
  // us to resolve them to actual entities.
  Field1Data(ctx context.Context, ids []string) (map[string]*model.Field1, error)
  Field2Data(ctx context.Context, ids []graphql.Int64), map[graphql.Int64]*model.Field2, error)
}

1. Now, for each entity where a batch resolver is present, all requests for 1 "level" will automatically be collected and only resolved once.

Am I missing something?

[macnibblet]

No that does make sense