-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Fixes for non-Int based lengths #37741
Conversation
Alternatively it could be a call to |
The |
Pretty old commit though; we might have needed it in 2016 but not now? I would just test stuff. Make sure you measure invalidations with some package that defines a new |
Can you do what you propose with something like: julia> x = 1:BigInt(10)
1:10
julia> size(x) |> typeof
Tuple{BigInt}
julia> SubArray(x,(1:2,)) |> size |> typeof # Tuple{BigInt} with this change
Tuple{Int64} |
I'm afraid this more recent one might also be relevant for JuliaArrays/InfiniteArrays.jl#44 |
Ouch, that one looks a disaster.... I should probably add a lightweight ∞-array implementation in the tests (a la the OffsetArray tests) to avoid these types of changes killing the package in future versions of Julia... |
But it's also pretty important to avoid invalidation. Do we really need to support arrays that don't have |
We do if you want ApproxFun to continue to work. InfiniteArrays.jl is working quite well right now so suddenly insisting arrays have In any case, adding |
It has nothing to do with a faster concatenation implementation, though that would be a side effect. See https://julialang.org/blog/2020/08/invalidations/. Not saying we can't change this, but you need to understand the reasons first and then we can talk about what to do about it. |
Yes I understand that. I'll try to come up with an alternative, I'm thinking something like: _vcat_similar(T, V) = similar(V[1], T, mapreduce(length, +, V))
_typed_vcat(::Type{T}, V::AbstractVecOrTuple{AbstractVector}) where T =
_typed_vcat!(_vcat_similar(T, V), V)
function _typed_vcat!(a, V)
pos = 1
… # current code with all the Int(...)::Int calls
end This would allow me to overload based on the type of |
I will bet this PR is probably OK for invalidation, but you should still check (until we have #37193 it requires a manual check). Maybe try the subject of #36459? It might be useful to temporarily back out #37088, though I bet that fixed problems in the |
Or actually, probably easier: julia> using MethodAnalysis
julia> cd(joinpath(dirname(dirname(pathof(MethodAnalysis))), "demos"))
julia> include("abstract.jl")
julia> include("abstract_tests.jl")
Test Failed at /home/tim/.julia/dev/MethodAnalysis/demos/abstract_tests.jl:58
Expression: meancounts < 32
Evaluated: 36.07617360496014 < 32
ERROR: LoadError: There was an error during testing
in expression starting at /home/tim/.julia/dev/MethodAnalysis/demos/abstract_tests.jl:58 Looks like we've already regressed substantially, but you'll find out if this PR makes it that much worse. |
It throws this: julia> include("abstract_tests.jl")
Test Failed at /Users/solver/.julia/packages/MethodAnalysis/VLLrU/demos/abstract_tests.jl:53
Expression: length(badcounts) < 1000
Evaluated: 1071 < 1000
ERROR: LoadError: There was an error during testing
in expression starting at /Users/solver/.julia/packages/MethodAnalysis/VLLrU/demos/abstract_tests.jl:53 |
Sorry, that was on the last tagged version of MethodAnalysis.jl. All tests pass on MethodAnalysis#master, apart from @test function_returns(ispow2, Bool) I don't see why that would be connected to this though. But I think it's probably best if I make all necessary changes for InfiniteArrays.jl in a single PR (there's some cases in printing where |
If you stay under the bounds in those tests then I think any changes you make should be fine, at least for the purpose of Julia itself. (Some packages might experience more invalidation, but in a sense that's their own fault for having poorly-inferred code---although in the case of |
InfiniteArrays tests now pass, so I believe this PR is ready to be merged. @timholy I had to revert some of #37080 because it triggered a breaking change independent of InfiniteArrays.jl: it caused |
@dlfivefifty, note test failures. |
@timholy It doesn't look like the test failures are caused by this PR, unless I'm not understanding. |
Try rebase on master where CI should be improved. |
Something like julia> mis = methodinstances(Base.print_matrix)
6-element Vector{Core.MethodInstance}:
MethodInstance for Base.print_matrix(::IOContext{Base.TTY}, ::Vector{Int64})
MethodInstance for Base.print_matrix(::IOContext{Base.TTY}, ::Matrix{Int64})
MethodInstance for Base.print_matrix(::IOContext{Base.TTY}, ::AbstractVecOrMat{T} where T)
MethodInstance for Base.print_matrix(::IOContext{Base.TTY}, ::Vector{Int64}, ::String, ::String, ::String, ::String, ::String, ::String, ::Int64, ::Int64)
MethodInstance for Base.print_matrix(::IOContext{Base.TTY}, ::Matrix{Int64}, ::String, ::String, ::String, ::String, ::String, ::String, ::Int64, ::Int64)
MethodInstance for Base.print_matrix(::IOContext{Base.TTY}, ::AbstractVecOrMat{T} where T, ::String, ::String, ::String, ::String, ::String, ::String, ::Int64, ::Int64)
julia> mi = mis[end] # pick one with abstract types in the signature
MethodInstance for Base.print_matrix(::IOContext{Base.TTY}, ::AbstractVecOrMat{T} where T, ::String, ::String, ::String, ::String, ::String, ::String, ::Int64, ::Int64)
julia> mi.backedges
4-element Vector{Any}:
MethodInstance for Base.print_matrix(::IOContext{Base.TTY}, ::Vector{Int64})
MethodInstance for Base.print_matrix(::IOContext{Base.TTY}, ::Matrix{Int64})
MethodInstance for Base.print_matrix(::IOContext{Base.TTY}, ::Vector{Core.MethodInstance})
MethodInstance for Base.print_matrix(::IOContext{Base.TTY}, ::Vector{Any})
julia> using AbstractTrees
julia> print_tree(mi)
MethodInstance for Base.print_matrix(::IOContext{TTY}, ::AbstractVecOrMat{T} where T, ::String, ::String, ::String, ::String, ::String, ::String, ::Int64, ::Int64)
├─ MethodInstance for Base.print_matrix(::IOContext{TTY}, ::Vector{Int64})
│ └─ MethodInstance for Base.print_array(::IOContext{TTY}, ::Vector{Int64})
│ └─ MethodInstance for show(::IOContext{TTY}, ::MIME{Symbol("text/plain")}, ::Vector{Int64})
├─ MethodInstance for Base.print_matrix(::IOContext{TTY}, ::Matrix{Int64})
│ └─ MethodInstance for Base.print_array(::IOContext{TTY}, ::Matrix{Int64})
│ └─ MethodInstance for show(::IOContext{TTY}, ::MIME{Symbol("text/plain")}, ::Matrix{Int64})
├─ MethodInstance for Base.print_matrix(::IOContext{TTY}, ::Vector{MethodInstance})
│ └─ MethodInstance for Base.print_array(::IOContext{TTY}, ::Vector{MethodInstance})
│ └─ MethodInstance for show(::IOContext{TTY}, ::MIME{Symbol("text/plain")}, ::Vector{MethodInstance})
└─ MethodInstance for Base.print_matrix(::IOContext{TTY}, ::Vector{Any})
└─ MethodInstance for Base.print_array(::IOContext{TTY}, ::Vector{Any})
└─ MethodInstance for show(::IOContext{TTY}, ::MIME{Symbol("text/plain")}, ::Vector{Any}) but I'm more interested in the backedges of |
Below is what I get. Should I try again with julia> mis = methodinstances(Base.print_matrix)
6-element Vector{Core.MethodInstance}:
MethodInstance for Base.print_matrix(::IOContext{Base.TTY}, ::Vector{Int64})
MethodInstance for Base.print_matrix(::IOContext{Base.TTY}, ::Matrix{Int64})
MethodInstance for Base.print_matrix(::IOContext{Base.TTY}, ::AbstractVecOrMat{T} where T)
MethodInstance for Base.print_matrix(::IOContext{Base.TTY}, ::Vector{Int64}, ::String, ::String, ::String, ::String, ::String, ::String, ::Int64, ::Int64)
MethodInstance for Base.print_matrix(::IOContext{Base.TTY}, ::Matrix{Int64}, ::String, ::String, ::String, ::String, ::String, ::String, ::Int64, ::Int64)
MethodInstance for Base.print_matrix(::IOContext{Base.TTY}, ::AbstractVecOrMat{T} where T, ::String, ::String, ::String, ::String, ::String, ::String, ::Int64, ::Int64)
julia> mi = mis[end] # pick one with abstract types in the signature
MethodInstance for Base.print_matrix(::IOContext{Base.TTY}, ::AbstractVecOrMat{T} where T, ::String, ::String, ::String, ::String, ::String, ::String, ::Int64, ::Int64)
julia> mi.backedges
ERROR: UndefRefError: access to undefined reference
Stacktrace:
[1] getproperty(x::Core.MethodInstance, f::Symbol)
@ Base ./Base.jl:33
[2] top-level scope
@ REPL[7]:1
julia> print_tree(mi)
MethodInstance for Base.print_matrix(::IOContext{TTY}, ::AbstractVecOrMat{T} where T, ::String, ::String, ::String, ::String, ::String, ::String, ::Int64, ::Int64)
julia> mis = methodinstances(Base._print_matrix)
4-element Vector{Core.MethodInstance}:
MethodInstance for Base._print_matrix(::IOContext{Base.TTY}, ::Vector{Int64}, ::String, ::String, ::String, ::String, ::String, ::String, ::Int64, ::Int64, ::UnitRange{Int64}, ::UnitRange{Int64})
MethodInstance for Base._print_matrix(::IOContext{Base.TTY}, ::Matrix{Int64}, ::String, ::String, ::String, ::String, ::String, ::String, ::Int64, ::Int64, ::UnitRange{Int64}, ::UnitRange{Int64})
MethodInstance for Base._print_matrix(::IOContext{Base.TTY}, ::Vector{Core.MethodInstance}, ::String, ::String, ::String, ::String, ::String, ::String, ::Int64, ::Int64, ::UnitRange{Int64}, ::UnitRange{Int64})
MethodInstance for Base._print_matrix(::IOContext{Base.TTY}, ::AbstractVecOrMat{T} where T, ::String, ::String, ::String, ::String, ::String, ::String, ::Int64, ::Int64, ::UnitRange{Int64}, ::UnitRange{Int64})
julia> mi = mis[end] # pick one with abstract types in the signature
MethodInstance for Base._print_matrix(::IOContext{Base.TTY}, ::AbstractVecOrMat{T} where T, ::String, ::String, ::String, ::String, ::String, ::String, ::Int64, ::Int64, ::UnitRange{Int64}, ::UnitRange{Int64})
julia> mi.backedges
4-element Vector{Any}:
MethodInstance for Base.print_matrix(::IOContext{Base.TTY}, ::Vector{Int64}, ::String, ::String, ::String, ::String, ::String, ::String, ::Int64, ::Int64)
MethodInstance for Base.print_matrix(::IOContext{Base.TTY}, ::Matrix{Int64}, ::String, ::String, ::String, ::String, ::String, ::String, ::Int64, ::Int64)
MethodInstance for Base.print_matrix(::IOContext{Base.TTY}, ::Vector{Core.MethodInstance}, ::String, ::String, ::String, ::String, ::String, ::String, ::Int64, ::Int64)
MethodInstance for Base.print_matrix(::IOContext{Base.TTY}, ::Vector{Any}, ::String, ::String, ::String, ::String, ::String, ::String, ::Int64, ::Int64)
julia> print_tree(mi)
MethodInstance for Base._print_matrix(::IOContext{TTY}, ::AbstractVecOrMat{T} where T, ::String, ::String, ::String, ::String, ::String, ::String, ::Int64, ::Int64, ::UnitRange{Int64}, ::UnitRange{Int64})
├─ MethodInstance for Base.print_matrix(::IOContext{TTY}, ::Vector{Int64}, ::String, ::String, ::String, ::String, ::String, ::String, ::Int64, ::Int64)
│ └─ MethodInstance for Base.print_matrix(::IOContext{TTY}, ::Vector{Int64})
│ └─ MethodInstance for Base.print_array(::IOContext{TTY}, ::Vector{Int64})
│ └─ MethodInstance for show(::IOContext{TTY}, ::MIME{Symbol("text/plain")}, ::Vector{Int64})
├─ MethodInstance for Base.print_matrix(::IOContext{TTY}, ::Matrix{Int64}, ::String, ::String, ::String, ::String, ::String, ::String, ::Int64, ::Int64)
│ └─ MethodInstance for Base.print_matrix(::IOContext{TTY}, ::Matrix{Int64})
│ └─ MethodInstance for Base.print_array(::IOContext{TTY}, ::Matrix{Int64})
│ └─ MethodInstance for show(::IOContext{TTY}, ::MIME{Symbol("text/plain")}, ::Matrix{Int64})
├─ MethodInstance for Base.print_matrix(::IOContext{TTY}, ::Vector{MethodInstance}, ::String, ::String, ::String, ::String, ::String, ::String, ::Int64, ::Int64)
│ └─ MethodInstance for Base.print_matrix(::IOContext{TTY}, ::Vector{MethodInstance})
│ └─ MethodInstance for Base.print_array(::IOContext{TTY}, ::Vector{MethodInstance})
│ └─ MethodInstance for show(::IOContext{TTY}, ::MIME{Symbol("text/plain")}, ::Vector{MethodInstance})
└─ MethodInstance for Base.print_matrix(::IOContext{TTY}, ::Vector{Any}, ::String, ::String, ::String, ::String, ::String, ::String, ::Int64, ::Int64)
└─ MethodInstance for Base.print_matrix(::IOContext{TTY}, ::Vector{Any})
└─ MethodInstance for Base.print_array(::IOContext{TTY}, ::Vector{Any})
└─ MethodInstance for show(::IOContext{TTY}, ::MIME{Symbol("text/plain")}, ::Vector{Any}) |
What's the status here? |
Oh, sorry I didn't notice your response, @dlfivefifty. So, as feared the backedges do go all the way back to julia> io, a = IOBuffer(), [1,2]
(IOBuffer(data=UInt8[...], readable=true, writable=true, seekable=true, append=false, size=0, maxsize=Inf, ptr=1, mark=-1), [1, 2])
julia> @btime show($io, MIME("text/plain"), $a)
559.135 μs (303 allocations: 35.20 KiB) So the |
Why should runtime matter for printing? |
If someone is using it to dump data to a file? If you're printing 10^5 arrays you'd probably notice. But I doubt that is very common. My main point was that we have to think about runtime performance, but having thought about it I don't think it's a problem in this case. |
I’m sure they can what 50ns... |
Yes, my point too. |
I changed it to julia> mis = methodinstances(Base.print_matrix)
10-element Vector{Core.MethodInstance}:
MethodInstance for Base.print_matrix(::IOContext{Base.TTY}, ::Vector{Int64})
MethodInstance for Base.print_matrix(::IOContext{Base.TTY}, ::Matrix{Int64})
MethodInstance for Base.print_matrix(::IOContext{Base.TTY}, ::Matrix{Float64})
MethodInstance for Base.print_matrix(::IOContext{Base.TTY}, ::Vector{Core.MethodInstance})
MethodInstance for Base.print_matrix(::IOContext{Base.TTY}, ::AbstractVecOrMat{T} where T)
MethodInstance for Base.print_matrix(::IOContext{Base.TTY}, ::Vector{Int64}, ::String, ::String, ::String, ::String, ::String, ::String, ::Int64, ::Int64)
MethodInstance for Base.print_matrix(::IOContext{Base.TTY}, ::Matrix{Int64}, ::String, ::String, ::String, ::String, ::String, ::String, ::Int64, ::Int64)
MethodInstance for Base.print_matrix(::IOContext{Base.TTY}, ::Matrix{Float64}, ::String, ::String, ::String, ::String, ::String, ::String, ::Int64, ::Int64)
MethodInstance for Base.print_matrix(::IOContext{Base.TTY}, ::Vector{Core.MethodInstance}, ::String, ::String, ::String, ::String, ::String, ::String, ::Int64, ::Int64)
MethodInstance for Base.print_matrix(::IOContext{Base.TTY}, ::AbstractVecOrMat{T} where T, ::String, ::String, ::String, ::String, ::String, ::String, ::Int64, ::Int64)
julia> mi = mis[end] # pick one with abstract types in the signature
MethodInstance for Base.print_matrix(::IOContext{Base.TTY}, ::AbstractVecOrMat{T} where T, ::String, ::String, ::String, ::String, ::String, ::String, ::Int64, ::Int64)
julia> mi.backedges
ERROR: UndefRefError: access to undefined reference
Stacktrace:
[1] getproperty(x::Core.MethodInstance, f::Symbol)
@ Base ./Base.jl:33
[2] top-level scope
@ REPL[36]:1
julia> print_tree(mi)
MethodInstance for Base.print_matrix(::IOContext{TTY}, ::AbstractVecOrMat{T} where T, ::String, ::String, ::String, ::String, ::String, ::String, ::Int64, ::Int64)
julia> mis = methodinstances(Base._print_matrix)
1-element Vector{Core.MethodInstance}:
MethodInstance for Base._print_matrix(::IOContext{Base.TTY}, ::AbstractVecOrMat{T} where T, ::String, ::String, ::String, ::String, ::String, ::String, ::Int64, ::Int64, ::UnitRange{Int64}, ::UnitRange{Int64})
julia> mi = mis[end] # pick one with abstract types in the signature
MethodInstance for Base._print_matrix(::IOContext{Base.TTY}, ::AbstractVecOrMat{T} where T, ::String, ::String, ::String, ::String, ::String, ::String, ::Int64, ::Int64, ::UnitRange{Int64}, ::UnitRange{Int64})
julia> mi.backedges
ERROR: UndefRefError: access to undefined reference
Stacktrace:
[1] getproperty(x::Core.MethodInstance, f::Symbol)
@ Base ./Base.jl:33
[2] top-level scope
@ REPL[40]:1
julia> print_tree(mi)
MethodInstance for Base._print_matrix(::IOContext{TTY}, ::AbstractVecOrMat{T} where T, ::String, ::String, ::String, ::String, ::String, ::String, ::Int64, ::Int64, ::UnitRange{Int64}, ::UnitRange{Int64}) Is this also a good solution for concatenation? Probably run time penalty matters there. |
The linux32 test failure seems unrelated: ERROR: LoadError: UndefVarError: rr not defined
Stacktrace:
[1] getproperty
@ ./Base.jl:26 [inlined]
[2] rr
@ /buildworker/worker/tester_linux32/build/rr_capture.jl:24 [inlined]
[3] (::var"#1#7")(dir::String)
@ Main /buildworker/worker/tester_linux32/build/rr_capture.jl:25
[4] mktempdir(fn::var"#1#7", parent::String; prefix::String)
@ Base.Filesystem ./file.jl:729
[5] mktempdir(fn::Function, parent::String)
@ Base.Filesystem ./file.jl:727
[6] top-level scope
@ /buildworker/worker/tester_linux32/build/rr_capture.jl:17
in expression starting at /buildworker/worker/tester_linux32/build/rr_capture.jl:17 |
Thanks for getting this in, @dlfivefifty! |
(cherry picked from commit e2f4073)
(cherry picked from commit e2f4073)
This almost looks like it is adding a "secret" extendible API with the |
I can add documentation |
(cherry picked from commit e2f4073)
JuliaArrays/InfiniteArrays.jl#44
This call requires a dodgy overload
Int(::Infinity) = ∞
which is causing unnecessary latency. I can't see a reason why it was needed in the first place.