-
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'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 steps