[Proposal] Performance/SelectFirst

[ghiculescu]

We have built this cop internally, for identifying uses of .select {}.first or .select {}.count that could be written in a more performant way. My question is, would rubocop accept a PR to add this to the main gem (if I added tests etc) or is it too niche?

module RuboCop
  module Cop
    module TandaCustomCops
      # Don't call `.find_all { }.first`. Instead just use `.find { }`
      # The bad form iterates over every item in an array and runs the block on each.
      # The good form returns as soon as a match is found.
      class SelectFirst < Cop
        def_node_matcher :find_all_first_candidate?, <<-PATTERN
          (send (block $(send _ ${:find_all :select}) ...) ${:first} $...)
        PATTERN
        def_node_matcher :find_all_count_candidate?, <<-PATTERN
          (send (block $(send _ ${:find_all :select}) ...) ${:count} $...)
        PATTERN

        MSG_FIND = <<~MSG.freeze
          `.select { }.first` can be written as `.find { }`. This is faster because it returns as soon as a match is found.`
        MSG

        MSG_COUNT = <<~MSG.freeze
          `.select { }.count` can be written as `.count { }`. This is marginally faster.`
        MSG

        def on_send(node)
          add_offense(node, message: MSG_FIND) if find_all_first_candidate?(node)
          add_offense(node, message: MSG_COUNT) if find_all_count_candidate?(node)
        end
      end
    end
  end
end

[koic]

Can you show a benchmark compared to alternative methods?

[ghiculescu]

Obviously the difference scales with the size of the array.

require 'benchmark/ips'

array = 100_000.times.map { rand > 0.1 ? 0 : 1 } # 10% of the array should be 1, 90% should be 0

Benchmark.ips do |x|
  x.report("slow") { array.select(&:nonzero?).first }
  x.report("fast") { array.find(&:nonzero?) }

  x.compare!
end

=begin
Warming up --------------------------------------
                slow     3.000  i/100ms
                fast    44.899k i/100ms
Calculating -------------------------------------
                slow     35.648  (±11.2%) i/s -    177.000
                fast    579.702k (±14.2%) i/s -      2.829M

Comparison:
                fast:   579701.7 i/s
                slow:       35.6 i/s - 16262.05x slower
=end

But even with a much smaller array it's still significant:

require 'benchmark/ips'

array = 100.times.map { rand > 0.1 ? 0 : 1 } # 10% of the array should be 1, 90% should be 0

Benchmark.ips do |x|
  x.report("slow") { array.select(&:nonzero?).first }
  x.report("fast") { array.find(&:nonzero?) }

  x.compare!
end

=begin
Warming up --------------------------------------
                slow     3.172k i/100ms
                fast    29.762k i/100ms
Calculating -------------------------------------
                slow     33.738k (±13.5%) i/s -    168.116k
                fast    397.477k (±12.9%) i/s -      1.964M

Comparison:
                fast:   397477.5 i/s
                slow:    33737.8 i/s - 11.78x slower
=end

[ghiculescu]

This one is less exciting, it's just neater code IMO:

require 'benchmark/ips'

array = 100_000.times.map { rand > 0.1 ? 0 : 1 } # 10% of the array should be 1, 90% should be 0

Benchmark.ips do |x|
  x.report("slow") { array.select(&:nonzero?).count }
  x.report("fast") { array.count(&:nonzero?) }

  x.compare!
end

=begin
Warming up --------------------------------------
                slow     3.000  i/100ms
                fast     3.000  i/100ms
Calculating -------------------------------------
                slow     37.756  (± 2.6%) i/s -    189.000
                fast     35.935  (±11.1%) i/s -    180.000

Comparison:
                slow:       37.8 i/s
                fast:       35.9 i/s - same-ish: difference falls within error
=end

[koic]

Thank you for benchmarking with alternative methods. I agree to add this cop to performance cops.

I will convey some of points considered.

• This proposed cop name Performance/SelectFirst looks good to me.
• filter method has been added in Ruby 2.6. So filter method is also targeted when Ruby 2.6 or higher. https://ruby-doc.org/core-2.6.3/Array.html#method-i-filter
• It would be preferable for this cop to replace with find method by auto-correct. Please see "Why did I choose this fix out of the possible options?" at the bottom.
• Be careful not to cause false positives in Active Record's select method. For example, Model.select(fields).first should not be warned. It should not be warned when an argument is not a block. https://api.rubyonrails.org/classes/ActiveRecord/QueryMethods.html#method-i-select
• Same for count. Please replace select { ... }.count only if an argument is a block. Otherwise, a SQL changes.
• https://github.com/JuanitoFatas/fast-ruby#enumerabledetect-vs-enumerableselectfirst-code would be useful as a reference.

Why did I choose this fix out of the possible options?

I think that coding context will be maintained by replacing find_all { ... }.first with find method and replacing select { ... }.first to detect method respectively. However, it will not be possible to replace it, as it will cause an error in an Active Record model when select { ... }.first method will be auto-corrected detect method. The following are examples.

# Use `select { ... }.first`
Model.select { |model| model.attr == value }.first #=> Returns an AR model object

# Use `find { ... }`
Model.find { |model| model.attr == value } #=> Returns an AR model object

# Use `delect { ... }`
Model.delect { |model| model.attr == value } #=> NoMethodError
NoMethodError: undefined method `delect' for #<Class:0x007f81bd065ae0>
Did you mean?  delete
               select

So I considered that it should be replaced by find method.
It means that performance is prioritized over representation by API name when using this cop. Because this cop is a performance cops.

[koic]

It may be possible to further consider whether it is okay to include select { ... }.first and select { ... }.count in the same cop. Because it is named SelectFirst cop. It looked good to me, but there may be other appropriate cop name.

Cc @rubocop-hq/rubocop-core

[bquorning]

it will cause an error in an Active Record model

I think that was caused by a typo: delect != detect.

Both find and detect should be valid alternatives.

[koic]

Yeah, detect is correct, not delect. This is my typo.

[ghiculescu]

• filter method has been added in Ruby 2.6. So filter method is also targeted when Ruby 2.6 or higher. ruby-doc.org/core-2.6.3/Array.html#method-i-filter

Is there a recommended approach to having rules only apply to specific ruby versions?

[koic]

Please use target_ruby_version. The following production code and spec code will be helpful.

• https://github.com/rubocop-hq/rubocop/blob/v0.72.0/lib/rubocop/cop/layout/rescue_ensure_alignment.rb#L131
• https://github.com/rubocop-hq/rubocop/blob/v0.72.0/spec/rubocop/cop/layout/rescue_ensure_alignment_spec.rb#L301

[rrosenblum]

I think Performance/Detect and Performance/Count cover most if not all of this functionality. As mentioned in #69, I don't think these cops were being run for most people. There are likely some enhancements that can be made to each of the cops.

[fatkodima]

@ghiculescu Have you started work on this?

[ghiculescu]

Ahh, no I haven't. @fatkodima if you're interested please go ahead, otherwise I will try and get to it soon.