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

Add new Performance/ArraySemiInfiniteRangeSlice cop #175

Merged
merged 1 commit into from
Oct 31, 2020

Conversation

fatkodima
Copy link
Contributor

Inspired by fastruby/fast-ruby#181

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

@koic koic merged commit 4fd2d40 into rubocop:master Oct 31, 2020
@koic
Copy link
Member

koic commented Oct 31, 2020

Thanks!

@such
Copy link

such commented Nov 16, 2020

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
Copy link

basex commented Nov 17, 2020

This cop is not safe. Example:

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

@marcandre
Copy link
Contributor

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

@marcandre
Copy link
Contributor

marcandre commented Nov 23, 2020

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
Copy link

fuegas commented Nov 23, 2020

@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.

tejasbubane added a commit to tejasbubane/rubocop-performance that referenced this pull request Nov 23, 2020
This cop was created due to a mistake in microbenchmark
Refer rubocop#175 (comment)

Closes rubocop#197, rubocop#198
tejasbubane added a commit to tejasbubane/rubocop-performance that referenced this pull request Nov 23, 2020
This cop was created due to a mistake in microbenchmark
Refer rubocop#175 (comment)

Closes rubocop#197, rubocop#198
tejasbubane added a commit to tejasbubane/rubocop-performance that referenced this pull request Nov 23, 2020
This cop was created due to a mistake in microbenchmark
Refer rubocop#175 (comment)

Closes rubocop#197, rubocop#198
tejasbubane added a commit to tejasbubane/rubocop-performance that referenced this pull request Nov 24, 2020
This cop was created due to a mistake in microbenchmark
Refer rubocop#175 (comment)

Closes rubocop#197, rubocop#198
tejasbubane added a commit to tejasbubane/rubocop-performance that referenced this pull request Nov 24, 2020
This cop was created due to a mistake in microbenchmark
Refer rubocop#175 (comment)

Closes rubocop#197, rubocop#198
tejasbubane added a commit to tejasbubane/rubocop-performance that referenced this pull request Nov 24, 2020
This cop was created due to a mistake in microbenchmark
Refer rubocop#175 (comment)

Closes rubocop#197, rubocop#198
patrickm53 pushed a commit to patrickm53/performance-develop-rubyonrails that referenced this pull request Sep 23, 2022
This cop was created due to a mistake in microbenchmark
Refer rubocop/rubocop-performance#175 (comment)

Closes #197, #198
richardstewart0213 added a commit to richardstewart0213/performance-build-Performance-optimization-analysis- that referenced this pull request Nov 4, 2022
This cop was created due to a mistake in microbenchmark
Refer rubocop/rubocop-performance#175 (comment)

Closes #197, #198
Cute0110 added a commit to Cute0110/Rubocop-Performance that referenced this pull request Sep 28, 2023
This cop was created due to a mistake in microbenchmark
Refer rubocop/rubocop-performance#175 (comment)

Closes #197, #198
SerhiiMisiura added a commit to SerhiiMisiura/Rubocop-Performance that referenced this pull request Oct 5, 2023
This cop was created due to a mistake in microbenchmark
Refer rubocop/rubocop-performance#175 (comment)

Closes #197, #198
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 this pull request may close these issues.

6 participants