Autoload nested constants

[gmcgibbon]

Reduces require time from ~160ms to ~5ms. Autoloads all constants under GraphQL to optimize boot time. Also autoloads GraphQL::Types and GraphQL::Query because they can't be required in sequence.

Script:

#!/usr/bin/env ruby
# frozen_string_literal: true

require "bundler/setup"
require "benchmark"

ms = 1000 * Benchmark.realtime do
  require "graphql"
end

puts "#{ms}ms"

Before:

> bin/boot
169.55200000666082ms

After:

> bin/boot
5.60200004838407ms

Rails will automatically eager load GraphQL, but other frameworks / setups will need to call GraphQL.eager_load!. If we agree with the approach, I can document this. Because we don't require on Active Support, we need to manually implement eager loading. ActiveSupport::Autoload implements the eager_load! method for you.

Also, if you're interested, I can implement testing for eager loading (example), but we'll need to roll our own test isolation feature.

[gmcgibbon]

Looks like I have some tests to fix. I'm on it 👀

[gmcgibbon]

I think the rest are happening on the main branch. Let me know if I'm wrong.

[rmosolgo]

Hey, thanks so much for putting in the time on this. It's been in the back of my mind for a long time to improve the organization of all those requires, but I didn't think of approaching it this way.

other frameworks / setups will need to call GraphQL.eager_load!

This seems alright to me. I'm trying to think if we can detect these and warn in cases where this should be called. Are there other setups that come to mind besides Sinatra and Hanami? (I mean, there's plain Rack, and a long tail of other possibilities, but any other big ones?)

testing for eager loading

I'm comfortable assuming that autoload works... but is there any way that we could make sure eager_load! will stay in-sync with our autoload ... configurations? For example, something like:

all_constants = GraphQL.constants.map { |const_name| GraphQL.const_get(const_name) }
eager_loadable_constants = all_constants - [
  # Any specific exceptions to autoloading can go here, along with a comment for why they're skipped
]
assert_equal eager_loadable_constants, GraphQL.eager_load!

And then do something similar for GraphQL::Query.

Yes, those other tests are failing on master 😿 . I haven't been able to get the system_tests to fail locally and I can't figure out how to fix the flaky test where database tables are somehow missing...

[gmcgibbon]

Are there other setups that come to mind besides Sinatra and Hanami?

I'll investigate this. Let's narrow the scope to Sinatra, Hanami, and Rails. Everything else can be supported later, and we can instruct folks to call that method themselves in more exotic cases. I'll add this to the docs.

I'm comfortable assuming that autoload works... but is there any way that we could make sure eager_load! will stay in-sync with our autoload ... configurations?

Sounds good. I'll re-implement a minimal version of ActiveSupport::Autload so that we can automate the eager_load! method.

[gmcgibbon]

I wrote a document page, added GraphQL::Autoload, added a warning for production not eager loading, and some rudimentary autoload tests. I can test the warning, but that would require an isolated test suite and re-implementing ActiveSupport::Testing::Isolation.

[rmosolgo]

Looks good, thanks again for this improvement! I'm going to shuffle the doc around a bit before I publish.

[rmosolgo]

I'm looking over it one more time and adding more docs... it seems like eager_load! should continue recursively, calling .eager_load! on any loaded constants which respond to that method (eg, GraphQL.eager_load! should call GraphQL::Query.eager_load!). Does that sound right to you? I'll open a PR with the change to review.

[rmosolgo]

🚢 in 2.4.5! Thanks again.

[gmcgibbon]

Ah, you're right. Thanks for the correction!

[sealabcore]

We are seeing this new error in our Rails app on web server boot, production console boot, and some rake tasks. Should we be seeing this? The docs in this PR mention that nothing should be needed when using the Graphql-Ruby gem with a Rails app.

[rmosolgo]

Hey @sealabcore, no... it's not supposed to show that all the time :S I was able to replicate this immediately with rails new .... Digging in, its a problem with the warning -- its detection for whether autoloading is happening or not is not sophisticated enough.

First, Rails calls Zeitwerk::Loader.eager_load_all, which loads application code (including app/graphql/my_schema.rb), which references GraphQL::Schema, which emits this warning.

Then, it does eager_load_namespaces.each(&:eager_load!) which eager-loads GraphQL's constants.

Here's the source for that: https://github.com/rails/rails/blob/91d456366638ac6c3f6dec38670c8ada5e7c69b1/railties/lib/rails/application/finisher.rb#L79-L81

So I think everything works -- everything is loaded when the application starts -- but the integration needs to be improved. I'll take a closer look tomorrow, sorry for the trouble!

[gmcgibbon]

Hm, the application could reference any GraphQL constant in theory, so it might make more sense to use config.before_eager_load { GraphQL.eager_load! } instead of appending to config.eager_load_namesapces.

[gmcgibbon]

I can confirm this fixes the issue. If we want to make sure the gem is eager loaded before we eager load the app, I think this is our best bet, so I'll issue the patch. Sorry for the problems folks.

#5182

[rmosolgo]

Thanks for taking care of it, @gmcgibbon! I just shipped that patch in 2.4.6.

@sealabcore, thanks for reporting this and sorry for the trouble. Please give that new version a try and let me know if it gives you any more issues!

[sealabcore]

Thanks to you both for the quick resolution! I’ll give this a try today.

[zvkemp]

We've had a report that this change breaks constant loading if any of the nested files are required during app boot (I can open a new issue on this if necessary).

Here's a minimal reproduction:

require 'bundler/inline'

gemfile do
  source 'https://rubygems.org'
  gem 'graphql', '2.4.7'
end

require 'graphql'
require 'graphql/tracing/active_support_notifications_trace'

GraphQL.eager_load!

What I think is happening is that the nested graphql/tracing/**.rb files don't require their parent (tracing.rb). So requiring this file:

• calls require "graphql/tracing/platform_trace",
• which loads platform_trace.rb,
• which encounters the autoload-registered constant GraphQL::Tracing
• which calls `require 'graphql/tracing'
• which calls require 'graphql/tracing/platform_trace', which is skipped because this file has already been required — at this point the loading of the PlatformTrace module is higher up in the callstack, so it's not going to complete until after the rest of tracing.rb is loaded
• Unfortunately, the rest of tracing.rb includes files that refer to PlatformTrace, so this error is raised:
  NameError: uninitialized constant GraphQL::Tracing::AppOpticsTrace::PlatformTrace

I believe all of these nested files should include a top-level require 'graphql/tracing' to ensure the order is correct.