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

Make UnicodePlots an optional dependency #49

Closed
andreasnoack opened this issue Feb 14, 2023 · 7 comments · Fixed by #51
Closed

Make UnicodePlots an optional dependency #49

andreasnoack opened this issue Feb 14, 2023 · 7 comments · Fixed by #51
Assignees

Comments

@andreasnoack
Copy link

This package is an indirect dependency of ours and UnicodePlots adds more than eight seconds to the load time of our package so it would be helpful if UnicodePlots could be made an weak dependency as described in https://pkgdocs.julialang.org/dev/creating-packages/#Transition-from-normal-dependency-to-extension

@chriselrod
Copy link

chriselrod commented Feb 15, 2023

Using @time_imports, I get

  20394.7 ms  UnicodePlots
      2.1 ms  UnicodePlots  UnitfulExt
     19.5 ms  CategoricalDistributions

That's 20 seconds loading UnicodePlots. That dependency is not worth it.

@ablaom
Copy link
Member

ablaom commented Feb 15, 2023

I'd support this change which makes sense.

We keep the first show and the second goes into the UnicodePlots glue code:

Base.show(io::IO, mime::MIME"text/plain",

This the only place UnicodePlots is used.

I can't make this a priority just now, but can promise a timely review if someone wants to make a PR.

@t-bltg
Copy link

t-bltg commented Feb 15, 2023

That's 20 seconds loading UnicodePlots.

This is weird, it should be around 4 s. On what OS is that ?

JuliaGraphics/ColorVectorSpace.jl#186 will also improve load times by reducing dependencies (merged, but not yet released).

@chriselrod
Copy link

chriselrod commented Feb 16, 2023

Fedora, Linux.
In a fresh Julia session with --startup=no, I get

julia> @time using UnicodePlots
  2.875718 seconds (5.95 M allocations: 346.315 MiB, 11.14% gc time, 3.13% compilation time: 24% of which was recompilation)

When using my startup, it is similar (recompilation goes up as a percent of compilation time, but compilation time goes down from its already low value)

julia> @time using UnicodePlots
  2.868708 seconds (5.85 M allocations: 337.308 MiB, 10.91% gc time, 1.19% compilation time: 65% of which was recompilation)

The 20s comes from @time_imports when loading other packages that depend on CategoricalArrays.

@t-bltg
Copy link

t-bltg commented Feb 16, 2023

Thanks, I'd be interested in reading suggestions on how to make the UnicodePlots experience smoother.

There is some latency involved by loading compiled code generated by precompile statements (tracked by JuliaPlots/UnicodePlots.jl#324, with no immediate solution (except disabling the precompilation workload with the effect of worsening TTFP)):

$ julia
julia> VERSION
v"1.9.0-beta4"
julia> using SnoopPrecompile, Preferences
julia> Preferences.set_preferences!(SnoopPrecompile, "skip_precompile" => ["UnicodePlots"]; force = true)
julia> exit(0)
$ julia -e 'using UnicodePlots'  # precompile
$ julia
julia> @time using UnicodePlots
  2.536887 seconds (3.35 M allocations: 230.944 MiB, 6.89% gc time, 8.91% compilation time: 17% of which was recompilation)

# previously, with precompile workload: 
  4.223118 seconds (5.61 M allocations: 368.600 MiB, 6.43% gc time, 4.18% compilation time: 22% of which was recompilation)

Some recent improvements (2s, 32%) have been made in JuliaPlots/UnicodePlots.jl#332 following JuliaPlots/UnicodePlots.jl#331 using package extensions - weak deps.

IMO the load time in the range [2s, 4s] is reasonable for a plotting package, but this is subjective (tradeoff for TTFP) and feedback would be appreciated.

@ablaom
Copy link
Member

ablaom commented Feb 16, 2023

@t-bltg Thanks for the update. It's reassuring to know there is clearly a concerted effort to optimise. I don't have any suggestions on how to improve things there.

I think the case for making UnicodePlots a weak dependency of CategoricalDistributions remains strong. It's only there for pretty printing and CategoricalDistributions will sit pretty low in the package food chain.

@ablaom ablaom changed the title Make UnicodePlots and optional dependency Make UnicodePlots an optional dependency Feb 23, 2023
@ablaom
Copy link
Member

ablaom commented Feb 23, 2023

Note to self:

Current load time for UnicodePlots 3.4.0 is about 13 times that of Distributions 25.82 in a @time_imports using CategoricalDistributions benchmark. Julia 1.8.5.

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 a pull request may close this issue.

4 participants