-
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?
Conversation
There's still a lot to do here for compatibility and performance, but the initial benchmark results are surprisingly good (even with the terrible ruby 3.4.1 (2024-12-25 revision 48d4efcb85) +PRISM [x86_64-darwin22]
Calculating -------------------------------------
graphql-ruby: 142 resolvers
- 399.549 (± 8.5%) i/s (2.50 ms/i) - 2.000k in 5.048521s
+ 538.453 (± 3.9%) i/s (1.86 ms/i) - 2.695k in 5.013324s
graphql-ruby: 140002 resolvers
- 0.517 (± 0.0%) i/s (1.93 s/i) - 3.000 in 5.797333s
+ 0.761 (± 0.0%) i/s (1.31 s/i) - 4.000 in 5.272811s
- Total allocated: 10149304 bytes (70353 objects)
+ Total allocated: 13431224 bytes (97389 objects) So that's a 34% or 47% speedup. It's using 30% more memory, because it's currently the Simplest Thing That Could Possibly Work. Presumably refactoring it to avoid those repeated allocations will make it even faster! |
I haven't pushed a commit here for a while but I'm still planning on landing this somehow. Here's a quick brain-dump in case anyone is curious:
|
|
||
def run | ||
# TODO unify the initialization lazies_at_depth | ||
@lazies_at_depth ||= Hash.new { |h, k| h[k] = [] } |
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 yield
s immediately to any work given to it). This keeps performance the same: #5422 (comment)
end | ||
end | ||
|
||
def run_isolated |
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.
What, basically, does run_isolated
do? I remember it's used in context of mutations, so I presume its a serial execution concern? In Cardinal we address serial constraints by splitting up the root into separate execution scopes that run serially, effectively treating it as N separate executions that happen to run in sequence.
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.
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.
end | ||
|
||
attr_accessor :graphql_dead | ||
attr_accessor :graphql_dead, :was_scoped |
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.
What is was_scoped
...? I've seen this floating around but never read back to figure out what it does, but I'm curious about it.
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.
It implements this feature: https://graphql-ruby.org/authorization/scoping.html#bypassing-object-level-authorization
I'm feeling really inspired by @gmac's proof-of-concept over at https://github.com/gmac/graphql-cardinal/, so I thought I'd try again at refactoring execution to use a queue instead of recursive method calls. I have tried before (#4967, #4968, somewhat #3998, #4935) and given up along the way, but I'm going to try again 😅
TODO:
continue_value
,resolve_list_item
call_method_on_directives
ResolveTypeStep
intoResultHash
authorized_new
lazy usage into ResultHashafter_lazy
usages and rebuild them using queue stepsMultiplex
,Interpreter
,Dataloader
)