-
Notifications
You must be signed in to change notification settings - Fork 551
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
speed up root_filter #396
speed up root_filter #396
Conversation
raszi
commented
Jul 6, 2015
@@ -4,7 +4,7 @@ | |||
SimpleCov.profiles.define "root_filter" do | |||
# Exclude all files outside of simplecov root | |||
add_filter do |src| | |||
!(src.filename =~ /^#{Regexp.escape(SimpleCov.root)}/i) | |||
!src.filename.downcase.start_with?(SimpleCov.root.downcase) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to think if there's any chance src.filename might be nil
Also, can you try benchmarking with /^#{Regexp.escape(SimpleCov.root)}/io
(the o means it will only compile the interpolated value once), and also maybe \A
instead of ^
, and also maybe ROOT_FILTER = /^{Regexp.escape(SimpleCov.root)}/i)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a bit surprised, regex solution is faster, but not because of the extraction and the o
modifier but because of the \A
change (which of course makes sense), but I did not expect that it will be faster than String#start_with?
.
Calculating -------------------------------------
start_with? 105.000 i/100ms
match_optimized 14.000 i/100ms
match_optimized_A 102.435k i/100ms
match 13.000 i/100ms
-------------------------------------------------
start_with? 1.110k (± 3.5%) i/s - 5.565k
match_optimized 147.382 (± 6.1%) i/s - 742.000
match_optimized_A 2.477M (± 6.6%) i/s - 12.395M
match 143.284 (± 6.3%) i/s - 715.000
Comparison:
match_optimized_A: 2477292.0 i/s
start_with?: 1110.0 i/s - 2231.75x slower
match_optimized: 147.4 i/s - 16808.64x slower
match: 143.3 i/s - 17289.33x slower
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
blog post? That's pretty interesting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mind gisting your benchmark here? or maybe adding it to bench/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This reverts commit 63e0c76.
Which rubies have you tested this against? Only MRI? I wonder if that's an artifact of the regex engine (http://www.geocities.jp/kosako3/oniguruma/ ) ref: discussion #396 (comment) |
I've tested against MRI 2.1.5 only. |
LGTM |
0.11.0 2015-11-29 ([changes](simplecov-ruby/simplecov@v0.10.0...v0.10.11)) ================= ## Enhancements * Added `SimpleCov.minimum_coverage_by_file` for per-file coverage thresholds. See [#392](simplecov-ruby/simplecov#392) (thanks @ptashman) * Added `track_files` configuration option to specify a glob to always include in coverage results, whether or not those files are required. See [#422](simplecov-ruby/simplecov#422) (thanks @hugopeixoto) * Speed up `root_filter` by an order of magnitude. See [#396](simplecov-ruby/simplecov#396) (thanks @raszi) ## Bugfixes * Fix warning about global variable `$ERROR_INFO`. See [#400](simplecov-ruby/simplecov#400) (thanks @amatsuda) * Actually recurse upward looking for `.simplecov`, as claimed by the documentation, rather than only the working directory. See [#423](simplecov-ruby/simplecov#423) (thanks @alexdowad)