Skip to content
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

Prefer require_relative #2425

Closed
wants to merge 1 commit into from
Closed

Conversation

marcandre
Copy link
Contributor

There are over 300 calls to require that could be more efficient with require_relative. If I'm not mistaken, current rubygems supports only Ruby 2.2+, so this is a free speed gain.

@deivid-rodriguez
Copy link
Member

Hi @marcandre, thanks for this! This was proposed in #2373, but rejected by @hsbt because he wanted a benchmark to prove the speed gain.

I tried to write one so that he will accept this. You can pass an integer argument to the script that sets the LOAD_PATH size. The higher the argument, the greater the speed gain.

require "fileutils"
require "benchmark/ips"

def load_path(n)
  $LOAD_PATH.replace([])

  (1..n.to_i).map do |dir|
    $LOAD_PATH << dir.to_s
  end

  Dir.mkdir(n)
  FileUtils.touch("#{n}/a.rb")
end

n = ARGV[0]

load_path(n)

Benchmark.ips do |x|
  x.report("require with LOAD_PATH of size #{n}:") { require "a" }
  x.report("require_relative with LOAD_PATH of size #{n}:") { require_relative "#{n}/a" }
  x.compare!
end

File.delete("#{n}/a.rb")
Dir.rmdir(n)

@marcandre
Copy link
Contributor Author

One thing is for sure, require_relative can only be faster than require 😆!

It's also better styling by being more explicit: makes requires of other gems stand out compared to files of rubygems itself.

@deivid-rodriguez
Copy link
Member

Absolutely, I would enable this just for style! But both of the times this was proposed performance was mentioned so I want to make sure it's accepted anyways :)

@MSP-Greg
Copy link
Contributor

MSP-Greg commented Oct 5, 2018

@deivid-rodriguez @marcandre

Thanks for this, and the benchmark. Working with Windows CI as much as I do, I've seen a wide variance In test times. Probably partly due to shared i/o on a drive. This will certainly help...

@amatsuda
Copy link
Member

amatsuda commented Oct 6, 2018

@marcandre I once did the same thing for Rails rails/rails#29638,
then the users hit this bug rails/rails#29638 (comment),
and so we made these fixes in Ruby 2.5 a year ago:
ruby/ruby@5754f15
ruby/ruby@b6d3927

I suppose the users will face the same problem where having rubygems with your patch installed under a symlinked path. We need to wait until rubygems is totally dropping Ruby < 2.5 support.

@deivid-rodriguez
Copy link
Member

Wow, @amatsuda, thanks. I was completely unaware of these issues. Let's wait then, yeah.

@MSP-Greg
Copy link
Contributor

MSP-Greg commented Oct 6, 2018

I'm not familiar with rvm, but I'm wondering about use cases where this would be an issue.

RG Is similar to a std-lib, in that normal installs or updates are always within the lib folder. Gems (Rails) are different.

I haven't been in the bundler code for quite a while, but I rarely see code loading parts of RG. A Gem class method or maybe some of the command files...

There maybe issues with some CI scenarios.

@marcandre
Copy link
Contributor Author

Oh, I see. So any path that might be required (from another gem/app) should not be also require_relative until Ruby 2.5+.

Since it's hard to know for which files this might be the case, probably best to wait then 😿

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants