rubocop / rubocop-performance

An extension of RuboCop focused on code performance checks.
https://docs.rubocop.org/rubocop-performance
MIT License
683 stars 81 forks source link

Only enable cops that actually improve performance #329

Open wteuber opened 1 year ago

wteuber commented 1 year ago

Is your feature request related to a problem? Please describe.

I found some cases in which performance cops autocorrects slowed down my code depending or the ruby version and system architecture.

details... `BigDecimalWithNumericArgument.rb` ```ruby require "benchmark/ips" require "bigdecimal" require "bigdecimal/util" def strategy1 BigDecimal('1', 2) '4'.to_d '4.5'.to_d end def strategy2 BigDecimal(1, 2) 4.to_d 4.5.to_d end Benchmark.ips do |x| x.report("string argument to BigDecimal") { strategy1 } x.report("numeric argument to BigDecimal") { strategy2 } x.compare! end ``` ___ ##### `ruby 3.1.2p20 (2022-04-12 revision 4491bb740a) [arm64-darwin21]` ``` Warming up -------------------------------------- string argument to BigDecimal 141.699k i/100ms numeric argument to BigDecimal 101.314k i/100ms Calculating ------------------------------------- string argument to BigDecimal 1.424M (± 1.0%) i/s - 7.227M in 5.074380s numeric argument to BigDecimal 998.446k (± 6.6%) i/s - 4.964M in 5.008903s Comparison: string argument to BigDecimal: 1424285.0 i/s numeric argument to BigDecimal: 998446.2 i/s - 1.43x (± 0.00) slower ``` ___ ##### `ruby 3.1.2p20 (2022-04-12 revision 4491bb740a) [x86_64-linux]` ``` Warming up -------------------------------------- string argument to BigDecimal 65.879k i/100ms numeric argument to BigDecimal 87.335k i/100ms Calculating ------------------------------------- string argument to BigDecimal 664.552k (± 2.6%) i/s - 3.360M in 5.059361s numeric argument to BigDecimal 859.141k (± 5.0%) i/s - 4.367M in 5.097011s Comparison: numeric argument to BigDecimal: 859140.5 i/s string argument to BigDecimal: 664552.3 i/s - 1.29x (± 0.00) slower ```

Describe the solution you'd like

Ideally, rubocop-performance comes with an executable configuration that identifies cops that actually improve performance to either guide the developer by printing performance characteristics for each cop and/or generating a section to be pasted in a projects .rubocop.yml. I'm aware that this is no trivial task, but that's a solution I'd like.

Describe alternatives you've considered

Adding test that verify performance improvements might cause builds to fail depending on the environment, so this is an option that can be ruled out.

For now, before enabling a rubocop-performance cop, I manually verify that the cop actually improves performance in the target environment in the style of fast-ruby or fasterer using examples from rubocop docs.

Additional context

Instead of simple disabling rubocop-performance cops that slow down my code, I'd rather "invert" them, i.e. autocorrecting from what was supposed to be the result to what was there originally, is there an easier way than writing a completely new cop for this? And if no, could the new cop somehow be derived from the existing cop?

Thank you very much!

fatkodima commented 1 year ago

Can you share concrete autocorrected examples which slowed down your code? So we could probably revise them.

Ah, found your BigDecimal example. Besides that?

wteuber commented 1 year ago

Hi @fatkodima,

thanks for your consideration. We haven't checked all of them, but have come across another performance cop that seems to lower performance in ruby 3.1.2p20 (both arm64-darwin21 and x86_64-linux): BlockGivenWithExplicitBlock.

For microbenchmarking, we used:

# frozen_string_literal: true

require 'benchmark/ips'

def strategy1(&block)
  yield(10) if block
end

def strategy2
  yield(10) if block_given?
end

Benchmark.ips do |x|
  x.report('use if block') { strategy1 { |a| a * 2 } }
  x.report('use if block_given?') { strategy2 { |a| a * 2 } }
  x.compare!
end

Here are the results:

ruby 3.1.2p20 (2022-04-12 revision 4491bb740a) [arm64-darwin21]
Warming up --------------------------------------
        use if block   403.070k i/100ms
 use if block_given?   846.906k i/100ms
Calculating -------------------------------------
        use if block      3.994M (± 1.5%) i/s -     20.154M in   5.047463s
 use if block_given?      8.509M (± 0.8%) i/s -     43.192M in   5.076179s

Comparison:
 use if block_given?:  8509301.7 i/s
        use if block:  3993706.9 i/s - 2.13x  (± 0.00) slower
ruby 3.1.2p20 (2022-04-12 revision 4491bb740a) [x86_64-linux]
Warming up --------------------------------------
        use if block   229.873k i/100ms
 use if block_given?   519.562k i/100ms
Calculating -------------------------------------
        use if block      2.207M (±13.4%) i/s -     10.804M in   5.085042s
 use if block_given?      4.283M (±32.2%) i/s -     19.224M in   5.128686s

Comparison:
 use if block_given?:  4282557.5 i/s
        use if block:  2207476.5 i/s - 1.94x  (± 0.00) slower

I'm happy to add new insights here (or in places more convenient for you).

wteuber commented 1 year ago

We benchmarked all currently available RuboCop Performance cops and found three cops that actually worsen performance in ruby 3.2.0.

env cops
ruby_3.2.0, x86_64_Linux Performance/ArraySemiInfiniteRangeSlice
ruby_3.2.0, arm64_Darwin Performance/BigDecimalWithNumericArgument
Performance/IoReadlines

We use the following benchmarks:

# ArraySemiInfiniteRangeSlice

require 'benchmark/ips'

ARRAY = (1..10).to_a

def strategy1
  ARRAY[..2]
  ARRAY[...2]
end

def strategy2
  ARRAY.take(3)
  ARRAY.take(2)
end

Benchmark.ips do |x|
  x.report('[...]') { strategy1 { |a| a * 2 } }
  x.report('take') { strategy2 { |a| a * 2 } }
  x.compare!
end
# ArraySemiInfiniteRangeSlice

require 'benchmark/ips'

ARRAY = (1..10).to_a

def strategy1
  ARRAY.slice(..2)
end

def strategy2
  ARRAY.take(3)
end

Benchmark.ips do |x|
  x.report('slice') { strategy1 { |a| a * 2 } }
  x.report('take') { strategy2 { |a| a * 2 } }
  x.compare!
end
# ArraySemiInfiniteRangeSlice

require 'benchmark/ips'

ARRAY = (1..10).to_a

def strategy1
  ARRAY[2..]
  ARRAY[2...]
end

def strategy2
  ARRAY.drop(2)
  ARRAY.drop(2)
end

Benchmark.ips do |x|
  x.report('[...]') { strategy1 { |a| a * 2 } }
  x.report('drop') { strategy2 { |a| a * 2 } }
  x.compare!
end
# BigDecimalWithNumericArgument

require 'benchmark/ips'
require 'bigdecimal'
require 'bigdecimal/util'

def strategy1
  BigDecimal('1', 2)
  '4'.to_d
  '4.5'.to_d
end

def strategy2
  BigDecimal(1, 2)
  4.to_d
  4.5.to_d
end

Benchmark.ips do |x|
  x.report("string argument to BigDecimal") { strategy1 }
  x.report("numeric argument to BigDecimal") { strategy2 }
  x.compare!
end
# IoReadlines

require 'benchmark/ips'

def strategy1
  File.readlines(__FILE__).each(&:reverse)
end

def strategy2
  File.open(__FILE__, 'r').each_line(&:reverse)
end

Benchmark.ips do |x|
  x.report("readlines each") { strategy1 }
  x.report("each_line") { strategy2 }
  x.compare!
end