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 grep/grep_v #8327

Closed
marcandre opened this issue Jul 13, 2020 · 5 comments · Fixed by #10082
Closed

Prefer grep/grep_v #8327

marcandre opened this issue Jul 13, 2020 · 5 comments · Fixed by #10082

Comments

@marcandre
Copy link
Contributor

The following code has 3 anti-patterns:

def self.tokens(pattern)
   pattern.scan(TOKEN).reject { |token| token =~ /\A#{SEPARATORS}\Z/ }
end

This issue is to identify that enum.reject { |x| x.match? regexp } should be enum.grep_v(regexp). Same with select vs grep.

See: rubocop/rubocop-ast#65

@fatkodima
Copy link
Contributor

I have run a quick benchmark. Looks like grep/grep_v are not so nice, while simplifying code a bit.

Benchmark code
# frozen_string_literal: true

require 'bundler/inline'

gemfile(true) do
  gem 'benchmark-ips'
  gem 'benchmark-memory'
end

arr = ['foobar', 'foobaz', 'bazquux']

REGEXP = /foo/

def select_match(arr)
  arr.select { |e| e.match?(REGEXP) }
end

def grep(arr)
  arr.grep(REGEXP)
end

Benchmark.ips do |x|
  x.report("select.match?")  { select_match(arr) }
  x.report("grep")           { grep(arr) }
  x.compare!
end

puts "********* MEMORY *********"

Benchmark.memory do |x|
  x.report("select.match?")  { select_match(arr) }
  x.report("grep")           { grep(arr) }
  x.compare!
end
Warming up --------------------------------------
       select.match?   159.869k i/100ms
                grep    56.192k i/100ms
Calculating -------------------------------------
       select.match?      1.561M (± 6.5%) i/s -      7.834M in   5.044218s
                grep    565.201k (± 1.6%) i/s -      2.866M in   5.071747s

Comparison:
       select.match?:  1561061.6 i/s
                grep:   565201.5 i/s - 2.76x  (± 0.00) slower

********* MEMORY *********
Calculating -------------------------------------
       select.match?    40.000  memsize (     0.000  retained)
                         1.000  objects (     0.000  retained)
                         0.000  strings (     0.000  retained)
                grep   208.000  memsize (     0.000  retained)
                         2.000  objects (     0.000  retained)
                         0.000  strings (     0.000  retained)

Comparison:
       select.match?:         40 allocated
                grep:        208 allocated - 5.20x more

@marcandre
Copy link
Contributor Author

Kudos for checking that @fatkodima I would not have thought so. It makes some sense, because grep calls === which is =~ for Regexp. This should be optimized by Ruby... I'll write an issue (unless you'd prefer doing that yourself)?

@fatkodima
Copy link
Contributor

Please, do 👍

@fatkodima
Copy link
Contributor

@marcandre
So do you think it is worth implementing, due to https://bugs.ruby-lang.org/issues/17030, but making it disabled by default?
grep/grep_v can be also extended by Enumerable#{any?,all?,none?,one?} with regexp argument.

@marcandre
Copy link
Contributor Author

Sure. What would be worth implementing is fixing 17030 :-) IIUC, Matz is ok with optimizing this

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 a pull request may close this issue.

2 participants