JuliaGraphics / ColorTypes.jl

Basic color definitions and traits
Other
78 stars 36 forks source link

Type instability in `ccolor` on julia-1.9.x when using `--check-bounds=no` #292

Open johnomotani opened 1 year ago

johnomotani commented 1 year ago

I came across this while investigating a slow-down using Makie (https://github.com/MakieOrg/Makie.jl/issues/3132).

There seems to be a type-instability in ccolor() when using --check-bounds=no. The issue (at least my original problem) was not present on julia-1.8.5, but is present on 1.9.0, 1.9.1, 1.9.2. I'm using ColorTypes-0.11.4.

Running the following in a REPL started with julia --check-bounds=no:

using Cthulhu, ColorTypes, FixedPointNumbers
using ColorTypes:ARGB32
@descend ColorTypes.ccolor(RGB{N0f8}, ARGB32)

I see that the signature of src/traits.jl:439 is

439     isabstracttype(Tdest::Type{N0f8}) || !(issupported(C::Union{Type{Union{}}, Type{RGB}}, Tdest::Type{N0f8})::Bool)::Bool || return C{Tdest}

and I guess the Union{Type{Union{}}, Type{RGB}} indicates a type instability. When not passing the --check-bounds=no flag, that line would be

439     isabstracttype(Tdest::Type{N0f8}) || !issupported(C::Type{RGB}, Tdest::Type{N0f8})::Bool || return C{Tdest}

I'm out of my depth! So hoping someone will be able to understand and fix this. Happy to do something to diagnose/test if you can let me know what would be useful.

Edit: it looks like the problem might be in pureinterset() (called from src/traits.jl:417)

417         C::Core.Const(Union{}) = pureintersect(Cdestbase::Type{RGB}, Csrcbase::Type{RGB24})::Type{Union{}}

without --check-bounds=no, Cthulhu does not annotate the the return type of that call, but is happy to say that C is Type{RGB} further down.

Actually, looking again maybe the problem is that isabstracttype(Cdestbase) is evaluating to false during type-inference without --check-bounds=no, but failing to do that with --check-bounds=no. See https://github.com/JuliaLang/julia/issues/50825 for an attempt to report a minimal version of this bug.

johnomotani commented 1 year ago

Edited the issue to add: it looks like the problem might be in pureinterset() (called from src/traits.jl:417)

417         C::Core.Const(Union{}) = pureintersect(Cdestbase::Type{RGB}, Csrcbase::Type{RGB24})::Type{Union{}}

without --check-bounds=no, Cthulhu does not annotate the the return type of that call, but is happy to say that C is Type{RGB} further down.

johnomotani commented 1 year ago

One more thought (again appended to the original issue): Actually, looking again maybe the problem is that isabstracttype(Cdestbase) is evaluating to false during type-inference without --check-bounds=no, but failing to do that with --check-bounds=no. Now I'm really stuck and have to give up for the moment!

johnomotani commented 1 year ago

According to https://github.com/JuliaLang/julia/issues/50825 this is a known issue and a 'won't-fix', so I guess there's nothing to be done here...

johnomotani commented 1 year ago

Reopening because although the MRE in https://github.com/JuliaLang/julia/issues/50825 is fixed on julia-master, the performance regression in Makie (https://github.com/MakieOrg/Makie.jl/issues/3132) still exists on julia-master. The problem still seems to be related to type-instability in ColorTypes.ccolor, related to issupported() and eltypes_supported(). One of the _eltypes_supported() methods is marked as @pure https://github.com/JuliaGraphics/ColorTypes.jl/blob/6a10da4e309ade78dcf8e6def510ca0cb3684901/src/traits.jl#L150 and apparently @pure no longer works (? I don't understand what's going on myself...) in julia-1.9.x, see https://github.com/JuliaLang/julia/issues/49472, https://github.com/JuliaLang/julia/pull/44776.

I can reproduce the type-instability (when running julia --check-bounds=no) with the following (which is probably not minimal, and depends on Makie - I could maybe try a bit to cut it down if it's useful, but I'm a newbie with both Makie and ColorTypes so I find it hard):

using Cthulhu, CairoMakie, FixedPointNumbers, ColorTypes; using ColorTypes: ARGB32, RGB
glnative = PermutedDimsArray{ARGB32, 2, (2, 1), (2, 1), PermutedDimsArray{ARGB32, 2, (2, 1), (2, 1), Matrix{ARGB32}}}(PermutedDimsArray{ARGB32, 2, (2, 1), (2, 1), Matrix{ARGB32}}(zeros(ARGB32,2,2)))
fig = Figure()
io = VideoStream(fig)
xdim, ydim = size(glnative)
@descend copy!(view(io.buffer, 1:xdim, 1:ydim), glnative)

Then using the Cthulhu interface to descend into: copyto! -> copyto_unaliased! -> setindex! -> _setindex! -> setindex! -> setindex! -> convert -> ccolor -> issupported -> eltypes_supported we see that _eltypes_supported has return type Any, showing that things are type-unstable.