Fix conversion in getindex #26615
Fix conversion in getindex #26615
Conversation
|
Right, didn't think about ranges of rationals. Maybe just specialize this implementation on BitIntegers and Bools then? Side-note: the test that fails seems odd. |
haampie
force-pushed
the
fix-range-conversion-in-getindex
branch
from
3860d1e to
d7bd85a
Compare
haampie
force-pushed
the
fix-range-conversion-in-getindex
branch
from
d7bd85a to
01dd920
Compare
| const OverflowSafe = Union{Bool,Int8,Int16,Int32,Int64,Int128, | ||
| UInt8,UInt16,UInt32,UInt64,UInt128} | ||
|
|
||
| function getindex(v::UnitRange{T}, i::Integer) where {T<:OverflowSafe} |
There was a problem hiding this comment.
You could just use Union{Bool, Base.BitInteger} for this.
There was a problem hiding this comment.
Yeah, I was trying that, but it turns out BitInteger is defined a couple includes away in int.jl, and when I tried moving the constants to a separate file int_types.jl, I broke stuff without very useful feedback from make. It's worth defining these types a bit earlier, because other places redefine them as well!
| @@ -473,11 +473,23 @@ end | |||
|
|
|||
| ## indexing | |||
|
|
|||
| _in_unit_range(v::UnitRange, val, i::Integer) = i > 0 && val <= v.stop && val >= v.start | |||
There was a problem hiding this comment.
Ah, yes, I totally believe that this is faster — bounds checks are very friendly to branch prediction and when they miss, well, we really don't care about performance anymore.
|
@nanosoldier I'm not sure if we have any tests that index into |
|
Broadcasting example: Before: 4.075 μs Further issue #26608 (comment) has an example where bounds checking is 2x faster. |
|
Same tricks with Fixing #26623 would make bounds checking for ranges slower unfortunately. |
|
@nanosoldier |
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan |
|
Is this good to go? |
|
I think yes |
* Test more out of bounds errors for the UnitRange type * Do the conversion in getindex of UnitRange only when returning
Fixes #26608 (all edge cases of it) and improves performance for the
@inboundscase a lot.Note that using
@inboundswill now return garbage results when going out of bounds, whereas it previously would throw a conversion error. Should be fine.