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 integer-->string conversions #36470

Merged
merged 5 commits into from
Oct 1, 2020
Merged

Conversation

kimikage
Copy link
Contributor

@kimikage kimikage commented Jun 29, 2020

This avoids invalidations caused by invalidating StringVector(::Integer). (cf. PR #36459) (Edit: The problem has been solved by PR #36459.)
This also makes bin(), dec() and hex() slightly faster, but does not change the Printf.

julia> versioninfo() # on WSL
Julia Version 1.6.0-DEV.312
Commit 6185d24ac3 (2020-06-28 22:42 UTC)
Platform Info:
  OS: Linux (x86_64-pc-linux-gnu)
  CPU: Intel(R) Core(TM) i7-8565U CPU @ 1.80GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-9.0.1 (ORCJIT, skylake)

julia> @btime string.(x, base=2) setup=(x=rand(Int64,1000));
  57.300 μs (2001 allocations: 151.67 KiB) # before
  38.400 μs (2001 allocations: 151.69 KiB) # after

julia> @btime string.(x, base=8) setup=(x=rand(Int64,1000));
  34.700 μs (2001 allocations: 117.30 KiB) # before
  34.800 μs (2001 allocations: 117.30 KiB) # after

julia> @btime string.(x, base=10) setup=(x=rand(Int64,1000));
  45.900 μs (2001 allocations: 117.27 KiB) # before
  35.400 μs (2001 allocations: 117.28 KiB) # after

julia> @btime string.(x, base=10) setup=(x=rand(Int32,1000));
  35.300 μs (2001 allocations: 101.69 KiB) # before
  31.300 μs (2001 allocations: 101.69 KiB) # after

julia> @btime string.(x, base=16) setup=(x=rand(Int64,1000));
  38.300 μs (2001 allocations: 115.84 KiB) # before
  33.600 μs (2001 allocations: 115.80 KiB) # after

@JeffBezanson
Copy link
Member

Awesome!

base/intfuncs.jl Outdated Show resolved Hide resolved
Copy link
Member

@timholy timholy left a comment

Choose a reason for hiding this comment

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

This is a pretty performance-sensitive operation, so unrolling those loops is a great idea.

base/intfuncs.jl Outdated Show resolved Hide resolved
@kimikage
Copy link
Contributor Author

kimikage commented Jul 2, 2020

@rfourquet, I appreciate your comment

I added the clamping into the string(), and changed the types of the pad of internal functions (e.g. bin()) from Integer to Int. Although we can leave them as Integer for "compatibility", I think there are no practical advantages.

I don't know the reason, but there is a little slowdown (~1 ns/number) from the original commit, but the unrolling is still effective.

@timholy
Copy link
Member

timholy commented Aug 2, 2020

Bumping this. I don't have any objections to merging this.

@kimikage
Copy link
Contributor Author

kimikage commented Aug 8, 2020

Since PR #36459 was merged, the ::Int for leading_zeros(x) doesn't seem to be needed anymore.

@musm
Copy link
Contributor

musm commented Sep 19, 2020

Looks like this PR has been approved, is it ok to merge?

@KristofferC KristofferC reopened this Sep 19, 2020
base/intfuncs.jl Outdated Show resolved Hide resolved
base/intfuncs.jl Outdated Show resolved Hide resolved
base/intfuncs.jl Outdated Show resolved Hide resolved
This avoids invalidations caused by invalidating `StringVector(::Integer)`.
This also makes `bin()`, `dec`() and `hex()` slightly faster,
but does not change the Printf.
@StefanKarpinski
Copy link
Member

I've kicked off the failed CI runs, which looked unrelated. This seems good once CI passes.

@StefanKarpinski StefanKarpinski added the merge me PR is reviewed. Merge when all tests are passing label Sep 29, 2020
@musm musm merged commit e7872b3 into JuliaLang:master Oct 1, 2020
@kimikage
Copy link
Contributor Author

kimikage commented Oct 1, 2020

Thanks!

@kimikage kimikage deleted the int2str branch October 1, 2020 23:43
@kimikage
Copy link
Contributor Author

kimikage commented Oct 3, 2020

I'm sorry, but I may have broken something.
cc: @rfourquet
https://github.com/JuliaCI/NanosoldierReports/blob/c1103cd2398e1d0442f115264893823991742a66/pkgeval/by_date/2020-10/01/logs/BitIntegers/1.6.0-DEV-2fe2b43606.log

Edit:
Perhaps the cause is the type of bit shifting.

x >>= 0x4

Can you fix this on the BitIntegers.jl side? (cf. rfourquet/BitIntegers.jl#13)

@rfourquet
Copy link
Member

Perhaps the cause is the type of bit shifting.

@kimikage Thank you for the report, I had no idea this x << 0x4 was broken in BitIntegers.

@DilumAluthge DilumAluthge removed the merge me PR is reviewed. Merge when all tests are passing label Jan 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Must go faster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants