Skip to content

Fix length(::AbstractUnitRange) and speed up length(::AbstractUnitRange{<:Rational}) #41479

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

Merged
merged 11 commits into from
Jul 28, 2021
17 changes: 8 additions & 9 deletions base/range.jl
Original file line number Diff line number Diff line change
Expand Up @@ -663,17 +663,17 @@ function checked_length(r::OrdinalRange{T}) where T
else
diff = checked_sub(stop, start)
end
a = Integer(div(diff, s))
return checked_add(a, oneunit(a))
a = div(diff, s)
return Integer(checked_add(a, oneunit(a)))
end

function checked_length(r::AbstractUnitRange{T}) where T
# compiler optimization: remove dead cases from above
if isempty(r)
return Integer(first(r) - first(r))
end
a = Integer(checked_add(checked_sub(last(r), first(r))))
return checked_add(a, oneunit(a))
a = checked_sub(last(r), first(r))
return Integer(checked_add(a, oneunit(a)))
end

function length(r::OrdinalRange{T}) where T
Expand All @@ -690,15 +690,14 @@ function length(r::OrdinalRange{T}) where T
else
diff = stop - start
end
a = Integer(div(diff, s))
return a + oneunit(a)
a = div(diff, s)
return Integer(a + oneunit(a))
end


function length(r::AbstractUnitRange{T}) where T
@_inline_meta
a = Integer(last(r) - first(r)) # even when isempty, by construction (with overflow)
return a + oneunit(a)
a = last(r) - first(r) # even when isempty, by construction (with overflow)
return Integer(a + oneunit(a))
end

length(r::OneTo) = Integer(r.stop - zero(r.stop))
Expand Down
18 changes: 18 additions & 0 deletions base/rational.jl
Original file line number Diff line number Diff line change
Expand Up @@ -533,3 +533,21 @@ function hash(x::Rational{<:BitInteger64}, h::UInt)
h = hash_integer(num, h)
return h
end

# These methods are only needed for performance. Since `first(r)` and `last(r)` have the
# same denominator (because their difference is an integer), `length(r)` can be calulated
# without calling `gcd`.
function length(r::AbstractUnitRange{T}) where T<:Rational
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method not only adds performance but changes behavior as well: Without this method, length(::AbstractUnitRange{<:Rational}) uses Rational arithmetic, which is checked. The specialized method uses integer arithmetic, so it can overflow.

I realize that we are okay with an overflowing length, but should this also apply to Rationals, which are using checked arithmetic by default? Or should the behavior be changed so that it matches Rational arithmetic?

I will add a specialized checked_length(::AbstractUnitRange{<:Rational}) method in any case.

Copy link
Member

Choose a reason for hiding this comment

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

I think it is okay, since it should generally only overflow for extreme values that are unlikely

@_inline_meta
f = first(r)
l = last(r)
return div(l.num - f.num + f.den, f.den)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
return div(l.num - f.num + f.den, f.den)
a = fld(l.num, l.den) - fld(f.num, f.den)
return a + oneunit(a)

This change would reduce the risk of overflow, but would be slightly less performant (two integer divisions instead of one). Is it worth it?

end
function checked_length(r::AbstractUnitRange{T}) where T<:Rational
f = first(r)
l = last(r)
if isempty(r)
return f.num - f.num
end
return div(checked_add(checked_sub(l.num, f.num), f.den), f.den)
end
15 changes: 14 additions & 1 deletion test/ranges.jl
Original file line number Diff line number Diff line change
Expand Up @@ -544,6 +544,19 @@ end
end
end

# A number type with the overflow behavior of `UInt8`. Conversion to `Integer` returns an
# `Int32`, i.e., a type with different `typemin`/`typemax`. See #41479
struct OverflowingReal <: Real
val::UInt8
end
OverflowingReal(x::OverflowingReal) = x
Base.:<=(x::OverflowingReal, y::OverflowingReal) = x.val <= y.val
Base.:+(x::OverflowingReal, y::OverflowingReal) = OverflowingReal(x.val + y.val)
Base.:-(x::OverflowingReal, y::OverflowingReal) = OverflowingReal(x.val - y.val)
Base.round(x::OverflowingReal, ::RoundingMode) = x
Base.Integer(x::OverflowingReal) = Int32(x.val)
@test length(OverflowingReal(1):OverflowingReal(0)) == 0

@testset "loops involving typemin/typemax" begin
n = 0
s = 0
Expand Down Expand Up @@ -1095,7 +1108,7 @@ end
@testset "issue 10950" begin
r = 1//2:3
@test length(r) == 3
@test_throws MethodError checked_length(r) == 3 # this would work if checked_sub is defined on Rational
@test checked_length(r) == 3
i = 1
for x in r
@test x == i//2
Expand Down