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

Introduce Overflow Checking for ^(::Integer, ::Integer) v2 #42633

Closed
wants to merge 1 commit into from

Conversation

oscardssmith
Copy link
Member

alternative to #21600. This does the check at the end rather than checking the computation that happened. Currently, this check is too lax for negative numbers to odd powers, but I believe that can be solved without introducing a further slowdown.

This PR still needs performance testing and testing that the check fully accurate.

@oscardssmith oscardssmith added needs decision A decision on this change is needed maths Mathematical functions labels Oct 13, 2021
@thofma
Copy link
Contributor

thofma commented Oct 15, 2021

We are using Int^Int quite heavily and it would be very unfortunate if it were slowed down for no real reason (a person playing in the REPL for the first time and and bothering searching/reading the manual when encountering overflow is not a valid reason IMHO).

It is also quite arbitrary. Will you check multiplication next? I heard some random student in some random class was bitten by this too. Or addition?

@PallHaraldsson
Copy link
Contributor

PallHaraldsson commented Oct 15, 2021

We are using Int^Int quite heavily and it would be very unfortunate if it were slowed down for no real reason [..]
It is also quite arbitrary. Will you check multiplication next?

There are valid reasons Int^Int should return floats in general (for negative power), so I'm going to make a PR for that (maybe it will not be merged until 2.0). It should be as fast as what we have now. The justification for floats does not apply to multiplication, as that naturally can only return integers, for such inputs.

@thofma
Copy link
Contributor

thofma commented Oct 15, 2021

We are using Int^Int quite heavily and it would be very unfortunate if it were slowed down for no real reason [..]
It is also quite arbitrary. Will you check multiplication next?

There are valid reasons Int^Int should return floats in general (for negative power), so I'm going to make a PR for that (maybe it will not be merged until 2.0).

To be honest, I hope that this will never be merged. If one wants to work with doubles, use doubles, but leave Int alone.

Multiplication can also overflow, so it should return doubles as well? Why have Int in the first place?

@KristofferC
Copy link
Member

KristofferC commented Oct 15, 2021

Could someone take the effort of benchmarking this before writing 10 posts about the performance implications? Also, @PallHaraldsson, let's keep on topic.

@DilumAluthge
Copy link
Member

DilumAluthge commented Oct 15, 2021

Is there an "escape hatch"?

E.g. some function unchecked_power(x::Int64, y::Int64) that does not have the overflow checking?

@timholy
Copy link
Member

timholy commented Oct 16, 2021

Escape hatch: Base.power_by_squaring

@chriselrod
Copy link
Contributor

chriselrod commented Oct 17, 2021

Seems like just using Base.checked_mul performs quite well (at least on this computer):

julia> function checked_power_by_squaring(x_, p::Integer)
           x = Base.to_power_type(x_)
           if p == 1
               return copy(x)
           elseif p == 0
               return one(x)
           elseif p == 2
               return Base.checked_mul(x,x)
           elseif p < 0
               isone(x) && return copy(x)
               isone(-x) && return iseven(p) ? one(x) : copy(x)
               Base.throw_domerr_powbysq(x, p)
           end
           t = trailing_zeros(p) + 1
           p >>= t
           while (t -= 1) > 0
               x = Base.checked_mul(x, x)
           end
           y = x
           while p > 0
               t = trailing_zeros(p) + 1
               p >>= t
               while (t -= 1) >= 0
                   x = Base.checked_mul(x, x)
               end
               y = Base.checked_mul(y, x)
           end
           return y
       end
checked_power_by_squaring (generic function with 1 method)

julia> @benchmark checked_power_by_squaring($(Ref(3))[], $(Ref(17))[])
BenchmarkTools.Trial: 10000 samples with 1000 evaluations.
 Range (min  max):  5.386 ns  13.899 ns  ┊ GC (min  max): 0.00%  0.00%
 Time  (median):     5.873 ns              ┊ GC (median):    0.00%
 Time  (mean ± σ):   5.878 ns ±  0.118 ns  ┊ GC (mean ± σ):  0.00% ± 0.00%

                                                          ▁█
  ▂▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▃██ ▂
  5.39 ns        Histogram: frequency by time        5.88 ns <

 Memory estimate: 0 bytes, allocs estimate: 0.

julia> @benchmark Base.power_by_squaring($(Ref(3))[], $(Ref(17))[])
BenchmarkTools.Trial: 10000 samples with 1000 evaluations.
 Range (min  max):  5.598 ns  14.048 ns  ┊ GC (min  max): 0.00%  0.00%
 Time  (median):     6.087 ns              ┊ GC (median):    0.00%
 Time  (mean ± σ):   6.101 ns ±  0.132 ns  ┊ GC (mean ± σ):  0.00% ± 0.00%

                               █
  ▂▂▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▂▁▁▁▁▁▁▁▆█▅▂▁▂▂▂▂▂▂▂▂▁▁▂▁▁▁▁▁▁▁▁▁▁▁▁▁▂ ▂
  5.6 ns         Histogram: frequency by time        6.56 ns <

 Memory estimate: 0 bytes, allocs estimate: 0.

We could make power_by_squaring accept a multiplication function as an input (defaulting to *).

EDIT:

julia> @btime checked_power_by_squaring($(Ref(3))[], $(Ref(17))[])
  5.385 ns (0 allocations: 0 bytes)
129140163

julia> @btime Base.power_by_squaring($(Ref(3))[], $(Ref(17))[])
  5.147 ns (0 allocations: 0 bytes)
129140163

julia> @btime checked_power_by_squaring($(Ref(3))[], $(Ref(27))[])
  6.743 ns (0 allocations: 0 bytes)
7625597484987

julia> @btime Base.power_by_squaring($(Ref(3))[], $(Ref(27))[])
  5.365 ns (0 allocations: 0 bytes)
7625597484987

julia> @btime checked_power_by_squaring($(Ref(3))[], $(Ref(37))[])
  6.272 ns (0 allocations: 0 bytes)
450283905890997363

julia> @btime Base.power_by_squaring($(Ref(3))[], $(Ref(37))[])
  5.609 ns (0 allocations: 0 bytes)
450283905890997363

julia> checked_power_by_squaring(3, 47)
ERROR: OverflowError: 14348907 * 1853020188851841 overflowed for type Int64
Stacktrace:
 [1] throw_overflowerr_binaryop(op::Symbol, x::Int64, y::Int64)
   @ Base.Checked ./checked.jl:154
 [2] checked_mul
   @ ./checked.jl:288 [inlined]
 [3] checked_power_by_squaring(x_::Int64, p::Int64)
   @ Main ./REPL[27]:26
 [4] top-level scope
   @ REPL[44]:1

julia> Base.power_by_squaring(3, 47)
7056148742039409131

@chriselrod
Copy link
Contributor

chriselrod commented Oct 17, 2021

For comparison, checked_pow as implemented in this PR is both slower and doesn't work:

julia> @inline function checked_pow(x::T, p::Integer) where T <: Base.BitInteger #check for overflow
           ans = Base.power_by_squaring(x,p)
           low_bound = ((sizeof(x)<<3)-leading_zeros(x)-1)*p
           trailing_ones(typemax(T)) < low_bound || ans < (one(T)<<low_bound) && throw(OverflowError("$x^$p overflowed"))
           return ans
       end
checked_pow (generic function with 1 method)

julia> @btime checked_pow($(Ref(3))[], $(Ref(17))[])
  7.615 ns (0 allocations: 0 bytes)
129140163

julia> @btime checked_pow($(Ref(3))[], $(Ref(27))[])
  8.120 ns (0 allocations: 0 bytes)
7625597484987

julia> @btime checked_pow($(Ref(3))[], $(Ref(37))[])
  8.041 ns (0 allocations: 0 bytes)
450283905890997363

julia> checked_pow(3, 47)
7056148742039409131

@KristofferC
Copy link
Member

Did anyone try with the original implementation in #21600?

@chriselrod
Copy link
Contributor

julia> @btime checked_power_by_squaring($(Ref(3))[], $(Ref(10))[])
  5.044 ns (0 allocations: 0 bytes)
59049

julia> @btime Base.power_by_squaring($(Ref(3))[], $(Ref(10))[])
  3.613 ns (0 allocations: 0 bytes)
59049

julia> @btime power_by_squaring($(Ref(3))[], $(Ref(10))[], Base.mul_with_overflow)
  4.807 ns (0 allocations: 0 bytes)
59049

julia> @btime checked_power_by_squaring($(Ref(3))[], $(Ref(17))[])
  5.395 ns (0 allocations: 0 bytes)
129140163

julia> @btime Base.power_by_squaring($(Ref(3))[], $(Ref(17))[])
  4.704 ns (0 allocations: 0 bytes)
129140163

julia> @btime power_by_squaring($(Ref(3))[], $(Ref(17))[], Base.mul_with_overflow)
  5.875 ns (0 allocations: 0 bytes)
129140163

julia> @btime checked_power_by_squaring($(Ref(3))[], $(Ref(27))[])
  6.476 ns (0 allocations: 0 bytes)
7625597484987

julia> @btime Base.power_by_squaring($(Ref(3))[], $(Ref(27))[])
  5.924 ns (0 allocations: 0 bytes)
7625597484987

julia> @btime power_by_squaring($(Ref(3))[], $(Ref(27))[], Base.mul_with_overflow)
  6.267 ns (0 allocations: 0 bytes)
7625597484987

julia> @btime checked_power_by_squaring($(Ref(3))[], $(Ref(37))[])
  6.956 ns (0 allocations: 0 bytes)
450283905890997363

julia> @btime Base.power_by_squaring($(Ref(3))[], $(Ref(37))[])
  6.299 ns (0 allocations: 0 bytes)
450283905890997363

julia> @btime power_by_squaring($(Ref(3))[], $(Ref(37))[], Base.mul_with_overflow)
  6.474 ns (0 allocations: 0 bytes)
450283905890997363

julia> checked_power_by_squaring(3, 47)
ERROR: OverflowError: 14348907 * 1853020188851841 overflowed for type Int64
Stacktrace:
 [1] throw_overflowerr_binaryop(op::Symbol, x::Int64, y::Int64)
   @ Base.Checked ./checked.jl:154
 [2] checked_mul
   @ ./checked.jl:288 [inlined]
 [3] checked_power_by_squaring(x_::Int64, p::Int64)
   @ Main ./REPL[6]:26
 [4] top-level scope
   @ REPL[33]:1

julia> power_by_squaring(3, 47, Base.mul_with_overflow)
ERROR: OverflowError
Stacktrace:
 [1] power_by_squaring(x_::Int64, p::Int64, mul_with_overflow::typeof(Base.Checked.mul_with_overflow))
   @ Main ./REPL[16]:32
 [2] top-level scope
   @ REPL[35]:1

@oscardssmith oscardssmith deleted the checked-int-pow branch December 28, 2021 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maths Mathematical functions needs decision A decision on this change is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants