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.

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

Merged
merged 15 commits into from
Jan 4, 2021
Merged

Conversation

dlfivefifty
Copy link
Contributor

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.

@dlfivefifty dlfivefifty changed the title Remove Int call Remove Int call in size(::SubArray) Sep 25, 2020
@dlfivefifty
Copy link
Contributor Author

Alternatively it could be a call to Base.to_shape but as the tests are passing I assume that's unnecessary?

@KristofferC
Copy link
Member

The Int comes from 7711aef#diff-c8c9f86af939c6f462b38975999f448cR50 cc @timholy

@timholy
Copy link
Member

timholy commented Sep 25, 2020

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 Integer subtype and see whether it increases badly when you eliminate the Int.

@dlfivefifty
Copy link
Contributor Author

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}

@pabloferz
Copy link
Contributor

pabloferz commented Sep 25, 2020

I'm afraid this more recent one might also be relevant for JuliaArrays/InfiniteArrays.jl#44

@dlfivefifty
Copy link
Contributor Author

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...

@timholy
Copy link
Member

timholy commented Sep 25, 2020

But it's also pretty important to avoid invalidation. Do we really need to support arrays that don't have Int dimensions? Is that really an array or is it a DiscreteFunction?

@dlfivefifty
Copy link
Contributor Author

We do if you want ApproxFun to continue to work. InfiniteArrays.jl is working quite well right now so suddenly insisting arrays have Int sizes, just for a faster concatenation implementation is a significant breaking change for very little reward.

In any case, adding ::Int everywhere is a clear case of "code smell". I don't really understand why it's not just mapreduce(length,+,...)

@timholy
Copy link
Member

timholy commented Sep 25, 2020

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.

@dlfivefifty
Copy link
Contributor Author

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 a.

@timholy
Copy link
Member

timholy commented Sep 26, 2020

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 cat pipeline (#37741 (comment)) moreso than this subarray one.

@timholy
Copy link
Member

timholy commented Sep 26, 2020

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.

@dlfivefifty
Copy link
Contributor Author

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

@dlfivefifty
Copy link
Contributor Author

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 UnitRange{Int}(...) is called which causes problems for both infinite arrays and BigInt sized arrays)

@timholy
Copy link
Member

timholy commented Sep 26, 2020

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 cat it's actually pretty hard to avoid some form of inference trouble.)

@dlfivefifty dlfivefifty changed the title Remove Int call in size(::SubArray) Fixes for non-Int based lengths Nov 18, 2020
@dlfivefifty
Copy link
Contributor Author

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 [1; 1:BigInt(10)] to error out, see #37741. Hopefully this doesn't cause too many issues with invalidation.

@timholy
Copy link
Member

timholy commented Nov 30, 2020

@dlfivefifty, note test failures.

@dlfivefifty
Copy link
Contributor Author

@timholy It doesn't look like the test failures are caused by this PR, unless I'm not understanding.

@KristofferC
Copy link
Member

KristofferC commented Nov 30, 2020

Try rebase on master where CI should be improved.

@timholy
Copy link
Member

timholy commented Dec 17, 2020

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 _print_matrix (which doesn't exist on master).

@dlfivefifty
Copy link
Contributor Author

dlfivefifty commented Dec 18, 2020

Below is what I get. Should I try again with Base.invokelast?

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})

@KristofferC KristofferC mentioned this pull request Dec 19, 2020
26 tasks
@KristofferC
Copy link
Member

What's the status here?

@timholy
Copy link
Member

timholy commented Dec 27, 2020

Oh, sorry I didn't notice your response, @dlfivefifty.

So, as feared the backedges do go all the way back to show. I'd recommend invokelatest; thoughts, @KristofferC? There's a bit of a runtime penalty (~50ns), but to me that seems nowhere near the problem of invalidation given that

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 invokelatest is 1 part in 10^4.

@dlfivefifty
Copy link
Contributor Author

Why should runtime matter for printing?

@timholy
Copy link
Member

timholy commented Dec 27, 2020

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.

@dlfivefifty
Copy link
Contributor Author

I’m sure they can what 50ns...

@timholy
Copy link
Member

timholy commented Dec 27, 2020

Yes, my point too.

@dlfivefifty
Copy link
Contributor Author

I changed it to Base.invokelatest and to my untrained eye it looks better:

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.

@dlfivefifty
Copy link
Contributor Author

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

@timholy timholy merged commit e2f4073 into JuliaLang:master Jan 4, 2021
@timholy
Copy link
Member

timholy commented Jan 4, 2021

Thanks for getting this in, @dlfivefifty!

KristofferC pushed a commit that referenced this pull request Jan 5, 2021
@KristofferC KristofferC removed the backport 1.6 Change should be backported to release-1.6 label Jan 6, 2021
staticfloat pushed a commit that referenced this pull request Jan 15, 2021
@KristofferC
Copy link
Member

This almost looks like it is adding a "secret" extendible API with the oneto and unitrange methods. It doesn't feel sustainable that people "hack in" their own undocumented extensions into the language.

@dlfivefifty
Copy link
Contributor Author

I can add documentation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants