Benchmark require relative and update to new rspec-support method #1348
Conversation
It appears not to make a difference.
|
This has been updated to leverage the rspec support method from rspec/rspec-support#45. It'll fail until that's merged. |
|
|
||
| RSpec::Support.define_optimized_require_for_rspec(:core) { |f| require_relative f } | ||
|
|
||
| %w[ |
There was a problem hiding this comment.
I sorta miss the line-per-require format - it allowed for groupings and it was easier to read even though it was repetitive.
There was a problem hiding this comment.
I initially had that, but we had more duplicated on each line (the RSpec::Support.require_rspec_core bit) than was different on each line (the file name). So I collapsed it.
What do you think about keeping the array.each but putting each file on its own line (with optional blank lines in there for grouping)?
There was a problem hiding this comment.
Sounds fair to me.
The require method could also take varargs so that RSpec::Support.require_rspec_core is up-front instead of trailing the array… but I don't have strong feelings about that TBH.
This benchmark suggested that `require_relative` isn't any faster than `require`, because it put `rspec/core/lib` onto the load path as the first dir, which made `require` for rspec-core files O(1) like `require_relative`. In practice, people's load path will have many more directories and this benchmark doesn't align with that reality. [ci skip]
|
Any other concerns before I merge this? |
| @@ -1,5 +1,6 @@ | |||
| require 'erb' | |||
| require 'shellwords' | |||
| require 'set' | |||
There was a problem hiding this comment.
An aside, are we not worried about polluting stdlib here?
There was a problem hiding this comment.
This isn't a new require; see 322d272.
We prefer not to pollute stdlib, but it's not an absolute rule.
|
Did you update the benchmarks for the current script? |
Nope. Not sure what you have in mind for updating it. The results? It's also kinda annoying to re-run it as to compare relative vs non-relative require you have to edit rspec-support (since it always uses relative when available). We're satisfied that the benchmarks demonstrate that |
|
To prove they have same performance benefit as the raw require_relative, or just remove them as stale already :) |
I re-ran the benchmarks, and am getting very similar numbers. I'm not going to update them since that'll just create unnecessary git history churn. |
Benchmark require relative and update to new rspec-support method
Some benchmarks for discussion with rspec/rspec-expectations#476.
I'll leave some comments there about what this means, I think.