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.

Sign up for GitHub

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

Jump to bottom

Correcting Performance/BigDecimalWithNumericArgument offences leads to slower code #454

Closed
owst opened this issue Jul 2, 2024 · 5 comments · Fixed by #463
Closed

Correcting Performance/BigDecimalWithNumericArgument offences leads to slower code #454

owst opened this issue Jul 2, 2024 · 5 comments · Fixed by #463
Labels
bug Something isn't working

Comments

@owst
Copy link
Contributor

owst commented Jul 2, 2024

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 ruby/bigdecimal#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
Copy link
Contributor

Earlopain commented Jul 3, 2024 •
edited
Loading

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

  • 2_000_000 => 2.35
  • 2_000_000_000 => 2.75

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
Copy link
Contributor Author

owst commented Jul 3, 2024

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

So perhaps the cop should be made aware of the limit

@Earlopain
Copy link
Contributor

Earlopain commented Jul 3, 2024 •
edited
Loading

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
Copy link

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 koic added the bug Something isn't working label Sep 6, 2024
@koic koic mentioned this issue Sep 6, 2024
Merged
8 tasks
@koic
Copy link
Member

koic commented Sep 6, 2024

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

@koic koic closed this as completed in #463 Sep 7, 2024
@koic koic closed this as completed in 6f20945 Sep 7, 2024
koic added a commit that referenced this issue Sep 7, 2024
@koic
Merge pull request #463 from koic/fix_false_positives_for_performance…
0f401f7 
…_big_decimal_with_numeric_argument

[Fix #454] Fix false positives for `Performance/BigDecimalWithNumericArgument`
@y-yagi y-yagi mentioned this issue Sep 17, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[Fix #454] Fix false positives for Performance/BigDecimalWithNumericArgument koic/rubocop-performance
4 participants
@koic @owst @corsonknowles @Earlopain