Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

[DO NOT MERGE] Use require relative to load files. #476

Closed
wants to merge 1 commit into from

3 participants

@xaviershay
Collaborator

In theory this should be faster, but I can't get a statistically
significant benchmark showing it is.

I'm currently not in favour of this change:

  • It's weird.
  • It requires defining a global method.
  • It rules out the ability to require files individually.
  • I can't show that it actually improves performance to justify the above.

#464

@xaviershay xaviershay Use require relative to load files.
In theory this should be faster, but I can't get a statistically
significant benchmark showing it is.
ac30e0b
@myronmarston

It's weird.

Do you find require_relative weird in general? Or do you just mean the kinda hackish nature of using require_relative if it's available but not otherwise?

It requires defining a global method.

There are simple ways to avoid defining a global method:

https://github.com/rspec/rspec-core/blob/508f93baf6063080007266265f909683871f2f8d/lib/rspec/core.rb#L1-L42

Or we could define RSpec::Expectations._require or something like that.

It rules out the ability to require files individually.

How so?

I can't show that it actually improves performance to justify the above.

If there's no noticable perf improvement, then certainly we should not do it. That said, this got done in rspec-core due to a quite noticable perf improvement it provided, as reported by a user:

https://groups.google.com/forum/#!searchin/rspec/require_relative/rspec/G4Dntk9aVeg/WYHhxMSsALAJ

I confess I haven't benchmarked this myself. I'd like to play with it a bit before ruling out doing this (at which point we should probably remove the code for this from rspec-core)...or, if I find a case where it is noticably faster than I'd like to do some version of it.

@xaviershay
Collaborator

kinda hackish nature

this

It rules out the ability to require files individually.

because you also need to ensure you have the file that defines the hack loaded. I guess if that is in it's own file you could do it, but now you're adding an extra require to every file...

Or we could define RSpec::Expectations._require or something like that.

Oh right, that's kind of obvious :/

I also forgot to mention: there are a heap of autoloads in this lib, I'm not sure there's a way to apply this fix to them.

@xaviershay
Collaborator

Also worth noting that require performance in 1.9.3 was a massive regression, and has since been fixed in 2 and later.

@JonRowe
Owner

I'm leaning heavily on "let's not do this" now... given the regression on 1.9.3 for next to no benefit?

@myronmarston

I found something interesting in rspec/rspec-core#1348 but my son wants to play a game so I'll explain later :).

@myronmarston

OK...so in that rspec-core PR, I set out to benchmark require vs require_relative to see what kind of difference it makes. My initial attempt at benchmarking (rspec/rspec-core@af5f60a) showed no discernable difference. It used this shell command to do the benchmarking:

time (for i in {1..100}; do ruby -Ilib:../rspec-support/lib -e "require 'rspec/core'"; done)

Then I noticed that there's already a benchmark script there, where David had measured a 12% difference in 1.9.3 (probably the newest MRI at the time):

https://github.com/rspec/rspec-core/blob/v3.0.0.beta2/benchmarks/require_relative_v_require.rb

I tried this benchmark on 1.9.3, 2.0 and 2.1 in rspec/rspec-core@78daad6 and definitely saw a noticeable difference.

This puzzled me at first, but after thinking about it some more, I think it actually makes perfect sense. Here's what I think is going on:

  • I believe the performance of require is O(N) where N is the number of directories in $LOAD_PATH -- or more precisely, the index in the load path where it resolves to a file. Think about how require works: it linearly searches $LOAD_PATH, in the order of the directories, to find the first directory where it finds a matching file, and loads that file. Therefore, if it resolves it against the first directory in the load path, you can think of it as being constant time, but if it resolves against the 75th directory in the load path, it had to try 75 times before it find a match (and will take approximately 75 times as long).
  • require_relative, on the other hand, is O(1) -- at least, I think it is. It's requiring a file relative to the current file, which means it doesn't have to iterate over the $LOAD_PATH to find a match -- it can look directly to see if the file is there or not in a single step.

In my first benchmark, I setup the load path with -I. Consider the output when I run ruby -Ilib -e 'puts $:.join("\n")':

/Users/myron/code/rspec-dev/repos/rspec-core/lib
/Users/myron/.rubies/ruby-2.0.0-p247/lib/ruby/site_ruby/2.0.0
/Users/myron/.rubies/ruby-2.0.0-p247/lib/ruby/site_ruby/2.0.0/x86_64-darwin12.4.0
/Users/myron/.rubies/ruby-2.0.0-p247/lib/ruby/site_ruby
/Users/myron/.rubies/ruby-2.0.0-p247/lib/ruby/vendor_ruby/2.0.0
/Users/myron/.rubies/ruby-2.0.0-p247/lib/ruby/vendor_ruby/2.0.0/x86_64-darwin12.4.0
/Users/myron/.rubies/ruby-2.0.0-p247/lib/ruby/vendor_ruby
/Users/myron/.rubies/ruby-2.0.0-p247/lib/ruby/2.0.0
/Users/myron/.rubies/ruby-2.0.0-p247/lib/ruby/2.0.0/x86_64-darwin12.4.0

Notice that it prepends the load path with the specified directory -- which means in my initial benchmark, I was comparing the best-case constant time performance of require against the always constant-time performance of require_relative.

In the second benchmark, I used bundle exec to run the benchmark (because that's what David had done), and playing around in bundle exec irb, I'm seeing that rspec-core's lib dir is at index 44 in the load path -- so each rspec-core require had 44 tries before it resolved.

So...here's what I think this means:

  • The perf difference between require and require_relative has nothing to do with the 1.9.3 require regression.
  • require_relative is never slower than require, as far as I can tell.
  • The improved performance of require_relative varies from "not at all" to "quite noticable", but it depends entirely on how many directories are on the loath path and rspec's placement in the load path.
  • In practice, I expect the require_relative perf improvement to be noticeable on projects with many gems in their Gemfile (such as a large or even typical rails app).

So...after my initial benchmark, I was thinking I agreed with both of you about not doing this (and also removing it from rspec-core). After the second benchmark and further reflection, I'm leaning towards doing it in all the rspec repos. However, I first want to test my thinking above some more. I'm going to try running some benchmarks with 10, 100 and 1000 dirs on my load path and see how require vs require_relative compares.

@myronmarston

OK, so I did the 10 vs 100 vs 1000 dirs in load path benchmarking I mentioned above:

rspec/rspec-core@950f6db

The results confirm what I conjectured above:

  • The boot time using require increased drammatically when I added more dirs on the load path. To boot 50 times it took 3.8 vs 5 vs 18.8 seconds with 10, 100 and 1000 load paths.
  • The perf improvement of using require_relative is much greater when there are more load paths. Initially I was surprised to see the numbers for the 100 and 1000 paths runs going up, but given that loading rspec-core has some non-relative requires (such as rspec-support and stdlib stuff), it makes sense that those got slower and the time increased.

@xaviershay / @JonRowe -- thoughts?

@JonRowe
Owner

I'm ok with it if we can refactor it into a non global method, how about we put this in support? Then we can have RSpec::Support.require or similar?

@myronmarston

I'm ok with it if we can refactor it into a non global method, how about we put this in support? Then we can have RSpec::Support.require or similar?

I can't think of a way to make it work for other libs from rspec-support, because require_relative is relative to whatever __FILE__ is when it's called -- so if the require_relative line is in rspec-support, it'll only work for files in that lib. I think each lib will need it's own custom require method. Not ideal, but it's a pretty small amount of duplication.

@myronmarston

Actually, I figured out a way we can reduce the duplication and leverage rspec-support. I'm working on a PR.

@myronmarston myronmarston referenced this pull request from a commit in rspec/rspec-support
@myronmarston myronmarston Add support for optimized requires.
See rspec/rspec-expectations#476
for the discussion and background to this.
6863276
@myronmarston myronmarston referenced this pull request from a commit in rspec/rspec-support
@myronmarston myronmarston Add support for optimized requires.
See rspec/rspec-expectations#476
for the discussion and background to this.
d5f41cf
@myronmarston myronmarston referenced this pull request in rspec/rspec-support
Merged

Add support for optimized requires. #45

@xaviershay
Collaborator

new benchmark convinces me.

@JonRowe
Owner

:+1:

@myronmarston

@xaviershay -- do you want to update this PR (or open a new one) that leverages the new support for this in rspec-support? Similar to rspec/rspec-mocks#607.

@xaviershay xaviershay closed this
@xaviershay
Collaborator

I won't be able to get this until the weekend, if anyone else wanted to have a crack.

@abotalov abotalov referenced this pull request from a commit in abotalov/xpath
@abotalov abotalov Gem infrastructure changes
* Use new and shiny require_relative (rspec/rspec-expectations#476 (comment))
* Remove references to RubyForge as it will die soon
* Require Ruby >= 1.9.3 and build on newer rubies
* Remove bundler_stubis from gitignore as Google knows nothing about it
* Remove references to RDoc as project uses YARD
* Remove rubygems_version as it shouldn't be specified (http://guides.rubygems.org/specification-reference/#rubygems_version)
d3a64f2
@abotalov abotalov referenced this pull request in jnicklas/xpath
Open

Infrastructure changes #62

@abotalov abotalov referenced this pull request from a commit in abotalov/xpath
@abotalov abotalov Gem infrastructure changes
* Use new and shiny require_relative (rspec/rspec-expectations#476 (comment))
* Remove references to RubyForge as it will die soon
* Require Ruby >= 1.9.3 and build on newer rubies
* Remove bundler_stubis from gitignore as Google knows nothing about it
* Remove references to RDoc as project uses YARD
* Remove rubygems_version as it shouldn't be specified (http://guides.rubygems.org/specification-reference/#rubygems_version)
3f24a45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 22, 2014
  1. @xaviershay

    Use require relative to load files.

    xaviershay authored
    In theory this should be faster, but I can't get a statistically
    significant benchmark showing it is.
This page is out of date. Refresh to see the latest.
View
23 lib/rspec/expectations.rb
@@ -1,13 +1,22 @@
+if Kernel.respond_to?(:require_relative)
+ def require_rspec_expectations(path)
+ require_relative path
+ end
+else
+ def require_rspec_expectations(path)
+ require "rspec/#{path}"
+ end
+end
require 'rspec/support/caller_filter'
require 'rspec/support/warnings'
-require 'rspec/matchers'
-require 'rspec/expectations/expectation_target'
-require 'rspec/matchers/configuration'
-require 'rspec/expectations/fail_with'
-require 'rspec/expectations/handler'
-require 'rspec/expectations/version'
-require 'rspec/expectations/diff_presenter'
+require_rspec_expectations 'matchers'
+require_rspec_expectations 'expectations/expectation_target'
+require_rspec_expectations 'matchers/configuration'
+require_rspec_expectations 'expectations/fail_with'
+require_rspec_expectations 'expectations/handler'
+require_rspec_expectations 'expectations/version'
+require_rspec_expectations 'expectations/diff_presenter'
module RSpec
# RSpec::Expectations provides a simple, readable API to express
View
4 lib/rspec/expectations/diff_presenter.rb
@@ -1,6 +1,6 @@
require 'diff/lcs'
-require "rspec/expectations/encoded_string"
-require "rspec/expectations/differ"
+require_rspec_expectations "expectations/encoded_string"
+require_rspec_expectations "expectations/differ"
require 'diff/lcs/hunk'
require 'pp'
View
14 lib/rspec/matchers.rb
@@ -1,10 +1,10 @@
-require 'rspec/matchers/pretty'
-require 'rspec/matchers/composable'
-require 'rspec/matchers/built_in'
-require 'rspec/matchers/generated_descriptions'
-require 'rspec/matchers/dsl'
-require 'rspec/matchers/matcher_delegator'
-require 'rspec/matchers/aliased_matcher'
+require_rspec_expectations 'matchers/pretty'
+require_rspec_expectations 'matchers/composable'
+require_rspec_expectations 'matchers/built_in'
+require_rspec_expectations 'matchers/generated_descriptions'
+require_rspec_expectations 'matchers/dsl'
+require_rspec_expectations 'matchers/matcher_delegator'
+require_rspec_expectations 'matchers/aliased_matcher'
module RSpec
# RSpec::Matchers provides a number of useful matchers we use to define
View
2  lib/rspec/matchers/built_in.rb
@@ -1,4 +1,4 @@
-require 'rspec/matchers/built_in/base_matcher'
+require_rspec_expectations 'matchers/built_in/base_matcher'
module RSpec
module Matchers
View
2  lib/rspec/matchers/built_in/be.rb
@@ -1,4 +1,4 @@
-require 'rspec/matchers/dsl'
+require_rspec_expectations 'matchers/dsl'
module RSpec
module Matchers
View
2  lib/rspec/matchers/configuration.rb
@@ -1,4 +1,4 @@
-require 'rspec/expectations/syntax'
+require_rspec_expectations 'expectations/syntax'
module RSpec
module Matchers
Something went wrong with that request. Please try again.