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/CollectionLiteralInMethod cop #303

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

exoego
Copy link

@exoego exoego commented Sep 4, 2022

Implements #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:

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

@exoego exoego force-pushed the collection-literal-in-method branch 4 times, most recently from 1cca3a6 to ca54649 Compare September 4, 2022 01:07
@exoego exoego marked this pull request as ready for review September 4, 2022 01:14
@exoego exoego force-pushed the collection-literal-in-method branch from ca54649 to 72b0dfc Compare September 5, 2022 00:17
@exoego exoego force-pushed the collection-literal-in-method branch from 72b0dfc to fab23f8 Compare September 5, 2022 00:19
@koic
Copy link
Member

koic commented Sep 5, 2022

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.

@exoego
Copy link
Author

exoego commented Sep 5, 2022

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 exoego closed this Sep 12, 2022
@exoego exoego reopened this Sep 12, 2022
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.

None yet

2 participants