Add new Performance/RedundantFetchBlock cop

[fatkodima]

Related reference - https://github.com/JuanitoFatas/fast-ruby#hashfetch-with-argument-vs-hashfetch--block-code

When block for fetch just returns Numeric, Symbol, true, false, nil or constant it is shorter (and faster) to pass an argument instead of using a block.

# bad
hash.fetch(:key) { 5 }
hash.fetch(:key) { true }
hash.fetch(:key) { false }
hash.fetch(:key) { nil }
array.fetch(5) { :value }
ENV.fetch(:key) { VALUE }

# good
hash.fetch(:key, 5)
hash.fetch(:key, true)
hash.fetch(:key, false)
hash.fetch(:key, nil)
array.fetch(5, :value)
ENV.fetch(:key, VALUE)

When hash contains a key, performance is the same for argument vs block syntaxes, but when hash is missing a key - argument version is faster.

Benchmark

# frozen_string_literal: true

require 'bundler/inline'

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

hash_with_key = { key: 1 }
hash_without_key = { not_a_key: 1 }

def fast(hash)
  hash.fetch(:key, 1)
end

def slow(hash)
  hash.fetch(:key) { 1 }
end

Benchmark.ips do |x|
  x.report('fast hash_with_key') { fast(hash_with_key) }
  x.report('slow hash_with_key') { slow(hash_with_key) }
  x.compare!
end

Benchmark.ips do |x|
  x.report('fast hash_without_key') { fast(hash_without_key) }
  x.report('slow hash_without_key') { slow(hash_without_key) }
  x.compare!
end

Results

Warming up --------------------------------------
  fast hash_with_key     1.043M i/100ms
  slow hash_with_key   996.735k i/100ms
Calculating -------------------------------------
  fast hash_with_key     10.292M (± 2.9%) i/s -     52.149M in   5.072099s
  slow hash_with_key      9.940M (± 1.5%) i/s -     49.837M in   5.015103s

Comparison:
  fast hash_with_key: 10291722.7 i/s
  slow hash_with_key:  9939687.7 i/s - same-ish: difference falls within error

Warming up --------------------------------------
fast hash_without_key
                         1.017M i/100ms
slow hash_without_key
                       659.264k i/100ms
Calculating -------------------------------------
fast hash_without_key
                         10.344M (± 0.5%) i/s -     51.892M in   5.016881s
slow hash_without_key
                          6.663M (± 1.6%) i/s -     33.622M in   5.047510s

Comparison:
fast hash_without_key: 10343873.0 i/s
slow hash_without_key:  6663049.0 i/s - 1.55x  (± 0.00) slower

[marcandre]

Good PR.

1. Could you please add true, false (even nil although that would be weird), rationals and complex numbers too, constant Ranges. I think that immutable_litteral? or basic_litteral? should be a good base.
2. Could you please add a test for symbols with interpolation (no offense)
3. I feel constant strings (without interpolation) should also be passed as arguments; if I'm not mistaken ruby runs with shared frozen string constants so there wouldn't be a time cost.
4. Not sure what to do with constants; they can not be completely safely exchanged though. E.g.:

h = {a: 1}
h.fetch(:a) { NotDefinedYet } # => 1
NotDefinedYet = 42
h.fetch(:b) { NotDefinedYet } # => NotDefinedYet

Autocorrecting the above will fail. That example is dubious, but some constants are autoloaded, etc.

You could mark this cop as Safe: false, or introduce a setting... Anyone?

[fatkodima]

@marcandre Thanks for review!

1. Oops, I completely forgot about true/false/nil 😄
   Seems like rationals/complex/ranges cannot be used here, as they allocate new instances every time they are used as argument for fetch and it is faster to allocate them in a block. Local benchmarks proved that.
2. added
3. same as for point 1
4. added a setting (not sure about a name, maybe there is a better one)

[marcandre]

Right, obviously you're right about Ranges & Complex, my bad.

Floats require allocation only on 32 bit machines so indeed your mileage may vary.

3. Which Ruby are you testing on? I get

Comparison:
  fast hash_with_key:  9353669.7 i/s
  slow hash_with_key:  8974184.9 i/s - same-ish: difference falls within error

[fatkodima]

3. Yeah, when the key is present in the hash, there is no difference in performance.
   But there is a difference when the key is missing:

Comparison:
fast hash_without_key: 10288181.0 i/s
slow hash_without_key:  6811963.9 i/s - 1.51x  (± 0.00) slower

So seems like the case when block returns a String isn't worth considering.

Tested on ruby 2.6.5p114

[marcandre]

But there is a difference when the key is missing

Yes, that's the point, right, the block form will always best equal or slower when the key is missing. It's thus best to avoid the block form for strings.

[fatkodima]

@marcandre Yes, sure, you are correct! I mixed up fast and slow parts in my benchmark and didn't notice this. I wondered after your first comment, why block with string is faster than string as argument when strings are frozen but trusted the benchmark.

Updated to check for strings.

Thanks for patience! 😄

[fatkodima]

1. Could you please add true, false (even nil although that would be weird), rationals and complex numbers too, constant Ranges. I think that immutable_litteral? or basic_litteral? should be a good base.

You were correct regarding Rationals and Complexs, I was not testing properly and forgot about that there are literals for them.

Updated with usage of basic_literal? + const_type?.

[bbatsov]

That's one of those cases that sit between style and performance. For me it's a given that you shouldn't be using a block unless you have to compute something, regardless of the performance implications.

[fatkodima]

@bbatsov So, wdyt, is this better to be moved to Style in main repo?

[bbatsov]

I'm on the fence. @rubocop-hq/rubocop-core what do you think?

[koic]

Yeah, I think this cop can move to Style department.

[fatkodima]

Transferred rubocop/rubocop#8147