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

Cop suggestion: merge({ kv1 }) is faster than merge(kv1) on Ruby >= 2.7 #372

Open
tagliala opened this issue Sep 16, 2023 · 0 comments
Open

Comments

@tagliala
Copy link
Contributor

tagliala commented Sep 16, 2023

Hi,

I do not know if this deserves a cop or if it is the correct place to discuss this kind of requests

While performing some tests, I've noticed that starting from Ruby 2.7 (because of keyword arguments?), {}.merge({ hello: 'world', foo: 'bar' }) is about 15% faster than {}.merge(hello: 'world', foo: 'bar') and uses the same memory.

I may tend to omit curl parenthesis when adding multiple keys in situations like

# I tend to write
hash.merge(
  key1: value1,
  key2: value2,
  key3: value3
)

# Rather than
hash.merge({
  key1: value1,
  key2: value2,
  key3: value3
})

Slowness is directly related to the number of arguments passed

%i[ips memory].each do |benchmark|
  Benchmark.send(benchmark) do |x|
    x.report('merge({...})') { {}.merge({ one: '1', two: '2', three: '3', four: '4' }) }
    x.report('merge(...)') { {}.merge(one: '1', two: '2', three: '3', four: '4') }
    x.compare!
  end
end
Ruby version: 3.2.2
Warming up --------------------------------------
        merge({...})   346.094k i/100ms
          merge(...)   273.690k i/100ms
Calculating -------------------------------------
        merge({...})      3.461M (± 1.2%) i/s -     17.305M in   5.001317s
          merge(...)      2.728M (± 0.7%) i/s -     13.684M in   5.015808s

Comparison:
        merge({...}):  3460524.7 i/s
          merge(...):  2728413.4 i/s - 1.27x  (± 0.00) slower

Calculating -------------------------------------
        merge({...})   376.000  memsize (     0.000  retained)
                         3.000  objects (     0.000  retained)
                         0.000  strings (     0.000  retained)
          merge(...)   376.000  memsize (     0.000  retained)
                         3.000  objects (     0.000  retained)
                         0.000  strings (     0.000  retained)

Comparison:
        merge({...}):        376 allocated
          merge(...):        376 allocated - same

M1 Pro, x86 Ruby, slowness of merge(kv1) compared to merge({ kv1 })

Ruby 1 arg 2 args 3 args 4 args
2.6.10 same-ish same-ish same-ish same-ish
2.7.8 1.07x 1.16x 1.23x 1.32x
3.2.2 1.09x 1.16x 1.25x 1.29x
3.3.0-p2 1.07x 1.14x 1.20x 1.25x

The lower, the better

Describe the solution you'd like

Maybe introduce a Cop to check that merge is not called with multiple keys

Describe alternatives you've considered

Not having this Cop is just fine

Additional context

A similar performance drop happens with merge!. Performance/RedundantMerge allows to detect issues when there are one or two arguments, but with three arguments RuboCop does not complain

Example:

%i[ips memory].each do |benchmark|
  Benchmark.send(benchmark) do |x|
    x.report('merge!({...})') { {}.merge!({ one: '1', two: '2', three: '3' }) }
    x.report('merge!(...)') { {}.merge!(one: '1', two: '2', three: '3') }
    x.compare!
  end
end
Ruby version: 3.2.2
Warming up --------------------------------------
       merge!({...})   303.044k i/100ms
         merge!(...)   249.452k i/100ms
Calculating -------------------------------------
       merge!({...})      3.018M (± 2.8%) i/s -     15.152M in   5.023729s
         merge!(...)      2.497M (± 4.4%) i/s -     12.473M in   5.006090s

Comparison:
       merge!({...}):  3018437.9 i/s
         merge!(...):  2496970.1 i/s - 1.21x  (± 0.00) slower

Calculating -------------------------------------
       merge!({...})   336.000  memsize (     0.000  retained)
                         2.000  objects (     0.000  retained)
                         0.000  strings (     0.000  retained)
         merge!(...)   336.000  memsize (     0.000  retained)
                         2.000  objects (     0.000  retained)
                         0.000  strings (     0.000  retained)

Comparison:
       merge!({...}):        336 allocated
         merge!(...):        336 allocated - same
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

No branches or pull requests

1 participant