Using require_relative in the Rails codebase

[amatsuda]

Summary

This PR replaces 773 Kernel#require calls to Kernel#require_relative for speed.

Details

Ruby has a faster variation of Kernel#require named Kernel#require_relative since its version 1.9. It's considered faster because it simply directly loads the specified file instead of scanning through all $LOAD_PATH entries.
https://github.com/ruby/ruby/blob/24171da2226cf82bdd46b691d56ccffafb6b30b4/load.c#L838-L846
Also, require_relative is not overridden by any 3rd party libraries such as RubyGems, Bundler, Polyglot, ActiveSupport, etc, unlike require. So we can bypass evil hacks from these gems by calling require_relative.

However, this feature is very rarely used in the Rails codebase. git grep shows only 79 occurrences in the whole repo, and most of them seems to be tests, bin/*, version.rb, etc.
So there're big room for improvements here. And this approach is much easier than adding yet another hack on Kernel#require.

Benchmark

This patch reduces 1,010 Ruby level methods calls when starting up a vanilla Rails app.

diff --git a/bin/rails b/bin/rails
index 0739660..984440e 100755
--- a/bin/rails
+++ b/bin/rails
@@ -1,4 +1,9 @@
 #!/usr/bin/env ruby
 APP_PATH = File.expand_path('../config/application', __dir__)
+count = 0
+trace = TracePoint.new(:call) {|tp| count += 1}
+trace.enable
+
 require_relative '../config/boot'
 require 'rails/commands'
+p count

In terms of speed, reduced time was about 6..7% (only 0.1 second) in average on a vanilla app on my local machine. It may be different if the app has longer $LOAD_PATH.

[bogdanvlviv]

Related to #29417

[fxn]

Superlative patch Akira.

I am personally positive about using require_relative like you propose. At a first glance the changes seem reasonable, but it is a big patch. I'll do a detailed pass as soon as I can.

[matthewd]

TBH I'm not thrilled about how these look... for the same reason I'd use a link to /users/bob from /users/fred, I guess: symmetry, avoidance of incidental dependency on filenames, etc

It does seem worthwhile despite my aesthetic concerns, though. 👍

[matthewd]

☝️

[amatsuda]

Wow! Good catch! Fixed.

[fxn]

@matthewd share the aesthetic concerns, the symmetry breaks, does not look as nice. I believe we see it similarly, as a trade-off, sacrifice a bit aesthetics for practical reasons.

Regarding the dependency on file names, I believe in practice it is less than one could think. If you move a file, you'll generally need to revise require calls, either normal or relative. Moving a file to another directory under $LOAD_PATH with the same path is rare in practice. You have extra work for the relative requires within a file being moved though, which is again a trade-off you choose.

[matthewd]

I did have one thought on a different aesthetic trade-off:

Taking actionpack/lib/action_dispatch/middleware/debug_exceptions.rb as an example, instead of:

-require "action_dispatch/http/request"
-require "action_dispatch/middleware/exception_wrapper"
-require "action_dispatch/routing/inspector"
+require_relative "../http/request"
+require_relative "exception_wrapper"
+require_relative "../routing/inspector"

We could:

-require "action_dispatch/http/request"
-require "action_dispatch/middleware/exception_wrapper"
-require "action_dispatch/routing/inspector"
+require_relative "../../action_dispatch/http/request"
+require_relative "../../action_dispatch/middleware/exception_wrapper"
+require_relative "../../action_dispatch/routing/inspector"

It restores [most of] the symmetry, by using a few needless levels of directory traversal -- but retains the speed of avoiding the load path search.

I'm not sure how I feel about it, but thought I'd throw it out there.

[fxn]

I think we could merge this patch, it is huge and we agree it looks in principle like a good change. Then, we can perhaps revise details here and there if we wish, or make a second pass with some uniform pattern if we prefer it.

I have done a systematic pass over all the changes and they seem correct.

The alternative going always up to the root of the component following parent directories might be interesting to explore, though this patch makes a normal usage of require_relative and the kind of edits present are the ones one would normally write. I guess in some cases we'd have a chain of dots that might look a bit long and strange, or brittle, or difficult to check if they are correct (though you could not load the file at all otherwise). But if the uniformity that it brings is worthwhile, then maybe the trade-off is moved to that pattern.

Alternatively, I was thinking that maybe we could just visually sort the lines. From external to internal so to speak. First require files in other gems or standard library, you know, the most external stuff, and then by descending number of leading dots. So at least you see some logic/uniformity.

@amatsuda thanks again for this work ❤️ .

[pointlessone]

I wonder if Ruby issue 10222 will manifest itself more often with this change. Will people feel concerned about elevated warnings numbers?

[fxn]

@pointlessone thanks for the pointer, that has to be kept in mind. Let's see what's the experience in practice.

[fxn]

For the archives, this was fixed in 4816e82.

[betesh]

@pointlessone wrote:

I wonder if Ruby issue 10222 will manifest itself more often with this change.

It is manifesting itself for me right now on a Jenkins continuos integration server, where /var/lib/jenkins is a symlink to /home/jenkins

Aside from the numerous warnings resulting from requiring the same file twice, this makes it impossible to load ActiveSupport::Callbacks:

09:18:08 /home/jenkins/.rvm/gems/ruby-2.2.2/bundler/gems/rails-e5fb6ed62b17/activesupport/lib/active_support/concern.rb:128:in `included': Cannot define multiple 'included' blocks for a Concern (ActiveSupport::Concern::MultipleIncludedBlocks)
09:18:08 	from /home/jenkins/workspace/my_engine/spec/dummy/config/application.rb:9:in `included'
09:18:08 	from    /home/jenkins/.rvm/gems/ruby-2.2.2/bundler/gems/rails-e5fb6ed62b17/activesupport/lib/active_support/callbacks.rb:65:in `<module:Callbacks>'
09:18:08 	from    /home/jenkins/.rvm/gems/ruby-2.2.2/bundler/gems/rails-e5fb6ed62b17/activesupport/lib/active_support/callbacks.rb:62:in `<module:ActiveSupport>'
09:18:08 	from    /home/jenkins/.rvm/gems/ruby-2.2.2/bundler/gems/rails-e5fb6ed62b17/activesupport/lib/active_support/callbacks.rb:13:in `<top (required)>'
09:18:08 	from    /home/jenkins/.rvm/gems/ruby-2.2.2/bundler/gems/rails-e5fb6ed62b17/activesupport/lib/active_support/execution_wrapper.rb:3:in `require_relative'
09:18:08 	from    /home/jenkins/.rvm/gems/ruby-2.2.2/bundler/gems/rails-e5fb6ed62b17/activesupport/lib/active_support/execution_wrapper.rb:3:in `<top (required)>'
09:18:08 	from /var/lib/jenkins/.rvm/gems/ruby-2.2.2/bundler/gems/rails-e5fb6ed62b17/activesupport/lib/active_support/executor.rb:3:in `require_relative'
09:18:08 	from /var/lib/jenkins/.rvm/gems/ruby-2.2.2/bundler/gems/rails-e5fb6ed62b17/activesupport/lib/active_support/executor.rb:3:in `<top (required)>'
09:18:08 	from    /home/jenkins/.rvm/gems/ruby-2.2.2/bundler/gems/rails-e5fb6ed62b17/railties/lib/rails/application.rb:134:in `initialize'
09:18:08 	from /var/lib/jenkins/.rvm/gems/ruby-2.2.2/bundler/gems/rails-e5fb6ed62b17/railties/lib/rails/railtie.rb:165:in `new'
09:18:08 	from /var/lib/jenkins/.rvm/gems/ruby-2.2.2/bundler/gems/rails-e5fb6ed62b17/railties/lib/rails/railtie.rb:165:in `instance'
09:18:08 	from    /home/jenkins/.rvm/gems/ruby-2.2.2/bundler/gems/rails-e5fb6ed62b17/railties/lib/rails/application.rb:95:in `instance'
09:18:08 	from /var/lib/jenkins/.rvm/gems/ruby-2.2.2/bundler/gems/rails-e5fb6ed62b17/railties/lib/rails/railtie.rb:125:in `config'

Notice how it alternates between loading rails files from /var/lib/jenkins/.rvm/gems/ruby-2.2.2 and /home/jenkins/.rvm/gems/ruby-2.2.2.

EDIT

My current workaround is to configure every CI job to use a custom path for its gems so that the gems in use aren't installed in a symlinked directory:

bundle install --path vendor/bundle

[yskkin]

@betesh 's problem is same as #29846.
I'm thinking about submitting PR that reverts this, but I'm quite not sure reverting this is proper approach to fix #29846.

[fxn]

I have been thinking about this and I am not totally sure what is the right thing to do.

Rails uses require_relative as any other Ruby program. This patch just adds more calls to the already existing ones. From that point of view, this code base is using Ruby idiomatically. If MRI has a bug for require_relative in some edge case, you could argue this is a Ruby issue and people finding this problem should workaround Ruby as @betesh did.

From the point of view of Rails team, we have introduced something that in practice has that consequence regardless of the source. Files should not be evaluated twice, period.

So, the mathematician in me leans towards the former, and the pragmatist in me leans just slightly towards the latter.

/cc @rafaelfranca @tenderlove @matthewd

[fxn]

Well, "introduced" may not be exact. Potentially this could be already there, any require_relative in Rails components can potentially misbehave if that file is now or in the future referenced with a require somewhere else.

So it is mostly a black & white situation I believe. Either we go with the first option, or else we ban require_relative from the entire code base except in the most trivial cases in which a require is not possible.

[matthewd]

Yeah, given that it's apparently a nontrivial bug in a relatively (😛) new feature, it does seem like we should avoid it for now. If it were fixed upstream and headed for release on all supported series, it might be reasonable to just expect symlink users to upgrade... but as the bug seems stalled, I think symlinks are too common for us to dismiss as Not Our Problem.

[pointlessone]

I don't think double load is fatal. In most cases it's just a bunch of constants redefined/classes reopened. It may be breaking in some pathological cases when some crazy (and broken) metaprogramming is involved, though.

I'd hope Rails being affected can motivate someone to fix it on MRI side.

[matthewd]

We could go with require "#{__dir__}/../etc", though... keep the distinction between things that are nominally relative and things that are being deferred to the load path by design (so it's trivial to switch back later), and presumably save some path searching in the process.

That spelling does do The Right Thing, doesn't it?

[pointlessone]

require "#{__dir__}/../etc"

I don't think that's any different to require_relative.

Consider when a file first required through a symlimked path. Then, through __dir__ (which is a real path, I believe). Same issue.

[matthewd]

Oh! I misunderstood the problem... require_relative is getting it "right"; it's require that's storing the possibly-still-symlink-containing path.

Does that suggest we could work around this by getting rubygems to realpath before it requires?

Alternatively, we could define our own require_relative-alike, which worked relative to File.dirname(caller_locations(1, 1).first.path). We'd want to be pretty sure that benchmarked well before bothering with such complication, though, I think.

[pointlessone]

@matthewd Please take a look at MRI issue 10222 for broader context.

[fxn]

@amatsuda you're a Ruby committer, which is your point of view?

[amatsuda]

@fxn @matthewd @pointlessone @yskkin We discussed about this at the developer meetings and RubyKaigi. We concluded this to be require_relatives bug, then fixed this problem in ruby trunk.
ruby/ruby@5754f15
ruby/ruby@b6d3927

So, current Rails master should work as expected with Ruby 2.5+, but this problem remains until we drop Ruby < 2.5 support (or we overwrite require_relative to store realpath).

I'm kind of happy that we could find and fix Ruby's bug through this challenge, but I'm sorry for messing up some edge users' environment. I'll soon revert my changes.

[matthewd]

@amatsuda great, thanks!

Do you have any sense of whether that's likely to be backported?

---

For a workaround, as we specify that the gem's "main" file is to always be required first, I wonder if we could update $: to expand the gem's path at the top of active_support.rb (and likewise for the other gems).

[bogdanvlviv]

So, current Rails master should work as expected with Ruby 2.5+, but this problem remains until we drop Ruby < 2.5 support (or we overwrite require_relative to store realpath).

Since Rails requires Ruby version 2.5.0 or later on the master, we could continue working on this patch.

[kaspth]

@bogdanvlviv I don't see a problem with attempting a 2019 version of this — just give @amatsuda credit for the commits if you use these as the base.

[amatsuda]

OK, I'll work on this again, maybe after 6.0.0 stable release :)

[bogdanvlviv]

@amatsuda I've been already working on this https://github.com/bogdanvlviv/rails/commits/require_relative_2019. I had to ask you whether i could work on this before I started (since you are the author of this PR). Do you mind if I finish work on this?

[kaspth]

@amatsuda still interested in updating this or would you rather pass the buck to @bogdanlviv? Bogdan are you interested in continuing or would you rather pass to someone else?

[amatsuda]

Thanks @bogdanvlviv for your offer, but this might perhaps easier and more reliable for me than anyone else to do because I preserve the script that I used for generating this patch.
I'll rerun the script and PR again soon. :)

[bogdanvlviv]

Thanks @bogdanvlviv for your offer, but this might perhaps easier and more reliable for me than anyone else to do because I preserve the script that I used for generating this patch.

👍. Actually, that's why I was asking. Using a script for this definitely will be safer and less time-consuming than doing this by hands.

[amatsuda]

Oh, maybe I'll PR this "require active_support/all everywhere" first, so we can cut some require calls.