rubocop-performance icon indicating copy to clipboard operation
rubocop-performance copied to clipboard

Add new `Performance/CollectionLiteralInMethod` cop

Open exoego opened this issue 3 years ago • 2 comments

Implements https://github.com/rubocop/rubocop-performance/issues/71

This PR adds new Performance/CollectionLiteralInMethod cop. This cop detects Array and Hash literal used in method definitions, including singleton methods. It prevents allocating collections every time the method is used.

This PR also fixed code offenses in codebase found by the new cop.

The new rule can be considered a stricter version of Performance/CollectionLiteralInLoop cop that allows literals in method as long as those are used outside of loop.

Benchmark

The below benchmark showed that using constant in method is 48-52% faster than literal in method.

require 'benchmark/ips'

Benchmark.ips do |x|
  x.config(:time => 5, :warmup => 2)
  x.time = 5
  x.warmup = 2

  CONFIG = {
    :foo => "bar",
    :sit => "amet",
  }.freeze

  x.report("literal") do
    {
      :foo => "bar",
      :sit => "amet",
    }.merge({ :hoge => "fuga" })
  end

  x.report("constant") do
    CONFIG.merge({ :hoge => "fuga" })
  end

  x.compare!
end

Ruby 2.7.6

Warming up --------------------------------------
             literal   521.232k i/100ms
            constant   773.297k i/100ms
Calculating -------------------------------------
             literal      5.219M (± 0.7%) i/s -     26.583M in   5.093590s
            constant      7.728M (± 1.0%) i/s -     38.665M in   5.003549s

Comparison:
            constant:  7728245.0 i/s
             literal:  5219105.6 i/s - 1.48x  (± 0.00) slower

Ruby 3.1.2

Warming up --------------------------------------
             literal   435.012k i/100ms
            constant   674.282k i/100ms
Calculating -------------------------------------
             literal      4.419M (± 0.7%) i/s -     22.186M in   5.021295s
            constant      6.703M (± 0.9%) i/s -     33.714M in   5.030216s

Comparison:
            constant:  6702873.6 i/s
             literal:  4418528.0 i/s - 1.52x  (± 0.00) slower

Before submitting the PR make sure the following are checked:

  • [x] The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • [x] Wrote good commit messages.
  • [x] Commit message starts with [Fix #issue-number] (if the related issue exists).
  • [x] Feature branch is up-to-date with master (if not - rebase it).
  • [x] Squashed related commits together.
  • [x] Added tests.
  • [x] Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
  • [x] 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.

exoego avatar Sep 04 '22 00:09 exoego

Hhm, I'm not sure if the benchmark you provided is reasonable. Because the benchmark does not include the cost of assignment to constants. For methods that are executed only once, writing literals as they are seems faster than constant assignment. So, unlike Performance/CollectionLiteralInLoop cop, constants may not always be faster than literals.

koic avatar Sep 05 '22 06:09 koic

For methods that are executed only once, writing literals as they are seems faster than constant assignment.

Correct.

I think #71 is for methods that are invoked many times where an overhead for constant initialization is negligible. The benchmark corresponds to such a situation.

exoego avatar Sep 05 '22 07:09 exoego