Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add new Performance/SortReverse cop #130

Merged
merged 1 commit into from
Jun 8, 2020

Conversation

fatkodima
Copy link
Contributor

It is much faster to just use sort.reverse than explicitly specifying block with code to sort in reverse order.

# bad
array.sort { |a, b| b <=> a }

# good
array.sort.reverse

Benchmark

# frozen_string_literal: true

require 'bundler/inline'
require 'securerandom'

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

ARR10 = 10.times.map { rand(1000) }
ARR1000 = 1000.times.map { rand(1000) }
ARR1000000 = 1000000.times.map { rand(1000) }

ARR_HEXES = 100.times.map { SecureRandom.hex(13) }

puts "======== Hexes ========"

Benchmark.ips do |x|
  x.report("sort.reverse")   { ARR_HEXES.sort.reverse }
  x.report("sort { ... }")   { ARR_HEXES.sort { |a, b| b <=> a } }
  x.compare!
end

puts "======== 10 ========"

Benchmark.ips do |x|
  x.report("sort.reverse")   { ARR10.sort.reverse }
  x.report("sort { ... }")  { ARR10.sort { |a, b| b <=> a } }
  x.compare!
end

puts "======== 1000 ========"

Benchmark.ips do |x|
  x.report("sort.reverse")   { ARR1000.sort.reverse }
  x.report("sort { ... }")  { ARR1000.sort { |a, b| b <=> a } }
  x.compare!
end

puts "======== 1_000_000 ========"

Benchmark.ips do |x|
  x.report("sort.reverse")   { ARR1000000.sort.reverse }
  x.report("sort { ... }")  { ARR1000000.sort { |a, b| b <=> a } }
  x.compare!
end

Result

======== Hexes ========
Warming up --------------------------------------
        sort.reverse     8.289k i/100ms
        sort { ... }     1.697k i/100ms
Calculating -------------------------------------
        sort.reverse     78.920k (± 7.1%) i/s -    397.872k in   5.069228s
        sort { ... }     16.214k (± 4.9%) i/s -     81.456k in   5.036694s

Comparison:
        sort.reverse:    78920.2 i/s
        sort { ... }:    16213.6 i/s - 4.87x  (± 0.00) slower

======== 10 ========
Warming up --------------------------------------
        sort.reverse   126.334k i/100ms
        sort { ... }    38.479k i/100ms
Calculating -------------------------------------
        sort.reverse      1.160M (±11.6%) i/s -      5.811M in   5.087874s
        sort { ... }    390.459k (± 2.2%) i/s -      1.962M in   5.028543s

Comparison:
        sort.reverse:  1160136.0 i/s
        sort { ... }:   390458.9 i/s - 2.97x  (± 0.00) slower

======== 1000 ========
Warming up --------------------------------------
        sort.reverse   963.000  i/100ms
        sort { ... }   145.000  i/100ms
Calculating -------------------------------------
        sort.reverse      9.567k (± 6.7%) i/s -     48.150k in   5.059807s
        sort { ... }      1.441k (± 2.2%) i/s -      7.250k in   5.034868s

Comparison:
        sort.reverse:     9566.7 i/s
        sort { ... }:     1440.7 i/s - 6.64x  (± 0.00) slower

======== 1_000_000 ========
Warming up --------------------------------------
        sort.reverse     1.000  i/100ms
        sort { ... }     1.000  i/100ms
Calculating -------------------------------------
        sort.reverse      9.747  (± 0.0%) i/s -     49.000  in   5.030332s
        sort { ... }      1.461  (± 0.0%) i/s -      8.000  in   5.474954s

Comparison:
        sort.reverse:        9.7 i/s
        sort { ... }:        1.5 i/s - 6.67x  (± 0.00) slower

@fatkodima
Copy link
Contributor Author

fatkodima commented Jun 8, 2020

Updated.

Wdyt about renaming this cop to SortWithBlock and adding #132 functionality to this?
Or that should be better implemented as a separate cop?

@koic
Copy link
Member

koic commented Jun 8, 2020

Yeah. I think that they should be separated into different cops of CompareWithBlock, SortReverse, and RedundantSortBlock because they have different roles.

@koic koic merged commit bda0a63 into rubocop:master Jun 8, 2020
@koic
Copy link
Member

koic commented Jun 8, 2020

Thanks!

tas50 added a commit to chef/chef that referenced this pull request Aug 3, 2020
It's faster. See rubocop/rubocop-performance#130

Signed-off-by: Tim Smith <tsmith@chef.io>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants