rubocop / rubocop-performance

An extension of RuboCop focused on code performance checks.
https://docs.rubocop.org/rubocop-performance
MIT License
687 stars 81 forks source link

Correcting `Performance/BigDecimalWithNumericArgument` offences leads to slower code #454

Closed owst closed 2 months ago

owst commented 5 months ago

Performance/BigDecimalWithNumericArgument suggests that BigDecimal(2000) is more performant as BigDecimal("2000") but that is not the case for recent versions of Ruby/BigDecimal:

require 'benchmark/ips'
require 'bigdecimal'

Benchmark.ips do |x|
  x.report('integer string new')  { BigDecimal("2000") }
  x.report('integer new')         { BigDecimal(2000) }
  x.compare!
end

Gives for me (using Ruby 3.3.3 and BigDecimal 3.1.8):

ruby 3.3.3 (2024-06-12 revision f1c7b6f435) [x86_64-darwin22]
Warming up --------------------------------------
  integer string new   228.898k i/100ms
         integer new   337.726k i/100ms
Calculating -------------------------------------
  integer string new      2.520M (± 5.9%) i/s -     12.589M in   5.015809s
         integer new      4.443M (±10.5%) i/s -     21.952M in   5.006675s

Comparison:
         integer new:  4442710.0 i/s
  integer string new:  2519800.0 i/s - 1.76x  slower

Having dug a little bit through various Ruby/BigDecimal versions, I think the improvement was introduced in BigDecimal 3.1.0 (which was first included in Ruby 3.1.0), I suspect this PR led to the performance improvement https://github.com/ruby/bigdecimal/pull/178

Expected behavior

There should be no offence for the faster code BigDecimal(2000)

Actual behavior

There was an offence.

Steps to reproduce the problem

Run rubocop-performance against a file containing BigDecimal(2000)

RuboCop version

1.61.0 (using Parser 3.3.3.0, rubocop-ast 1.31.3, running on ruby 3.3.3) [x86_64-darwin22]
  - rubocop-performance 1.21.1
Earlopain commented 5 months ago

On my machine the difference is even more stark, with 2.25 worse performance. This actually seems to rise when the number is higher:

Somewhere before 2_000_000_000_000 it inverts, then the integer is 2.3x slower. There's some integer limit there I can't be bothered to find out right now. It might be platform-dependant.

owst commented 5 months ago

For me it inverts between 18446744073709551615 and 18446744073709551616 (i.e. 2**64 - 1 and 2**64), which makes sense given the linked PR description https://github.com/ruby/bigdecimal/pull/178 "Implement special conversions for 64-bit integers"

So perhaps the cop should be made aware of the limit

Earlopain commented 5 months ago

Yeah, right. I totally missed the PR title. Seems like this was released in 3.1.0 from December 2021. RuboCop now has a requires_gem api but I don't think you have the granularity to behave differently depending on the version. One thing could be to only activate the cop on 3.1.0 and later (with requires_gem, accepts a version constraint) to provide correct suggestions only. For projects without a lockfile Ruby 3.1.0 is the first release that contains this improvement.

corsonknowles commented 2 months ago

Hoping it's appropriate to offer an opinion here, since Ruby 3.0 is EOL, I'd suggest the best fix here is making the Cop aware of the inversion point, and simply dropping support for 3.0 and below -- either by just stating this in the Docs for this Cop or explicitly disabling it for EOL Ruby versions.

koic commented 2 months ago

Yep, I've opened #454 to resolve this issue.