Add new `Performance/ArraySemiInfiniteRangeSlice` cop #175

fatkodima · 2020-09-29T13:05:33Z

Benchmarks

# frozen_string_literal: true

require 'bundler/inline'

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

ARRAY = (1..100).to_a

Benchmark.ips do |x|
  x.report('Array#take(3)')   { ARRAY.take(3) }
  x.report('Array#[..2]')     { ARRAY[..2] }
  x.compare!
end

Benchmark.ips do |x|
  x.report('Array#drop(90)')  { ARRAY.drop(90) }
  x.report('Array#[90..]')    { ARRAY[90..] }
  x.compare!
end

Results

Warming up --------------------------------------
       Array#take(3)   836.792k i/100ms
         Array#[..2]   491.866k i/100ms
Calculating -------------------------------------
       Array#take(3)      8.578M (± 1.2%) i/s -     43.513M in   5.073484s
         Array#[..2]      4.961M (± 1.1%) i/s -     25.085M in   5.056928s

Comparison:
       Array#take(3):  8577744.5 i/s
         Array#[..2]:  4961147.7 i/s - 1.73x  (± 0.00) slower

Warming up --------------------------------------
      Array#drop(90)   811.713k i/100ms
        Array#[90..]   472.930k i/100ms
Calculating -------------------------------------
      Array#drop(90)      8.086M (± 3.5%) i/s -     40.586M in   5.026753s
        Array#[90..]      4.726M (± 2.2%) i/s -     23.646M in   5.005881s

Comparison:
      Array#drop(90):  8086260.0 i/s
        Array#[90..]:  4726170.7 i/s - 1.71x  (± 0.00) slower

spec/rubocop/cop/performance/array_semi_infinite_range_slice_spec.rb

koic · 2020-10-31T08:51:41Z

Thanks!

such · 2020-11-16T18:22:42Z

String#drop and String#take don't exist but we can use string[2..0] and the Cop will complain (and auto-correct if asked to). Is there something else we can use or do we have to disable the Cop in that case?

basex · 2020-11-17T21:06:07Z

This cop is not safe. Example:

".ext"[1..]
 => "ext" 
".ext".drop(1)
NoMethodError (undefined method `drop' for ".ext":String)

marcandre · 2020-11-23T01:54:06Z

Didn't anyone file a bug report on MRI for this? The solution here is not to optimize Ruby code but MRI...

marcandre · 2020-11-23T02:08:16Z

Actually, this is just an error in the benchmarking. The creation of the range ..2 and 90.. is what is taking extra time. This has nothing to do with semi-infinite ranges at all, or drop/rake being quicker. Note: to avoid creating a Range, one can use Array#[index, length]. In general though, Array#[range] often is the clearest.

fuegas · 2020-11-23T10:13:09Z

@marcandre I'm with you on this, a slight alteration of the example code gives the following results:

Benchmarks

# frozen_string_literal: true

require 'bundler/inline'

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

ARRAY = (1..100).to_a

range_take_3 = (..2).freeze
range_drop_90 = (90..).freeze

Benchmark.ips do |x|
  x.report('Array#take(3)')    { ARRAY.take(3) }
  x.report('Array#[..2]')      { ARRAY[..2] }
  x.report('var Array#[..2]')  { ARRAY[range_take_3] }
  x.compare!
end

Benchmark.ips do |x|
  x.report('Array#drop(90)')   { ARRAY.drop(90) }
  x.report('Array#[90..]')     { ARRAY[90..] }
  x.report('var Array#[90..]') { ARRAY[range_drop_90] }
  x.compare!
end

Results

Warming up --------------------------------------
       Array#take(3)     1.316M i/100ms
         Array#[..2]   691.734k i/100ms
     var Array#[..2]     1.283M i/100ms
Calculating -------------------------------------
       Array#take(3)     12.827M (± 3.2%) i/s -     64.483M in   5.032604s
         Array#[..2]      7.657M (± 4.1%) i/s -     38.737M in   5.068075s
     var Array#[..2]     12.934M (± 3.7%) i/s -     65.452M in   5.068425s

Comparison:
     var Array#[..2]: 12933514.2 i/s
       Array#take(3): 12826906.7 i/s - same-ish: difference falls within error
         Array#[..2]:  7657214.0 i/s - 1.69x  (± 0.00) slower

Warming up --------------------------------------
      Array#drop(90)     1.232M i/100ms
        Array#[90..]   652.314k i/100ms
    var Array#[90..]     1.208M i/100ms
Calculating -------------------------------------
      Array#drop(90)     11.575M (± 2.9%) i/s -     57.887M in   5.005320s
        Array#[90..]      7.051M (± 3.1%) i/s -     35.225M in   5.001099s
    var Array#[90..]     11.879M (± 3.6%) i/s -     60.411M in   5.092881s

Comparison:
    var Array#[90..]: 11878564.5 i/s
      Array#drop(90): 11574983.6 i/s - same-ish: difference falls within error
        Array#[90..]:  7050724.0 i/s - 1.68x  (± 0.00) slower

However, Rubocop does not seem to detect the range usage when you're using variables...

tmp/range.rb:16:34: C: Performance/ArraySemiInfiniteRangeSlice: Use take instead of [] with semi-infinite range.
  x.report('Array#[..2]')      { ARRAY[..2] }
                                 ^^^^^^^^^^
tmp/range.rb:23:34: C: Performance/ArraySemiInfiniteRangeSlice: Use drop instead of [] with semi-infinite range.
  x.report('Array#[90..]')     { ARRAY[90..] }
                                 ^^^^^^^^^^^

So as @marcandre said, the creation of a range is the factor that slows the benchmark down, not the usage of a Range to get a slice of an Array.

Also, take and drop do not work on a String which gives false positives right now as @basex has already written.

This cop was created due to a mistake in microbenchmark Refer rubocop#175 (comment) Closes rubocop#197, rubocop#198

This cop was created due to a mistake in microbenchmark Refer rubocop/rubocop-performance#175 (comment) Closes #197, #198

koic reviewed Oct 30, 2020

View reviewed changes

spec/rubocop/cop/performance/array_semi_infinite_range_slice_spec.rb Outdated Show resolved Hide resolved

koic reviewed Oct 30, 2020

View reviewed changes

spec/rubocop/cop/performance/array_semi_infinite_range_slice_spec.rb Outdated Show resolved Hide resolved

Add new Performance/ArraySemiInfiniteRangeSlice cop

d40868d

fatkodima force-pushed the array_endless_range_slice branch from 844c097 to d40868d Compare October 30, 2020 09:35

koic approved these changes Oct 30, 2020

View reviewed changes

koic merged commit 4fd2d40 into rubocop:master Oct 31, 2020

sankichi92 mentioned this pull request Nov 20, 2020

Bump rubocop-performance from 1.8.1 to 1.9.0 sankichi92/LiveLog#1082

Closed

marcandre mentioned this pull request Nov 23, 2020

Retire Performance/ArraySemiInfiniteRangeSlice cop #198

Open

parkerfinch mentioned this pull request Nov 23, 2020

Compare Array#drop with Array#[] fastruby/fast-ruby#181

Closed

jmkoni mentioned this pull request Nov 30, 2020

updates to rubocop 1.4.2 and rubocop-performance 1.9.1 standardrb/standard#225

Merged

dependabot bot mentioned this pull request Mar 8, 2021

Bump standard from 0.6.0 to 1.0.4 AlexWayfer/rubyapi#201

Closed

mattt mentioned this pull request Sep 5, 2022

Add RuboCop Performance dependabot/dependabot-core#5586

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new `Performance/ArraySemiInfiniteRangeSlice` cop #175

Add new `Performance/ArraySemiInfiniteRangeSlice` cop #175

fatkodima commented Sep 29, 2020

koic commented Oct 31, 2020

such commented Nov 16, 2020

basex commented Nov 17, 2020

marcandre commented Nov 23, 2020

marcandre commented Nov 23, 2020 •

edited

Loading

fuegas commented Nov 23, 2020

Add new Performance/ArraySemiInfiniteRangeSlice cop #175

Add new Performance/ArraySemiInfiniteRangeSlice cop #175

Conversation

fatkodima commented Sep 29, 2020

Benchmarks

Results

koic commented Oct 31, 2020

such commented Nov 16, 2020

basex commented Nov 17, 2020

marcandre commented Nov 23, 2020

marcandre commented Nov 23, 2020 • edited Loading

fuegas commented Nov 23, 2020

Benchmarks

Results

Add new `Performance/ArraySemiInfiniteRangeSlice` cop #175

Add new `Performance/ArraySemiInfiniteRangeSlice` cop #175

marcandre commented Nov 23, 2020 •

edited

Loading