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

Optimize String#to_u methods for the case of a negative number #7446

Merged
merged 6 commits into from
May 21, 2019

Conversation

wooster0
Copy link
Contributor

This makes String's to_u64_info method return early when the number that should be converted to an unsigned integer is negative.

Benchmark.ips do |bm|
  bm.report("old") do
    "-112978314788778".to_u64 { false }
  end

  bm.report("new") do
    "-112978314788778".new_to_u64 { false }
  end
end
old  58.13M (  17.2ns) (± 5.40%)  0 B/op  10.21× slower
new  593.7M (  1.68ns) (± 4.43%)  0 B/op        fastest

Benchmark.ips do |bm|
  bm.report("old") do
    "-112978314788778913431478989314731437891343734789943413789474187178137914713173484137134780".to_u16 { false }
  end

  bm.report("new") do
    "-112978314788778913431478989314731437891343734789943413789474187178137914713173484137134780".new_to_u16 { false }
  end
end
old  43.73M ( 22.87ns) (± 8.58%)  0 B/op  13.42× slower
new 586.69M (   1.7ns) (± 3.40%)  0 B/op        fastest

src/string.cr Show resolved Hide resolved
src/string.cr Outdated Show resolved Hide resolved
Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the early return is worth merging.

src/string.cr Outdated Show resolved Hide resolved
src/string.cr Outdated Show resolved Hide resolved
src/string.cr Outdated Show resolved Hide resolved
@RX14 RX14 merged commit 8a28fb2 into crystal-lang:master May 21, 2019
@RX14 RX14 added this to the 0.29.0 milestone May 21, 2019
@RX14
Copy link
Contributor

RX14 commented May 21, 2019

It improves performance in an edge case only, but that doesn't mean it's not worth merging, as it's only a small bit of code.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants