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

Delete show(::IO, ::Type) specialization #67

Merged
merged 1 commit into from
Jul 1, 2020

Conversation

timholy
Copy link
Member

@timholy timholy commented Jul 1, 2020

Overloading show for types is strongly discouraged. I'm currently tracking down a Julia segfault that's caused by this line.

Refs:

Overloading `show` for types is strongly discouraged. I'm currently tracking down a Julia segfault that's caused by this line.

Refs:
- JuliaMath/FixedPointNumbers.jl#11
- PainterQubits/Unitful.jl#321
@SimonDanisch
Copy link
Member

Oh, oh....
I remember getting that recommendation for Julia 0.4, and then I thought it was fixed/improved for ~1.0, so I started going for it again :D Anyways, if this is what it needs to not get segfaults, I gladly accept this PR :)
I actually thought I overload other types as well, but seems I only overloaded summary.. Maybe I should also do that for TupleView

@SimonDanisch SimonDanisch merged commit 29f1fa6 into master Jul 1, 2020
@SimonDanisch SimonDanisch deleted the teh/remove_show_type branch July 1, 2020 10:04
@timholy
Copy link
Member Author

timholy commented Jul 1, 2020

Yeah, it's weird, this only started happening this morning but I've tried to bisect back and it's affecting master at least as of Saturday morning, but I was messing around with Plots pretty recently and it should have triggered then. Maybe I got it via a package update?

@timholy
Copy link
Member Author

timholy commented Jul 1, 2020

but seems I only overloaded summary.. Maybe I should also do that for TupleView

Do you know about showarg?

help?> Base.showarg
  showarg(io::IO, x, toplevel)

  Show x as if it were an argument to a function. This function is used by summary to display type information in terms of sequences of function calls on objects. toplevel is true if this is the direct
  call from summary and false for nested (recursive) calls.

  The fallback definition is to print x as "::$(typeof(x))", representing argument x in terms of its type. (The double-colon is omitted if toplevel=true.) However, you can specialize this function for
  specific types to customize printing.

  Example
  ≡≡≡≡≡≡≡≡≡

  A SubArray created as view(a, :, 3, 2:5), where a is a 3-dimensional Float64 array, has type

  SubArray{Float64,2,Array{Float64,3},Tuple{Colon,Int64,UnitRange{Int64}},false}

  The default show printing would display this full type. However, the summary for SubArrays actually prints as

  2×4 view(::Array{Float64,3}, :, 3, 2:5) with eltype Float64

  because of a definition similar to

  function Base.showarg(io::IO, v::SubArray, toplevel)
      print(io, "view(")
      showarg(io, parent(v), false)
      print(io, ", ", join(v.indices, ", "))
      print(io, ')')
      toplevel && print(io, " with eltype ", eltype(v))
  end

  Note that we're calling showarg recursively for the parent array type, indicating that any recursed calls are not at the top level. Printing the parent as ::Array{Float64,3} is the fallback
  (non-toplevel) behavior, because no specialized method for Array has been defined.

@timholy
Copy link
Member Author

timholy commented Jul 1, 2020

I can go back as far as JuliaLang/julia@65c2a03 and the original still segfaults. Before that, the functionality I'm testing wasn't possible. So it must have been via a package update.

Could we perhaps get a release with this? As soon as JuliaLang/julia#35877 merges (not sure when that will happen), I'll want to get my blog post on invalidations out, and anyone who tries the new tools (which BTW are already publicly available as of SnoopCompile 1.7, see the dev docs for SnoopCompile) will get a segfault if they try the obvious target of Plots (since it's the historic TTFP benchmark).

EDIT: I don't necessarily recommend that one go hunting for invalidations until that PR from Jeff merges. Or you can update manually, see which commits you want in JuliaPlots/Plots.jl#2835 (comment).

@SimonDanisch
Copy link
Member

Already tagged: JuliaRegistries/General#17284

@timholy
Copy link
Member Author

timholy commented Jul 1, 2020

BTW, for your interest (I'm affected by JuliaGL/GLFW.jl#198 so the display part errors for me):

julia-1.4:

julia> @time using Makie; @time p = plot(rand(10)); @time display(p)
 11.100282 seconds (23.52 M allocations: 1.236 GiB, 3.75% gc time)
 15.379420 seconds (39.29 M allocations: 1.980 GiB, 3.87% gc time)

That customized julia-1.6 build in JuliaPlots/Plots.jl#2835 (comment):

julia> @time using Makie; @time p = plot(rand(10)); @time display(p)
  6.123158 seconds (11.53 M allocations: 705.654 MiB, 3.68% gc time)
 16.065592 seconds (40.85 M allocations: 2.274 GiB, 5.15% gc time)

I've profiled it, and most of your time in the plot command is for inference. I'd recommend waiting until Jeff's invalidation/ambiguity PR merges and then try SnoopCompile again. I know you tried it before and it didn't really help, but a lot has changed since then. Many (not all) of the previous failures of SnoopCompile to make much difference seem to have been due to invalidations; the situation is better now and you can diagnose what's going wrong more easily.

On that Julia 1.6 branch you're down from an estimated 6000 invalidations (JuliaLang/www.julialang.org#794, though there might be some mistakes in that estimate) to ~500. That's why, once all that work merges, it will be time to try again.

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.

2 participants