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

Improve inference broadly throughout REPL #37081

Merged
merged 6 commits into from Aug 26, 2020
Merged

Improve inference broadly throughout REPL #37081

merged 6 commits into from Aug 26, 2020

Conversation

timholy
Copy link
Sponsor Member

@timholy timholy commented Aug 17, 2020

This attempts to improve inference in REPL. There are several pieces:

  • type-stable interfaces have beend added for all abstract types REPLCompletions.Completion and TerminalMenus.AbstractMenu
  • by defining the Options struct prior to creating the LineEdit module, we gain the ability to improve many inference results
  • several methods assume the presence of fields that are not guaranteed for all subtypes. Restrict to subtypes that have the requisite fields.
  • because REPL is compiled with low optimization settings, to prevent loss of type information it's helpful to declare more types for input arguments on internal methods.
  • in a couple of places, avoiding unnecessary Unions makes later steps easier
  • many implementations were improved. In most or perhaps all places I removed return-type annotations (left from previous attempts to enforce a minimal level of inference accuracy) since they are no longer necessary.

This does a lot to make REPL more inferrable and less vulnerable to invalidation, but if you check out the precompile statements that get issued during bootstrap, REPL is still the standout "bad actor" with regards to non-inferrable code. (Especially after some of my other PRs from today.)

@timholy
Copy link
Sponsor Member Author

timholy commented Aug 18, 2020

OK, this grew into a much bigger deal. This should fix inference throughout essentially all of REPL.

@@ -358,7 +358,7 @@ function pipe_reader end
function pipe_writer end

write(io::AbstractPipe, byte::UInt8) = write(pipe_writer(io)::IO, byte)
unsafe_write(io::AbstractPipe, p::Ptr{UInt8}, nb::UInt) = unsafe_write(pipe_writer(io)::IO, p, nb)
unsafe_write(io::AbstractPipe, p::Ptr{UInt8}, nb::UInt) = unsafe_write(pipe_writer(io)::IO, p, nb)::Union{Int,UInt}
Copy link
Sponsor Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Methods for Union{Core.CoreSTDERR,Core.CoreSTDOUT} and Base.FileSystem.File return UInt, the rest (I think) return Int. Should we make them all one or the other?

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not to derail the main point too much, but for overall context:

julia> for (m, rt) in zip(methods(write, (IO, Any)), Base.return_types(write, (IO, Any)))
           println(m => rt)
       end
write(io::Union{Core.CoreSTDERR, Core.CoreSTDOUT}, x::UInt8) in Base at coreio.jl:26 => Int64
write(s::IO, x::Union{Float16, Float32, Float64, Int128, Int16, Int32, Int64, UInt128, UInt16, UInt32, UInt64}) in Base at io.jl:649 => Any
write(io::IO, s::Union{SubString{String}, String}) in Base at strings/io.jl:185 => Int64
write(io::IO, s::Base.CodeUnits) in Base at strings/basic.jl:744 => Int64
write(to::IO, p::Ptr) in Base at io.jl:654 => Any
write(io::IO, x::Enum{T}) where T<:Integer in Base.Enums at Enums.jl:21 => Any
write(s::IO, a::SubArray{T,N,var"#s68",I,L} where L where I where var"#s68"<:Array) where {T, N} in Base at io.jl:675 => Any
write(s::IO, z::Rational) in Base at rational.jl:94 => Int64
write(io::IO, s::AbstractString) in Base at strings/io.jl:181 => Int64
write(s::IO, a::Array) in Base at io.jl:667 => Any
write(s::IO, x::Ref{T}) where T in Base at io.jl:647 => Any
write(s::IO, z::Complex) in Base at complex.jl:220 => Int64
write(s::IO, B::BitArray) in Base at bitarray.jl:1823 => Any
write(s::IO, A::AbstractArray) in Base at io.jl:656 => Any
write(s::IO, x::Bool) in Base at io.jl:653 => Any
write(io::IO, s::Symbol) in Base at io.jl:708 => Any
write(io::IO, c::Char) in Base at io.jl:696 => Int64
write(io::IO, cred::LibGit2.GitCredential) in LibGit2 at /home/tim/src/julia-master/usr/share/julia/stdlib/v1.6/LibGit2/src/gitcredential.jl:94 => Nothing
write(io::IO, s::Base.SecretBuffer) in Base at secretbuffer.jl:126 => Any
write(s::IO, x::Int8) in Base at io.jl:648 => Any
write(io::Base.AbstractPipe, byte::UInt8) in Base at io.jl:360 => Any
write(to::Base.GenericIOBuffer, from::Base.GenericIOBuffer) in Base at iobuffer.jl:408 => Int64
write(to::IO, from::IO) in Base at io.jl:713 => Any
write(to::Base.GenericIOBuffer, a::UInt8) in Base at iobuffer.jl:445 => Int64
write(pipe::Base64.Base64EncodePipe, x::UInt8) in Base64 at /home/tim/src/julia-master/usr/share/julia/stdlib/v1.6/Base64/src/encode.jl:98 => Int64
write(io::Base.SecretBuffer, b::UInt8) in Base at secretbuffer.jl:112 => Int64
write(s::Base.BufferStream, b::UInt8) in Base at stream.jl:1351 => Int64
write(s::Base.LibuvStream, b::UInt8) in Base at stream.jl:1091 => Int64
write(f::Base.Filesystem.File, c::UInt8) in Base.Filesystem at filesystem.jl:136 => UInt64
write(::Base.DevNull, ::UInt8) in Base at coreio.jl:16 => Int64
write(s::IOStream, b::UInt8) in Base at iostream.jl:366 => Int64
write(s::IO, x::UInt8) in Base at io.jl:224 => Union{}
write(io::IO, x) in Base at io.jl:635 => Union{}

@timholy timholy force-pushed the teh/repl branch 2 times, most recently from 52ecf01 to f46a78a Compare August 19, 2020 09:18
@timholy
Copy link
Sponsor Member Author

timholy commented Aug 19, 2020

I expect this version should finally pass tests, as on the previous run only 32-bit was failing. Here, for completion methods I've changed typeof(ret[2]) from UnitRange{Int64} to UnitRange{Int}. That choice in #26930 did not receive any discussion that I can see, so I'm going with the view that it wasn't particularly deliberate. Really making it work properly with Int64 (which we do for some file-position methods even on 32-bit) would require more extensive changes, and I doubt it's relevant for REPL completions.

timholy added a commit that referenced this pull request Aug 21, 2020
Moving this earlier in the load sequence allows one to annotate the
return type of some methods.
timholy added a commit that referenced this pull request Aug 21, 2020
…rt of #37081)

Since MIState and others have containers with abstract typing, these interfaces
fix inference problems broadly.
timholy added a commit that referenced this pull request Aug 21, 2020
Since entire modules are marked `@nospecialize`, we need
to declare argument types anywhere we want good inference.
This also improves numerous implementations to ensure
inferrability.

For the completion methods, notice this changes typeof(ret[2]) from
UnitRange{Int64} to UnitRange{Int}. Internally, the methods are using
Int rather than Int64, so consistently supporting Int64 would require
more extensive changes.
timholy added a commit that referenced this pull request Aug 21, 2020
timholy added a commit that referenced this pull request Aug 25, 2020
Moving this earlier in the load sequence allows one to annotate the
return type of some methods.
timholy added a commit that referenced this pull request Aug 25, 2020
…rt of #37081)

Since MIState and others have containers with abstract typing, these interfaces
fix inference problems broadly.
timholy added a commit that referenced this pull request Aug 25, 2020
Since entire modules are marked `@nospecialize`, we need
to declare argument types anywhere we want good inference.
This also improves numerous implementations to ensure
inferrability.

For the completion methods, notice this changes typeof(ret[2]) from
UnitRange{Int64} to UnitRange{Int}. Internally, the methods are using
Int rather than Int64, so consistently supporting Int64 would require
more extensive changes.
timholy added a commit that referenced this pull request Aug 25, 2020
@timholy timholy added the stdlib:REPL Julia's REPL (Read Eval Print Loop) label Aug 25, 2020
@timholy
Copy link
Sponsor Member Author

timholy commented Aug 25, 2020

I'm aware that this is a monster to review and that there aren't that many people who work on the REPL, so if necessary I'm happy to take responsibility for problems this causes.

Copy link
Sponsor Member

@KristofferC KristofferC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not super familiar with the REPL code but looks good to me from what I can see.

Moving this earlier in the load sequence allows one to annotate the
return type of some methods.
…rt of #37081)

Since MIState and others have containers with abstract typing, these interfaces
fix inference problems broadly.
Since entire modules are marked `@nospecialize`, we need
to declare argument types anywhere we want good inference.
This also improves numerous implementations to ensure
inferrability.

For the completion methods, notice this changes typeof(ret[2]) from
UnitRange{Int64} to UnitRange{Int}. Internally, the methods are using
Int rather than Int64, so consistently supporting Int64 would require
more extensive changes.
@timholy timholy merged commit 69eadbc into master Aug 26, 2020
@timholy timholy deleted the teh/repl branch August 26, 2020 15:38
aviatesk added a commit to JunoLab/FuzzyCompletions.jl that referenced this pull request Aug 26, 2020
@aviatesk
Copy link
Sponsor Member

As for completions part: the changes looks good (I gave it a quick try and even made equivalent changes for FuzzyCompletions.jl, and it seems to work great)

aviatesk added a commit to JunoLab/FuzzyCompletions.jl that referenced this pull request Aug 26, 2020
@timholy
Copy link
Sponsor Member Author

timholy commented Aug 26, 2020

@ChrisRackauckas it's really fun to be able to load DifferentialEquations and have your next tab-complete not have a 0.25s latency.

@ChrisRackauckas
Copy link
Member

you're the hero we needed.

@timholy
Copy link
Sponsor Member Author

timholy commented Aug 26, 2020

Also fun: Julia 1.5:

julia> @time using DifferentialEquations
 22.845265 seconds (61.37 M allocations: 3.389 GiB, 4.10% gc time)

Today's master branch:

julia> @time using DifferentialEquations
  6.944444 seconds (21.22 M allocations: 1.442 GiB, 5.81% gc time)

simeonschaub pushed a commit to simeonschaub/julia that referenced this pull request Aug 29, 2020
* REPL: move Options to improve inference

Moving this earlier in the load sequence allows one to annotate the
return type of some methods.

* REPL: add interfaces for abstract types and change one field type

Since MIState and others have containers with abstract typing, these interfaces
fix inference problems broadly.

* IO: improve inferrability of write and unsafe_write

* REPL: add argument typing and improve implementations

Since entire modules are marked `@nospecialize`, we need
to declare argument types anywhere we want good inference.
This also improves numerous implementations to ensure
inferrability.

For the completion methods, notice this changes typeof(ret[2]) from
UnitRange{Int64} to UnitRange{Int}. Internally, the methods are using
Int rather than Int64, so consistently supporting Int64 would require
more extensive changes.

* REPL: add argtypes to keymap functions

* REPL: test inference in LineEdit
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:latency Compiler latency stdlib:REPL Julia's REPL (Read Eval Print Loop)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants