Add new `ZipForArrayWrapping` cop #462

corsonknowles · 2024-09-05T00:18:55Z

Performance Cop for the more efficient way to generate an Array of Arrays.

Performs 40-90% faster than.map to iteratively wrap array contents.
Performs 5 - 55% faster on converting a range to an array of arrays with .map, depending on size.
Performs faster than mutating an array into an array of arrays in place with .map!
Performs faster than using each_with_object

This optimization is particularly helpful in performance sensitive paths or in large scale applications that need to generate arrays of arrays at scale. For example, leveraging the bulk enqueuing feature in Sidekiq requires an array of arrays, and is by definition used at scale.

A performance gain is always present for all sizes of arrays and ranges. The gain is smallest with small ranges, but still significant for small arrays.

This is not a style cop, but it is poetic that the more performant approach also has simpler and shorter syntax.

.zip has been intentionally optimized in Ruby. This has been discussed publicly since at least 2012:

https://stackoverflow.com/a/5254764

Official .zip documentation:

Source code for .zip:

This performance cop isn't just an announcement to use .zip in the spirit of appreciating Ruby's great features, it is also a useful and necessary tool to leverage Rubocop to clean up and add rigor in large Ruby code bases. Rubocop is a much better approach than pattern matching for clean up at scale here, and it comes with the added benefit of proactive user feedback as additions are made to the code base going forward.

For Arrays with 1000 entries, a common size for bulk operations, this performs 70% faster:

array = (1..1000).to_a
n = 100000 # Number of iterations
 Benchmark.bmbm(10) do |x|
  x.report(".zip:") do
    n.times do
      array.zip
    end
  end
  x.report(".map { |id| [id] }:") do
    n.times do
      array.map { |id| [id] }
    end
  end
end
Rehearsal -------------------------------------------------------
.zip:                 2.371856   0.037964   2.409820 (  2.410515)
.map { |id| [id] }:   4.102925   0.058121   4.161046 (  4.201261)
---------------------------------------------- total: 6.570866sec

                          user     system      total        real
.zip:                 2.384201   0.041635   2.425836 (  2.447048)
.map { |id| [id] }:   4.112258   0.046967   4.159225 (  4.164115)

Here it is at 70% faster, using benchmark-ips

irb(main):094* array = (1..1000).to_a; Benchmark.ips do |x|
irb(main):095*    x.report(".zip:") do
irb(main):096*
irb(main):097*        array.zip
irb(main):098*
irb(main):099*    end
irb(main):100*    x.report(".map { |id| [id] }:") do
irb(main):101*
irb(main):102*        array.map { |id| [id] }
irb(main):103*
irb(main):104*    end
irb(main):105>  end
ruby 3.3.0 (2023-12-25 revision 5124f9ac75) [arm64-darwin23]
Warming up --------------------------------------
               .zip:     4.019k i/100ms
 .map { |id| [id] }:     2.404k i/100ms
Calculating -------------------------------------
               .zip:     41.193k (± 1.4%) i/s -    208.988k in   5.074349s
 .map { |id| [id] }:     24.277k (± 2.8%) i/s -    122.604k in   5.054623s

50% faster for small arrays.

irb(main):001> require 'benchmark/ips';

irb(main):055* array = [1,2,3,4,5]; Benchmark.ips do |x|
irb(main):056*    x.report(".zip:") do
irb(main):057*      n.times do
irb(main):058*        array.zip
irb(main):059*      end
irb(main):060*    end
irb(main):061*    x.report(".map { |id| [id] }:") do
irb(main):062*      n.times do
irb(main):063*        array.map { |id| [id] }
irb(main):064*      end
irb(main):065*    end
irb(main):066>  end
ruby 3.3.0 (2023-12-25 revision 5124f9ac75) [arm64-darwin23]
Warming up --------------------------------------
               .zip:     5.000 i/100ms
 .map { |id| [id] }:     3.000 i/100ms
