-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Use a queue for execution #5389
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
base: master
Are you sure you want to change the base?
Changes from all commits
1fdc9cd
9d5b6da
fdbfd2f
8f04cc9
c45f628
32ea55c
c188312
d9f4ebb
56f38c0
186e822
caa754b
0f090cc
325d5d8
b233c92
8837fa7
d3927b8
123bf35
898ba4a
6f6c79e
466a80c
bdc1b08
7f24a79
805605d
96e5d1c
46044fa
e3cf018
b1a8c74
5be5032
f29cac4
c4ebc03
a46bcfa
1f5117e
77e7485
a2285cd
7812e22
b386fd6
7d1b365
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,76 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| module GraphQL | ||
| class Dataloader | ||
| class FlatDataloader < Dataloader | ||
| def initialize(*) | ||
| # TODO unify the initialization lazies_at_depth | ||
| @lazies_at_depth ||= Hash.new { |h, k| h[k] = [] } | ||
| @steps_to_rerun_after_lazy = [] | ||
| @queue = [] | ||
| end | ||
|
|
||
| def run | ||
| while @queue.any? | ||
| while (step = @queue.shift) | ||
| step.call | ||
| end | ||
|
|
||
| while @lazies_at_depth&.any? | ||
| smallest_depth = nil | ||
| @lazies_at_depth.each_key do |depth_key| | ||
| smallest_depth ||= depth_key | ||
| if depth_key < smallest_depth | ||
| smallest_depth = depth_key | ||
| end | ||
| end | ||
|
|
||
| if smallest_depth | ||
| lazies = @lazies_at_depth.delete(smallest_depth) | ||
| lazies.each(&:value) # resolve these Lazy instances | ||
| end | ||
| end | ||
|
|
||
| if @steps_to_rerun_after_lazy.any? | ||
| @steps_to_rerun_after_lazy.each(&:call) | ||
| @steps_to_rerun_after_lazy.clear | ||
| end | ||
| end | ||
| end | ||
|
|
||
| def run_isolated | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What, basically, does There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's used in a couple of hacky places where, for legacy reasons ([which ones??]), we need a dataloader-enabled code block to return right away, without running any other enqueued work. Yeah, it seems like initializing a new dataloader, using it for one thing, then discarding it would do. I'll give that a try sometime. |
||
| prev_queue = @queue | ||
| prev_stral = @steps_to_rerun_after_lazy | ||
| prev_lad = @lazies_at_depth | ||
| @steps_to_rerun_after_lazy = [] | ||
| @queue = [] | ||
| @lazies_at_depth = @lazies_at_depth.dup&.clear | ||
| res = nil | ||
| append_job { | ||
| res = yield | ||
| } | ||
| run | ||
| res | ||
| ensure | ||
| @queue = prev_queue | ||
| @steps_to_rerun_after_lazy = prev_stral | ||
| @lazies_at_depth = prev_lad | ||
| end | ||
|
|
||
| def clear_cache; end | ||
|
|
||
| def yield(_source) | ||
| raise GraphQL::Error, "GraphQL::Dataloader is not running -- add `use GraphQL::Dataloader` to your schema to use Dataloader sources." | ||
| end | ||
|
|
||
| def append_job(callable = nil, &block) | ||
| @queue << (callable || block) | ||
| nil | ||
| end | ||
|
|
||
| def with(*) | ||
| raise GraphQL::Error, "GraphQL::Dataloader is not running -- add `use GraphQL::Dataloader` to your schema to use Dataloader sources." | ||
| end | ||
| end | ||
| end | ||
| end | ||
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose the disadvantage of this is that we always have to take a lazy pathway for running data loader. At present, you can run dataloader inline then return directly if there are no lazies enqueued. With this you'd always need to run dataloader out-of-band to both run jobs and resolve lazies.
I speak in the context of my own interests. This is certainly simpler and I presume better for the library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's a good point -- on this branch, performance is better than baseline after arranging things this way. But I haven't run benchmarks on #5422 yet. I will!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A quick look at the benchmarks revealed that the initial pass at this work should not use the new FlatDataloader (which captures procs but doesn't use Fibers to run them), and instead, should have lazy resolution implemented inside the existing NullDataloader (which
yields immediately to any work given to it). This keeps performance the same: #5422 (comment)