Benchmark require relative and update to new rspec-support method #1348

Merged
merged 7 commits into from Feb 25, 2014

Conversation

Projects
None yet
3 participants
@myronmarston
Member

myronmarston commented Feb 23, 2014

Some benchmarks for discussion with rspec/rspec-expectations#476.

I'll leave some comments there about what this means, I think.

@myronmarston myronmarston referenced this pull request in rspec/rspec-expectations Feb 23, 2014

Closed

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

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Feb 24, 2014

Member

This has been updated to leverage the rspec support method from rspec/rspec-support#45. It'll fail until that's merged.

Member

myronmarston commented Feb 24, 2014

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[

This comment has been minimized.

@soulcutter

soulcutter Feb 24, 2014

Member

I sorta miss the line-per-require format - it allowed for groupings and it was easier to read even though it was repetitive.

@soulcutter

soulcutter Feb 24, 2014

Member

I sorta miss the line-per-require format - it allowed for groupings and it was easier to read even though it was repetitive.

This comment has been minimized.

@JonRowe

JonRowe Feb 24, 2014

Member

Ya, think I agree...

@JonRowe

JonRowe Feb 24, 2014

Member

Ya, think I agree...

This comment has been minimized.

@myronmarston

myronmarston Feb 24, 2014

Member

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)?

@myronmarston

myronmarston Feb 24, 2014

Member

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)?

This comment has been minimized.

@soulcutter

soulcutter Feb 25, 2014

Member

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.

@soulcutter

soulcutter Feb 25, 2014

Member

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.

myronmarston added some commits Feb 25, 2014

Remove benchmark file.
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]
@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Feb 25, 2014

Member

Any other concerns before I merge this?

Member

myronmarston commented Feb 25, 2014

Any other concerns before I merge this?

@@ -1,5 +1,6 @@
require 'erb'
require 'shellwords'
+require 'set'

This comment has been minimized.

@JonRowe

JonRowe Feb 25, 2014

Member

An aside, are we not worried about polluting stdlib here?

@JonRowe

JonRowe Feb 25, 2014

Member

An aside, are we not worried about polluting stdlib here?

This comment has been minimized.

@myronmarston

myronmarston Feb 25, 2014

Member

This isn't a new require; see 322d272.

We prefer not to pollute stdlib, but it's not an absolute rule.

@myronmarston

myronmarston Feb 25, 2014

Member

This isn't a new require; see 322d272.

We prefer not to pollute stdlib, but it's not an absolute rule.

@JonRowe

This comment has been minimized.

Show comment
Hide comment
@JonRowe

JonRowe Feb 25, 2014

Member

Did you update the benchmarks for the current script?

Member

JonRowe commented Feb 25, 2014

Did you update the benchmarks for the current script?

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Feb 25, 2014

Member

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 require_relative is never slower than require and is faster when $LOAD_PATH is large....so what would be the benefit of updating the benchmarks at this point?

Member

myronmarston commented Feb 25, 2014

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 require_relative is never slower than require and is faster when $LOAD_PATH is large....so what would be the benefit of updating the benchmarks at this point?

@JonRowe

This comment has been minimized.

Show comment
Hide comment
@JonRowe

JonRowe Feb 25, 2014

Member

To prove they have same performance benefit as the raw require_relative, or just remove them as stale already :)

Member

JonRowe commented Feb 25, 2014

To prove they have same performance benefit as the raw require_relative, or just remove them as stale already :)

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Feb 25, 2014

Member

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.

Member

myronmarston commented Feb 25, 2014

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.

myronmarston added a commit that referenced this pull request Feb 25, 2014

Merge pull request #1348 from rspec/benchmark-require-relative
Benchmark require relative and update to new rspec-support method

@myronmarston myronmarston merged commit 1e5ea31 into master Feb 25, 2014

@myronmarston myronmarston deleted the benchmark-require-relative branch Feb 25, 2014

@bf4 bf4 referenced this pull request in rails-api/active_model_serializers Sep 17, 2015

Merged

add require statements to top of file #1171

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment