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.

Sign up for GitHub

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

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

Merged
merged 11 commits into from
Jul 28, 2021
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
6fd2576
Fix length(::AbstractUnitRange), faster length(::AbstractUnitRange{<:…
sostock Jul 6, 2021
206f154
Replace `one` by `oneunit`
sostock Jul 6, 2021
c8e04d6
Add test
sostock Jul 6, 2021
4a49d28
Add reference to PR
sostock Jul 7, 2021
6a4a9a8
Add comment that specialized method is only needed for performance
sostock Jul 20, 2021
0078f1e
More one(a) -> oneunit(a)
sostock Jul 20, 2021
7110222
Merge branch 'master' into unitrangelength
sostock Jul 21, 2021
926029d
Consistent use of Integer()
sostock Jul 22, 2021
c39e26b
Merge branch 'master' into unitrangelength
sostock Jul 24, 2021
875a0fa
Add checked_length(::AbstractUnitRange{<:Rational})
sostock Jul 24, 2021
e6528e1
Fix a test
sostock Jul 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions base/range.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -681,8 +681,8 @@ 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 + one(a)
a = last(r) - first(r) # even when isempty, by construction (with overflow)
return Integer(a + oneunit(a))
sostock marked this conversation as resolved.
Show resolved Hide resolved
end

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

function length(r::AbstractUnitRange{T}) where T<:Rational
sostock marked this conversation as resolved.
Show resolved Hide resolved
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
13 changes: 13 additions & 0 deletions test/ranges.jl
View file
Open in desktop
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