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

avoid expensive runtime div in parse for common bases #29892

Merged
merged 1 commit into from
Nov 2, 2018

Conversation

KristofferC
Copy link
Sponsor Member

@KristofferC KristofferC commented Nov 1, 2018

10 and 16 are common bases to parse to so let's avoid the "killer div" in those cases.

Benchmarks:

julia> @btime parse(Int64, "12");
  45.326 ns (0 allocations: 0 bytes)

julia> @btime parse(Int64, "12345678912345");
  84.381 ns (0 allocations: 0 bytes)

julia> @btime parse(UInt8, "0x10");
  47.826 ns (0 allocations: 0 bytes)

julia> @btime parse(UInt64, "0x123aca1231a");
  78.174 ns (0 allocations: 0 bytes)

After

julia> @btime parse(Int64, "12");
  36.399 ns (0 allocations: 0 bytes)

julia> @btime parse(Int64, "12345678912345");
  74.086 ns (0 allocations: 0 bytes)

julia> @btime parse(UInt8, "0x10");
  39.586 ns (0 allocations: 0 bytes)

julia> @btime parse(UInt64, "0x123aca1231a");
  72.017 ns (0 allocations: 0 bytes)

I have some thought on how this overflow handling can be done a bit better, but for now, might as well do the incremental thing.

@KristofferC KristofferC added performance Must go faster domain:strings "Strings!" labels Nov 1, 2018
@KristofferC
Copy link
Sponsor Member Author

@nanosoldier runbenchmarks("parse", vs = ":master")

base/parse.jl Outdated
m::T = div(typemax(T) - base + 1, base)
# Special case the common cases of base being 10 or 16 to avoid expensive runtime div
m::T = base == 10 ? div(typemax(T) - T(11), T(10)) :
base == 16 ? div(typemax(T) - T(16), T(16)) :
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Should these be - T(9) and - T(15)?

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

Oops, should have ran the tests first heh.

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

@KristofferC KristofferC merged commit 05180b3 into master Nov 2, 2018
@KristofferC KristofferC deleted the kc/div_bleh branch November 2, 2018 18:29
KristofferC added a commit that referenced this pull request Nov 12, 2018
@KristofferC KristofferC mentioned this pull request Nov 12, 2018
61 tasks
tkf pushed a commit to tkf/julia that referenced this pull request Nov 21, 2018
KristofferC added a commit that referenced this pull request Dec 12, 2018
KristofferC added a commit that referenced this pull request Feb 11, 2019
KristofferC added a commit that referenced this pull request Feb 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:strings "Strings!" performance Must go faster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants