-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Lazily load types and fields in a schema #4919
Comments
Hey, thanks for the detailed write-up. I'm definitely open to exploring options for loading the schema as-needed. Currently, any GraphQL query loads the entire schema, because, as you noticed, I think making a
If we did all that, we'd probably want to keep the old code too, so that in Production, the whole schema could be loaded upfront -- before the first request comes in, to reduce latency, and before any forking, to improve memory sharing in copy-on-write situations. So, it's all possible -- but I think it will be a bit more than accepting procs for |
Thanks for mapping out the process @rmosolgo. I'll try to roughly implement this and let you know if I get stuck. Once we have something working I imagine it will be easier to accept a patch. |
Looks like I'm wrong about this. As soon as the schema is loaded everything is loaded (which also explains my particularly flamegraphs). Proc typed fields just defer loading when you load them in isolation (eg. |
👋 I'm gearing up to take a try at this. You mention something that's halfway there -- have you had any luck in your approaches to this change? |
I'm starting on this in #4974. So far I've done the bare minimum: delay calls to I plan to keep working on:
|
Sorry I was on vacation. Before I left, I tried making schema additions lazy, but I'm not super happy with the implementation yet. I tried to make the type loading happen when we execute a query (so we load what we use in the query, essentially). It doesn't account for eager-loading yet, though. I think we can save a bunch of time if we simply lazy-load the root types, which seems like an easier thing to implement so I'm looking into that next. In our case specifically, 99% of the time we're parsing queries and not mutations, but it takes a long time to load the root mutation when we parse a query. Also in our case, we use procs over strings to get some semblance of type coherence in our code, so I'm not sure if strings will work. Though, this makes getting the name of the type very difficult, and AFAIK field objects take issue with not knowing the type upfront because of the extension API. I'll take a closer look at your PR this week (I'm sure its much more coherent than my work), thanks so much for looking into this! |
What do you think of accepting procs as types as a good next step? It seems like any future in this area will need a way to load I think that work could be done as a precursor, what do you think? (And a lot of it was done in your |
My latest attempt is to build a replacement for I think this approach will let me build a new implementation of type lookup while maintaining parity (or conscious difference) with the current implementation. Also, they can be swapped at runtime (and maybe even run at the same time, so you could get log messages when they diverge...). It's basically working now, but for it to really be useful, I'll have to tune it to do as little |
Awesome! I got lazy type evaluation to work on the schema I was working on, but it is a little hacky. I did it by deferring load hook execution: class MySchema < GraphQL::Schema
unless Rails.configuration.eager_load
def self.root_type_for_operation(operation)
case operation
when "query"
query.run_load_hooks_once
own_types.delete(query.graphql_name) # delete and re-add type (and nested types)
add_type_and_traverse(query, root: true)
when "mutation"
mutation.run_load_hooks_once
own_types.delete(mutation.graphql_name) # delete and re-add type (and nested types)
add_type_and_traverse(mutation, root: true)
end
super
end
end
end
end The load hooks are what makes loading my schema so slow, and were initially executed as soon as the root type is loaded. With this patch, the warden problem and the schema constant loading problem don't matter. However, this is a band-aid and I don't think it is correct long-term. I think if we unwrap proc-wrapped root types in |
That's great, I'm glad you found something that works for now 👍 What does |
It calls something like this: @called ||= begin
ActiveSupport.run_load_hooks(:query_root, self)
true
end That way you can write lines like this in initializers: ActiveSupport.on_load(:query_root) do
# field definitions or module includes here...
end This basically just lets us control when callbacks are invoked to sidestep the issue. I'll be able to take a second look at the upstream implementation of this in ~2 weeks. |
Thanks for sharing, very cool. No hurry on #4998, I'm going to keep chipping away at it over there. I've got it nearly full parity with Warden, but I think I'm going to identify the parts of the new code which require loading all types, and see if it can be made to not do that -- even if it breaks parity. I think with good documentation and runtime warnings, a migration would be possible (and would probably require implementing |
Update here: The new
Both of those changes included Rubocop rules to enforce their use when desired. I have a few more improvements to make to |
Is your feature request related to a problem? Please describe.
Right now, resolver classes are defined like this:
In Rails applications with lots of resolvers, this can trigger a lot of autoloads, and contribute to loading a lot of files we don't actually need to execute a query in development mode.
Describe the solution you'd like
GraphQL types have solved this problem already by wrapping type defs in procs like this:
I think it would be great if we could have this syntax to delay loading resolvers until actually used:
But it raises an error:
Describe alternatives you've considered
I considered opening an issue for a feature that lazily loads the root
query
ormutation
, but that seems a little more difficult from a public API perspective, and doesn't really solve the problem as granularly as I'd like. It would defer loading all mutation code until the first mutation, and all query code until the first query. Lazy resolvers should theoretically only load the code and types needed to execute a query/mutation while keeping the root types intact.I also thought about creating a schema for queries and a different one for mutations to split loading the root query and mutation. This would work, but it seems like a hack that doesn't really address the root concern in the GraphQL gem.
Additional context
I noticed this problem when profiling my application and seeing that
AppNameSchema.execute(...)
would load both query and mutation roots (and subsequent resolvers). My application's schema is really big and takes ~3 seconds to load the mutation root and ~2 seconds to load the query root, so this feature would really help the time to first query in autoloaded environments.cc @swalkinshaw
The text was updated successfully, but these errors were encountered: