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

show_default, but without type parameters #36263

Open
goretkin opened this issue Jun 12, 2020 · 8 comments
Open

show_default, but without type parameters #36263

goretkin opened this issue Jun 12, 2020 · 8 comments
Labels
display and printing Aesthetics and correctness of printed representations of objects.

Comments

@goretkin
Copy link
Contributor

Definitions like

show(io::IO, r::OneTo) = print(io, "Base.OneTo(", r.stop, ")")
could be no more than something like

show(io::IO, r::OneTo) = show_default(io, r; type_parameters=false)

There are many places where that's a default behavior that is wanted. For example, when all the type parameters correspond to fields that when show'd reveal their type. For example

struct Foo{T}
  x::T
end

show(Foo(Base.OneTo(3)))

produces

Foo{Base.OneTo{Int64}}(Base.OneTo(3)),
which is unnecessary.

Also, show_default has great logic to use IOContext to see which names need to be qualified or not:

julia> Base.show_default(stdout, Base.OneTo(3))
Base.OneTo{Int64}(3)
julia> Base.show_default(stdout, Foo(Base.OneTo(3)))
Foo{Base.OneTo{Int64}}(Base.OneTo(3))
julia> using Base: OneTo

julia> Base.show_default(stdout, Base.OneTo(3))
OneTo{Int64}(3)
julia> Base.show_default(stdout, Foo(Base.OneTo(3)))
Foo{OneTo{Int64}}(Base.OneTo(3))

Notice the inconsistency in Foo{OneTo{Int64}}(Base.OneTo(3)).

x-ref: #33260

@JeffBezanson JeffBezanson added the display and printing Aesthetics and correctness of printed representations of objects. label Jun 13, 2020
@goretkin
Copy link
Contributor Author

goretkin commented Jun 13, 2020

Seems suspiciously relevant

julia/base/show.jl

Lines 477 to 483 in 3222aba

function print_without_params(@nospecialize(x))
if isa(x,UnionAll)
b = unwrap_unionall(x)
return isa(b,DataType) && b.name.wrapper === x
end
return false
end
but doesn't quite fit the bill where it's used.

One idea is a trait-based API, where a type could register "when you show me, my type is obvious", but that would require more careful design since it would likely be public.

goretkin added a commit to goretkin/julia that referenced this issue Jun 13, 2020
@goretkin
Copy link
Contributor Author

@timholy
Copy link
Member

timholy commented Nov 17, 2020

If a change like this is made, it shouldn't be done without an awareness that at least until recently show_default was a major invalidation risk. One would want to run the tests in #37193, which had to be reverted (see #38461), to make sure this kind of change doesn't cause unintended problems.

@timholy
Copy link
Member

timholy commented Nov 17, 2020

Also,

julia> r = Base.OneTo(3)
Base.OneTo(3)

julia> io = IOBuffer();

julia> @btime show($io, $r)
  135.656 ns (2 allocations: 96 bytes)

julia> @btime Base.show_default($io, $r)
  345.577 μs (44 allocations: 21.73 KiB)

A 2000x performance regression is not something to embrace lightly.

@bramtayl
Copy link
Contributor

Is there a way to make show_default more performant?

@goretkin
Copy link
Contributor Author

A 2000x performance regression is not something to embrace lightly.

I agree. I suspect this is due to "show_default has great logic to use IOContext to see which names need to be qualified or not" and it also has logic to print circular references.

I notice that the definitions prevent specializing compiled code to the type of x:

julia/base/show.jl

Lines 390 to 397 in 814945c

show(io::IO, @nospecialize(x)) = show_default(io, x)
show(x) = show(stdout::IO, x)
# avoid inferring show_default on the type of `x`
show_default(io::IO, @nospecialize(x)) = _show_default(io, inferencebarrier(x))
function _show_default(io::IO, @nospecialize(x))

It seems reasonable that specializing on the type would abate that performance penalty, at the cost of introducing a compilation penalty.

@timholy
Copy link
Member

timholy commented Jan 21, 2021

It seems reasonable that specializing on the type would abate that performance penalty

When the type is known to inference, yes. When the type is not known to inference (e.g., printing elements of a Vector{Any}) it would have precisely the opposite effect. Currently

julia> using MethodAnalysis

julia> methodinstances(Base._show_default)
3-element Vector{Core.MethodInstance}:
 MethodInstance for _show_default(::IOContext{IOBuffer}, ::Any)
 MethodInstance for _show_default(::IOBuffer, ::Any)
 MethodInstance for _show_default(::IO, ::Any)

so you only have to dispatch on the io argument. If you allow specialization on the type, now you'll go from 3 MethodInstances to hundreds or thousands, and now you have to runtime dispatch on the type argument, and that will be bad for runtime performance. Given that it would also murder latency (it's an expensive method to compile because it's complicated), I think it's very unlikely that this would be a win overall. This is a textbook case for @nospecialize.

@timholy
Copy link
Member

timholy commented Jan 21, 2021

I suspect this is due to "show_default has great logic to use IOContext to see which names need to be qualified or not" and it also has logic to print circular references.

Don't guess, profile. 99% of the time is spent on this line:

show(io, inferencebarrier(t))

It ends up spending all of its time in calling show_typealias. Most of its time is spent looking up names in modules and deciding whether there's a corresponding isdefined object, whether the name is deprecated, and whether it's const.

A bit of memoization, if that would be safe, might be quite effective here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
display and printing Aesthetics and correctness of printed representations of objects.
Projects
None yet
Development

No branches or pull requests

4 participants