Calculating -------------------------------------
               .zip:     59.311 (± 1.7%) i/s -    300.000 in   5.060765s
 .map { |id| [id] }:     38.599 (± 2.6%) i/s -    195.000 in   5.058584s

Here's additional data for both large and small arrays and ranges:

range = (11_000..20_000)
 n = 100000 # Number of iterations
 Benchmark.bmbm(10) do |x|
   x.report(".zip:") do
     n.times do
       range.zip
     end
   end
   x.report(".map { |id| [id] }:") do
     n.times do
       range.map { |id| [id] }
     end
   end
 end
Rehearsal -------------------------------------------------------
.zip:                33.859854   0.677726  34.537580 ( 36.971688)
.map { |id| [id] }:  47.099024   0.728071  47.827095 ( 50.111365)
--------------------------------------------- total: 82.364675sec

                          user     system      total        real
.zip:                34.482279   0.550758  35.033037 ( 35.213391)
.map { |id| [id] }:  47.477530   0.737015  48.214545 ( 51.073250)

  
array = (11_000..20_000).to_a
 n = 100000 # Number of iterations
 Benchmark.bmbm(10) do |x|
   x.report(".zip:") do
     n.times do
       array.zip
     end
   end
   x.report(".map { |id| [id] }:") do
     n.times do
       array.map { |id| [id] }
     end
   end
 end
Rehearsal -------------------------------------------------------
.zip:                24.430554   0.833668  25.264222 ( 28.398712)
.map { |id| [id] }:  39.077948   0.631763  39.709711 ( 40.032396)
--------------------------------------------- total: 64.973933sec

                          user     system      total        real
.zip:                24.047782   0.655816  24.703598 ( 26.524327)
.map { |id| [id] }:  39.049433   0.615901  39.665334 ( 39.754370)

  
array = [1,2,3]
 n = 100000 # Number of iterations
 Benchmark.bmbm(10) do |x|
   x.report(".zip:") do
     n.times do
       array.zip
     end
   end
   x.report(".map { |id| [id] }:") do
     n.times do
       array.map { |id| [id] }
     end
   end
 end
Rehearsal -------------------------------------------------------
.zip:                 0.028292   0.012210   0.040502 (  0.042233)
.map { |id| [id] }:   0.020826   0.016060   0.036886 (  0.042100)
---------------------------------------------- total: 0.077388sec

                          user     system      total        real
.zip:                 0.007976   0.000023   0.007999 (  0.008004)
.map { |id| [id] }:   0.013670   0.000076   0.013746 (  0.013770)

  
range = (1..3)
 n = 100000 # Number of iterations
 Benchmark.bmbm(10) do |x|
   x.report(".zip:") do
     n.times do
       range.zip
     end
   end
   x.report(".map { |id| [id] }:") do
     n.times do
       range.map { |id| [id] }
     end
   end
 end
Rehearsal -------------------------------------------------------
.zip:                 0.046591   0.024528   0.071119 (  0.076043)
.map { |id| [id] }:   0.050247   0.013530   0.063777 (  0.063886)
---------------------------------------------- total: 0.134896sec

                          user     system      total        real
.zip:                 0.026723   0.000065   0.026788 (  0.026885)
.map { |id| [id] }:   0.029150   0.000640   0.029790 (  0.029885)

Before submitting the PR make sure the following are checked:

The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
Wrote good commit messages.
Commit message starts with [Fix #issue-number] (if the related issue exists).
Feature branch is up-to-date with master (if not - rebase it).
Squashed related commits together.
Added tests.
Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.

corsonknowles · 2024-09-09T22:48:25Z

Some of the gain here comes from simply not allocating the block. But I think most of it comes from the lower level conversion of the array into an array of arrays.

corsonknowles · 2024-09-09T22:50:20Z

Dear @koic and @Earlopain,
Are there any questions I can answer for you or modifications I can provide?

Thank you so much for your consideration!

Earlopain

This largely makes sense to me. Never really thought about using zip without arguments, that's a nice idea.

I think this pattern is relatively common in rails where they need to handle composite primary keys. I'll edit my findings in later if I decide to check. Edit: Nope, I was thinking about wrapping a single value in an array like [id]

Tip: use benchmark-ips for future benchmarks. The output is much more legible.

changelog/new_merge_pull_request_459_from.md

config/default.yml

lib/rubocop/cop/performance/use_zip_to_wrap_array_contents.rb

spec/rubocop/cop/performance/use_zip_to_wrap_array_contents_spec.rb

corsonknowles · 2024-09-14T20:06:54Z

I really appreciate the thoughtful comments @Earlopain

Fully revised and ready for review.

I had run the original version on 50,000 files. I added 2 more tests and diversified the input slightly to get confident in the new on_send approach. I predict this should be safe to merge now.

corsonknowles · 2024-09-14T20:30:59Z

Thanks for the benchmark-ips tip. I added some data using it for large and small arrays.

Earlopain

Almost done from my side, just a few remaining things.

Can you also add a test for map and map {}? These are common places for errors in the past. I think it works fine in your case right now

lib/rubocop/cop/performance/use_zip_to_wrap_array_contents.rb

spec/rubocop/cop/performance/use_zip_to_wrap_array_contents_spec.rb

lib/rubocop/cop/performance/use_zip_to_wrap_array_contents.rb

technicalpickles · 2024-09-15T15:50:36Z

First off, this will be a nice improvement for our code base, as we often use and recommend the map {|id| [id]} for Sidekiq.perform_bulk. Disclaimer: work with @corsonknowles 😁

I ran some of my own benchmarks based on @corsonknowles's in the body against small (4), medium (1000) and big (50k) arrays using benchmark-ips, and also added benchmark-memory for good measure. High level, we have:

small: zip is 1.45x faster
medium: zip is 1.74x faster
big: zip is 1.93x faster

I'm kinda surprised, but memory usage ends up the same which I'm not quite sure how to explain yet 😅

Here is the benchmark:

require 'benchmark/ips'
require 'benchmark/memory'

arrays = {
  small: (1..4).to_a,
  medium: (1..1000).to_a,
  big: (1..50000).to_a,
}

arrays.each do |size, array|
  puts "=== #{size} ==="
  Benchmark.ips do |x|
    x.report(".map { |id| [id] }:") do
      array.map { |id| [id] }
    end

    x.report(".zip:") do
      array.zip
    end

    x.compare! order: :baseline
  end

  Benchmark.memory do |x|
    x.report(".map { |id| [id] }:") do
      array.map { |id| [id] }
    end

    x.report(".zip:") do
      array.zip
    end
  end
end

And the results:

=== small ===
ruby 3.3.2 (2024-05-30 revision e5a195edf6) [arm64-darwin23]
Warming up --------------------------------------
 .map { |id| [id] }:   405.112k i/100ms
               .zip:   590.068k i/100ms
Calculating -------------------------------------
 .map { |id| [id] }:      4.029M (± 1.1%) i/s  (248.17 ns/i) -     20.256M in   5.027463s
               .zip:      5.846M (± 6.7%) i/s  (171.06 ns/i) -     29.503M in   5.085950s

Comparison:
 .map { |id| [id] }::  4029499.2 i/s
               .zip::  5845845.8 i/s - 1.45x  faster

Calculating -------------------------------------
 .map { |id| [id] }:   240.000  memsize (     0.000  retained)
                         5.000  objects (     0.000  retained)
                         0.000  strings (     0.000  retained)
               .zip:   240.000  memsize (     0.000  retained)
                         5.000  objects (     0.000  retained)
                         0.000  strings (     0.000  retained)
=== medium ===
ruby 3.3.2 (2024-05-30 revision e5a195edf6) [arm64-darwin23]
Warming up --------------------------------------
 .map { |id| [id] }:     2.042k i/100ms
               .zip:     3.533k i/100ms
Calculating -------------------------------------
 .map { |id| [id] }:     20.169k (± 1.7%) i/s   (49.58 μs/i) -    102.100k in   5.063731s
               .zip:     35.106k (± 1.6%) i/s   (28.48 μs/i) -    176.650k in   5.033169s

Comparison:
 .map { |id| [id] }::    20169.1 i/s
               .zip::    35106.3 i/s - 1.74x  faster

Calculating -------------------------------------
 .map { |id| [id] }:    48.040k memsize (     0.000  retained)
                         1.001k objects (     0.000  retained)
                         0.000  strings (     0.000  retained)
               .zip:    48.040k memsize (     0.000  retained)
                         1.001k objects (     0.000  retained)
                         0.000  strings (     0.000  retained)
=== big ===
ruby 3.3.2 (2024-05-30 revision e5a195edf6) [arm64-darwin23]
Warming up --------------------------------------
 .map { |id| [id] }:    44.000 i/100ms
               .zip:    64.000 i/100ms
Calculating -------------------------------------
 .map { |id| [id] }:    425.048 (± 0.7%) i/s    (2.35 ms/i) -      2.156k in   5.072641s
               .zip:    818.393 (± 2.3%) i/s    (1.22 ms/i) -      4.096k in   5.007817s

Comparison:
 .map { |id| [id] }::      425.0 i/s
               .zip::      818.4 i/s - 1.93x  faster

Calculating -------------------------------------
 .map { |id| [id] }:     2.400M memsize (     0.000  retained)
                        50.001k objects (     0.000  retained)
                         0.000  strings (     0.000  retained)
               .zip:     2.400M memsize (     0.000  retained)
                        50.001k objects (     0.000  retained)
                         0.000  strings (     0.000  retained)

corsonknowles · 2024-09-16T04:47:44Z

Thanks @Earlopain !

Additional specs added, including the derivative cases that you noted, also showing that it plays nice in a method chain.
Both block types/patterns are folded into the on_send now
Added RangeHelp to get the corrector range to match both the method call and the block, but not map's receiver.
Alphabetized the RESTRICT_ON_SEND placement, which seems to be the norm here for constant declaration order.

lib/rubocop/cop/performance/use_zip_to_wrap_array_contents.rb

corsonknowles · 2024-09-16T23:42:19Z

For the very curious, .zip is also faster than .map! {[_1]}.

.each_with_object([]) {|id, object| object << [id]} is among the slowest of the reasonable strategies.

technicalpickles · 2024-09-17T19:36:49Z

.each_with_object([]) {|id, object| object << [id]} is among the slowest of the reasonable strategies.

I added it to my benchmark, and it is slower AND takes a lot more memory:

=== big ===
ruby 3.3.2 (2024-05-30 revision e5a195edf6) [arm64-darwin23]
Warming up --------------------------------------
 .map { |id| [id] }:    43.000 i/100ms
               .zip:    64.000 i/100ms
.each_with_object([]) {|id, object| object << [id]}:
                        32.000 i/100ms
Calculating -------------------------------------
 .map { |id| [id] }:    458.271 (± 1.3%) i/s    (2.18 ms/i) -      2.322k in   5.067609s
               .zip:    622.430 (± 7.7%) i/s    (1.61 ms/i) -      3.136k in   5.087861s
.each_with_object([]) {|id, object| object << [id]}:
                        327.959 (± 2.4%) i/s    (3.05 ms/i) -      1.664k in   5.077224s

Comparison:
 .map { |id| [id] }::      458.3 i/s
               .zip::      622.4 i/s - 1.36x  faster
.each_with_object([]) {|id, object| object << [id]}::      328.0 i/s - 1.40x  slower

Calculating -------------------------------------
 .map { |id| [id] }:     2.400M memsize (     0.000  retained)
                        50.001k objects (     0.000  retained)
                         0.000  strings (     0.000  retained)
               .zip:     2.400M memsize (     0.000  retained)
                        50.001k objects (     0.000  retained)
                         0.000  strings (     0.000  retained)
.each_with_object([]) {|id, object| object << [id]}:
                         2.454M memsize (     0.000  retained)
                        50.001k objects (     0.000  retained)
                         0.000  strings (     0.000  retained)

corsonknowles · 2024-09-25T23:37:36Z

@koic Is there anything else I can do to help get this ready to merge?

corsonknowles · 2024-10-10T23:15:38Z

@bbatsov @rrosenblum I'm looking for committers. Are you able to take a look at this PR?

Thanks so much.

rrosenblum

This is a pretty cool optimization for a specific use case.

I think it would be great to add at least 1 test for a multi-line example

[1, 2, 3].map do |id|
  [id]
end

lib/rubocop/cop/performance/use_zip_to_wrap_array_contents.rb

spec/rubocop/cop/performance/use_zip_to_wrap_array_contents_spec.rb

corsonknowles · 2024-10-13T22:56:18Z

This is a pretty cool optimization for a specific use case.

I think it would be great to add at least 1 test for a multi-line example
[1, 2, 3].map do |id|

  [id]

end

Done!

corsonknowles · 2024-10-27T21:12:38Z

@koic @rrosenblum @Earlopain
Small update here, I renamed this cop in line with:

https://docs.rubocop.org/rubocop-rspec/development.html#choose-a-name

rrosenblum

The code looks good to me. You'll need to find a maintainer to actually approve the changes

rrosenblum · 2024-10-29T14:49:56Z

spec/rubocop/cop/performance/zip_for_array_wrapping_spec.rb

+      it 'does not register an offense as the use of collect is obscure and perhaps intentional' do
+        expect_no_offenses(<<~RUBY)
+          [1, 2, 3].collect { |id| [id] }
+        RUBY
+      end


collect and map are aliases for each other. This should probably be supported

Thanks. Updated to correct on collect as well!

Add new `Performance/ZipForArrayWrapping` cop that checks patterns like `.map { |id| [id] }` or `.map { [_1] }` and can safely replace them with `.zip` This is a Performance Cop for the more efficient way to generate an Array of Arrays. * Performs 40-90% faster than `.map` to iteratively wrap array contents. * Performs 5 - 55% faster on ranges, depending on size.

corsonknowles force-pushed the add_performance_use_zip_to_wrap_arrays branch 7 times, most recently from 46fa44a to c2fe0d8 Compare September 6, 2024 10:32

Earlopain reviewed Sep 14, 2024

View reviewed changes

corsonknowles force-pushed the add_performance_use_zip_to_wrap_arrays branch 4 times, most recently from 260f786 to 4864c84 Compare September 14, 2024 20:05

corsonknowles force-pushed the add_performance_use_zip_to_wrap_arrays branch from 4864c84 to 3733eca Compare September 14, 2024 20:32

corsonknowles requested a review from Earlopain September 14, 2024 21:24

Earlopain reviewed Sep 15, 2024

View reviewed changes

corsonknowles force-pushed the add_performance_use_zip_to_wrap_arrays branch from 3733eca to a6f1f90 Compare September 16, 2024 04:41

corsonknowles force-pushed the add_performance_use_zip_to_wrap_arrays branch from a6f1f90 to 7d516ae Compare September 16, 2024 05:05

corsonknowles requested a review from Earlopain September 16, 2024 05:08

corsonknowles force-pushed the add_performance_use_zip_to_wrap_arrays branch 2 times, most recently from 8bd882d to 93c36b6 Compare September 16, 2024 05:40

Earlopain reviewed Sep 16, 2024

View reviewed changes

lib/rubocop/cop/performance/use_zip_to_wrap_array_contents.rb Outdated Show resolved Hide resolved

corsonknowles force-pushed the add_performance_use_zip_to_wrap_arrays branch from 93c36b6 to 8fcb933 Compare September 16, 2024 15:01

corsonknowles requested a review from Earlopain September 16, 2024 15:02

corsonknowles force-pushed the add_performance_use_zip_to_wrap_arrays branch from 8fcb933 to 3f5085f Compare September 16, 2024 23:44

rrosenblum reviewed Oct 12, 2024

View reviewed changes

corsonknowles force-pushed the add_performance_use_zip_to_wrap_arrays branch 2 times, most recently from ca0e7c6 to 2f64093 Compare October 13, 2024 13:12

corsonknowles changed the title ~~Add UseZipToWrapArrayContents~~ Add new UseZipToWrapArrayContents cop Oct 27, 2024

corsonknowles force-pushed the add_performance_use_zip_to_wrap_arrays branch from 2f64093 to 2697a86 Compare October 27, 2024 21:07

corsonknowles changed the title ~~Add new UseZipToWrapArrayContents cop~~ Add new ZipForArrayWrapping cop Oct 27, 2024

rrosenblum reviewed Oct 29, 2024

View reviewed changes

corsonknowles force-pushed the add_performance_use_zip_to_wrap_arrays branch from 2697a86 to b06bfaf Compare November 10, 2024 23:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new `ZipForArrayWrapping` cop #462

Add new `ZipForArrayWrapping` cop #462

corsonknowles commented Sep 5, 2024 •

edited

Loading

corsonknowles commented Sep 9, 2024

corsonknowles commented Sep 9, 2024

Earlopain left a comment •

edited

Loading

corsonknowles commented Sep 14, 2024 •

edited

Loading

corsonknowles commented Sep 14, 2024

Earlopain left a comment

technicalpickles commented Sep 15, 2024

corsonknowles commented Sep 16, 2024 •

edited

Loading

corsonknowles commented Sep 16, 2024 •

edited

Loading

technicalpickles commented Sep 17, 2024

corsonknowles commented Sep 25, 2024

corsonknowles commented Oct 10, 2024 •

edited

Loading

rrosenblum left a comment

corsonknowles commented Oct 13, 2024

corsonknowles commented Oct 27, 2024

rrosenblum left a comment

rrosenblum Oct 29, 2024

corsonknowles Nov 10, 2024

Add new ZipForArrayWrapping cop #462

Are you sure you want to change the base?

Add new ZipForArrayWrapping cop #462

Conversation

corsonknowles commented Sep 5, 2024 • edited Loading

corsonknowles commented Sep 9, 2024

corsonknowles commented Sep 9, 2024

Earlopain left a comment • edited Loading

Choose a reason for hiding this comment

corsonknowles commented Sep 14, 2024 • edited Loading

corsonknowles commented Sep 14, 2024

Earlopain left a comment

Choose a reason for hiding this comment

technicalpickles commented Sep 15, 2024

corsonknowles commented Sep 16, 2024 • edited Loading

corsonknowles commented Sep 16, 2024 • edited Loading

technicalpickles commented Sep 17, 2024

corsonknowles commented Sep 25, 2024

corsonknowles commented Oct 10, 2024 • edited Loading

rrosenblum left a comment

Choose a reason for hiding this comment

corsonknowles commented Oct 13, 2024

corsonknowles commented Oct 27, 2024

rrosenblum left a comment

Choose a reason for hiding this comment

rrosenblum Oct 29, 2024

Choose a reason for hiding this comment

corsonknowles Nov 10, 2024

Choose a reason for hiding this comment

Add new `ZipForArrayWrapping` cop #462

Add new `ZipForArrayWrapping` cop #462

corsonknowles commented Sep 5, 2024 •

edited

Loading

Earlopain left a comment •

edited

Loading

corsonknowles commented Sep 14, 2024 •

edited

Loading

corsonknowles commented Sep 16, 2024 •

edited

Loading

corsonknowles commented Sep 16, 2024 •

edited

Loading

corsonknowles commented Oct 10, 2024 •

edited

Loading