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

Experimental/Need feedback: Implement pluggable batching/dedup/concurrent fetch like facebook/dataloader #154

Closed
wants to merge 6 commits into from

Conversation

bigdrum
Copy link
Contributor

@bigdrum bigdrum commented Jul 24, 2016

I have been considering how to implement the idea of facebook/dataloader with graphql in go (which address issues like #106, #132), and this PR shows the progress so far.

This PR is mainly a demonstration on how it could work. And so far I'm happy with the idea.

This requires some minimum changes to the graphql package (just to inject a executor with a simple interface). The real concurrency feature is implemented in the godataloader library that I wrote separately, which can be plugged into graphql.

examples/dataloader contains an actual example, which demonstrate the following features:

  • Opt-in.
  • Concurrently loading multiple data.
  • Load duplicate requests once.
  • Batching multiple requests.
  • No unnecessary goroutines are spawn unless concurrency actually happen.
  • Synchronous programming style (without Promise-like object).

The test might best demonstrate the behavior.

func TestQuery(t *testing.T) {
    schema := dataloaderexample.CreateSchema()
    r := dataloaderexample.RunQuery(`{
        p1_0: post(id: "1") { id author { name }}
        p1_1: post(id: "1") { id author { name }}
        p1_2: post(id: "1") { id author { name }}
        p1_3: post(id: "1") { id author { name }}
        p1_4: post(id: "1") { id author { name }}
        p1_5: post(id: "1") { id author { name }}
        p2_1: post(id: "2") { id author { name }}
        p2_2: post(id: "2") { id author { name }}
        p2_3: post(id: "2") { id author { name }}
        p3_1: post(id: "3") { id author { name }}
        p3_2: post(id: "3") { id author { name }}
        p3_3: post(id: "3") { id author { name }}
        u1_1: user(id: "1") { name }
        u1_2: user(id: "1") { name }
        u1_3: user(id: "1") { name }
        u2_1: user(id: "3") { name }
        u2_2: user(id: "3") { name }
        u2_3: user(id: "3") { name }
    }`, schema)
    if len(r.Errors) != 0 {
        t.Error(r.Errors)
    }
    t.Error(r)
    // The above query would produce log like this:
    // 2016/07/23 23:49:31 Load post 3
    // 2016/07/23 23:49:31 Load post 1
    // 2016/07/23 23:49:31 Load post 2
    // 2016/07/23 23:49:32 Batch load users [3 1 2]
    // Notice the first level post loading is done concurrently without duplicate.
    // The user loading is also done in the same fashion, but batched fetch is used instead.
    // TODO: Make test actually verify the logged behavior.
}

This PR is not meant to be merged yet (at least the example probably doesn't fit to be part of this project because of its external dependency, unless the godataloader library is moved).

The godataloader library is probably a little bit more complicated than one would expect. And I'm happy to explain it if anyone finds this idea interesting.

…ems. And an example to demonstrate how it can be used.
@bsr203
Copy link

bsr203 commented Jul 24, 2016

It may be a while till I expiriment with it, but like how you done it all with minimal changes and also github.com/bigdrum/godataloader is kinda small. thanks for improving this library. cheers.

@coveralls
Copy link

coveralls commented Jul 24, 2016

Coverage Status

Coverage increased (+0.05%) to 90.807% when pulling b7944de on bigdrum:executor into 491504a on graphql-go:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 90.807% when pulling fd2a5d6 on bigdrum:executor into 491504a on graphql-go:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 90.797% when pulling d514566 on bigdrum:executor into 491504a on graphql-go:master.

@coveralls
Copy link

coveralls commented Jul 24, 2016

Coverage Status

Coverage increased (+0.04%) to 90.797% when pulling 61de32e on bigdrum:executor into 491504a on graphql-go:master.

@teloon
Copy link

teloon commented Sep 15, 2016

This look really interesting. Anything we're missing here?

@bigdrum
Copy link
Contributor Author

bigdrum commented Oct 20, 2016

FYI, I've been using this at a small scale production environment. And I haven't seen any bugs so far. The only problem that I still consider this as experiment is that for a object with only trivial resolve function (one that without expensive IO), the dataloader would still kick off a goroutine (one per parent object, not one per item, though, so not that many). We don't see any performance issue so far but it would be nice to provide some hint to optimize that away.

@ajackson-cpi
Copy link
Contributor

Any status on this? I'm interested in parallel processing for my use-cases. GoRoutines are quite light if they're short-lived, so I wouldn't spend too much time on them.

@tonyghita
Copy link

Have you looked at https://github.com/nicksrandall/dataloader? This library should probably be orthogonal to any dataloader

@bigdrum
Copy link
Contributor Author

bigdrum commented Mar 4, 2017

https://github.com/nicksrandall/dataloader only dispatches the batch when batch size reaches some limit or some pre-configured amount of wait time has passed. This will introduce unnecessary delay. On the other hand, facebook's dataloader.js and my implementation do not have such issue.

@tonyghita
Copy link

@bigdrum this is because facebook's implementation takes advantage of the javascript event loop. Golang (to my understanding) doesn't have such an event loop, so @nicksrandall's implementation approximates this with the batch size/time elapsed mechanism (both parameters are configurable to your use case).

@bigdrum
Copy link
Contributor Author

bigdrum commented Mar 7, 2017 via email

@bigdrum
Copy link
Contributor Author

bigdrum commented Mar 7, 2017

To be more elaborated, in essence, there are two kinds of tasks, the tasks that collect what need to be loaded, and the tasks of the fetching. If we can prioritize the collection task over the fetching task, we can batch as much as possible.

FB's dataloader.js takes advantage of js env where a closure can be schedule at the end of the event loop, which essentially allow them to schedule the lower priority fetch tasks after the collection task. So event loop is not essence here, the essence is the ability to dispatch tasks in a certain order. The event loop and its API just allows dataloader.js to implement the custom scheduling logic, in a way that hides perfectly from the end user.

Go doesn't support custom go routine scheduling, if it does, we could create a schedule domain, dispatch go routine with different strict priority. So I ended up implement a custom scheduling mechanism that allows running different closure with a custom order, which has two queues internally, to allow executing the tasks with two tier priority. Thus to achieve the same result. But I could not figure out a perfectly non-intrusive way as dataloader.js can do in node.js.

@nicksrandall's implementation achieve this by manually delay the fetch task with a fixed amount of time, to allow the collection tasks to be executed before that. But since it doesn't know when the collection is done, it pays the cost of waiting blindly.

It is a trade-off, @nicksrandall is definitely much easier to understand, and my implementation is harder, but to me, the precise scheduling without compromising the latency is exactly what I really want and inspired from FB's dataloader.js. Without which, I might just use node.js for graphql implementation. I'm presenting a way to achieve the same effect even though the underlying runtime is so different between node.js and go.

In the end, what I'm proposing here is just a interface to allow such different execution model to be injected. People can choose different actual executor as they see fits their needs.

(Sorry for potential grammar errors as I'm not a English native speaker.)

@ajackson-cpi
Copy link
Contributor

I'm not sure where to look, but this scenario sounds like it needs a sync.WaitGroup{} that sees all the .Add() calls before the collector tasks start, and something does .Wait() before starting the fetchers. The collector tasks can call .Done()

That's the scatter-gather pattern in GoLang.

@nicksrandall
Copy link

@bigdrum I like your approach of creating a custom scheduler. I know this a first draft but a few features that I think are critical for a datoader are max batch size and max time out. I worry that your implementation under load could grow unbounded. That feature should be pretty easy to add.

@bigdrum
Copy link
Contributor Author

bigdrum commented Mar 8, 2017

@nicksrandall Right that's very good suggestion, and my approach does introduce more footprint comparing to yours as it creates new goroutines when it sees the need to yield to another graphql branch to collect more tasks (but note at a given moment only one goroutine is active, all others are waiting to be scheduled).

The interesting thing is that this logic could be put into the custom scheduler. For example, we could provide some fairness to the low priority queue, like if the high priority queue size reaches some limit, dispatch the task in low priority queue. This allows the pending batch to be flushed. Same for the time based criteria.

Since the dataloader/scheduler is per graphql query. We could also introduce a shared semaphore state so that the total pending tasks of a server could not exceed some given limit.

@ajackson-cpi
Copy link
Contributor

I'm interested in seeing this merged too. What's the status?

@sandeepone
Copy link

Any Update? 👍

@divoxx
Copy link

divoxx commented Jun 20, 2018

Any updates on this?

@shengwu
Copy link

shengwu commented Jul 13, 2018

Any updates on this vs parallel resolution?

@chris-ramon
Copy link
Member

Thanks a lot guys! 👍 — Closing this one in favor of #388, you might want to take a look to a real-use case working example that shows the support for concurrent resolvers.

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.