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

Reduce time to first plot by precompiling functions #2250

Merged
merged 1 commit into from
Nov 14, 2019

Conversation

yhls
Copy link
Contributor

@yhls yhls commented Nov 13, 2019

It turns out that using precompile statements generated by SnoopCompile.jl helps quite a lot! For example, this PR reduces the time to evaluate display(plot(rand(10), rand(10))) for the first time from ~22 seconds to ~12 seconds for me.

To generate precompile.jl, which is currently unused, I've included a script that uses SnoopCompile to log method compilations that take place while running the examples on the GR backend.

@@ -1,3 +1,658 @@
function _precompile_()
ccall(:jl_generating_output, Cint, ()) == 1 || return nothing
precompile(Tuple{typeof(Plots.gr_set_gradient), Plots.Series})
precompile(Tuple{typeof(Plots.gr_display), Plots.Subplot{Plots.GRBackend}, Measures.Length{:mm, Float64}, Measures.Length{:mm, Float64}, Array{Float64, 1}})
Copy link
Member

Choose a reason for hiding this comment

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

so there's no need to split gr_display?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I still need to try that, but thought I'd put what I've got up.

@yhls
Copy link
Contributor Author

yhls commented Nov 13, 2019

By the way, when I run the examples, I see lots of send: broken pipe messages. Also, @df is undefined when running some of them, like here: https://github.com/JuliaPlots/Plots.jl/blob/master/src/examples.jl#L311

@ChrisRackauckas
Copy link
Member

Also, @df is undefined when running some of them, like here:

That's from StatsPlots.jl

Copy link
Member

@ChrisRackauckas ChrisRackauckas left a comment

Choose a reason for hiding this comment

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

lgtm

@ChrisRackauckas
Copy link
Member

What's the process for maintaining the precompile file? When functions in the list change will it fail, so it needs to be re-ran?

@ViralBShah
Copy link
Contributor

The PR includes a generateprecompiles.jl file. Is deps a better place for this than test?

@ViralBShah
Copy link
Contributor

I confirm I observed the same speedup as well.

@baggepinnen
Copy link
Contributor

I see lots of send: broken pipe messages. Also, @df is undefined when running some of them, like here:

I get this error as well on the follwoing setup

(v1.3) pkg> st Plots
    Status `~/.julia/environments/v1.3/Project.toml`
  [28b8d3ca] GR v0.42.0
  [91a5bcdd] Plots v0.27.0
  [2913bbd2] StatsBase v0.32.0

The errors are always preceeded by an

Segmentation fault (core dumped)
send: Broken pipe

which seems to happen outside julia since julia happily continues on, without plotting. I can continue plotting again if I call closeall().

@timholy
Copy link
Contributor

timholy commented Nov 13, 2019

A couple of notes from my own experience:

  • precompile returns true if it succeeds and false otherwise. Even if you delete the method you're trying to precompile, only that false will tell you that a precompile directive has become obsolete (unless you delete the function altogether, of course). If you really care about making sure it doesn't regress, put each one in an @assert. Of course, this forces you to deal with the precompiles in the course of regular development, something that many developers may not want to do.
  • I now personally use @snoopi rather than @snoopc, since only the inference is cached and @snoopc doesn't even measure inference time. I just generate the list manually---perhaps becuase I've poked at precompilation itself a bit, I now like to see the consequence of each precompile directive. (Some of them don't actually "work" due to the issue in Change precompilation format to be more amenable to copying JuliaLang/julia#32705.) Plots may be big enough, though, that this isn't very practical. It might be time for an automatic script to convert the output of @snoopi.

That said, the proof is in the pudding, and if this is contributing towards a reduction in plotting time, that's a great thing!

@SimonDanisch
Copy link
Member

By how much does this increase load times?
Also note, if you check in precompile files, that they may be highly OS, Julia and package version dependend, so without a try catch you will get errors on different machines

@ViralBShah
Copy link
Contributor

ViralBShah commented Nov 13, 2019

For me, @time using Plots went up from 2s to 4s.

I'll also note, that @yhls gave me a few more precompiles for Base and GR.jl. With those I am able to get the first plot in 8.3s.

Copy link
Member

@daschw daschw left a comment

Choose a reason for hiding this comment

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

Thanks a lot @yhls! This is so amazing!

@@ -0,0 +1,26 @@
# To figure out what should be precompiled, run this script, then move
Copy link
Member

Choose a reason for hiding this comment

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

Could we move this file to deps instead of test as @ViralBShah suggested?
Would it make sense to have a Project.toml in deps then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. I thought Project.toml files were just for the top level of a project/package, or am I missing something?

Copy link
Member

Choose a reason for hiding this comment

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

For someone without StatsPlots or RDataSets installed the test_examples 25 and 30 would still fail here. Furthermore, example 6 fails if FileIO is not installed.
Hence, I was thinking we could add a Project.toml with these dependencies, so people could just do pkg> activate deps before running the generateprecompiles.jl script.
However, this does not work anyway, because in @snoopc a new julia process with the default project is started.
Maybe we should add a comment here that StatsPlots, RDataSets, FileIO and (obviously) SnoopCompile have to be installed to run this script.

test/generateprecompiles.jl Outdated Show resolved Hide resolved
test/generateprecompiles.jl Outdated Show resolved Hide resolved
@SimonDanisch
Copy link
Member

Also note, if you check in precompile files, that they may be highly OS, Julia and package version dependend, so without a try catch you will get errors on different machines

I just saw, that there are only Plots.jl functions checked into the precompile file, so I guess that will be much lesser of an issue...
Glad that including such a precompile file does actually improve something :)

@yhls
Copy link
Contributor Author

yhls commented Nov 13, 2019

  • Thanks @ChrisRackauckas, I've fixed the examples with @df.
  • @ViralBShah, good point with putting generateprecompiles.jl in deps, done.
  • @baggepinnen Hmm, I don't get any segfaults, despite the broken pipes.
  • @timholy, thanks for your notes. Much of the time is spent on lots of little methods, so we probably want to stick with automatically generated precompiles.
  • I also now include precompiles for anonymous functions like #attr!##kw that have to do with keyword arguments, which should be named reproducibly. For now, I've done this by changing the regex that SnoopCompile uses to throw away anonymous functions to r"#{1,2}[^\"#]+#{1,2}\d+", but depending on whether SnoopCompile adopts something like this, we may have to modify generateprecompiles.jl. (See bottom of Status of SnoopCompile timholy/SnoopCompile.jl#29)

@daschw daschw merged commit 2245e57 into JuliaPlots:master Nov 14, 2019
@non-Jedi
Copy link

I think you might want to more carefully measure the performance effect here. Earlier this year, I submitted a PR to Gadfly to use snoopcompile-generated precompile statements. What I found there was that while it did reduce the @time plot(...) by 2-4 seconds for most of the Gadfly test-suite, it universally added 4 seconds to @time using Gadfly.

See the graphs at GiovineItalia/Gadfly.jl#1280 (comment) and GiovineItalia/Gadfly.jl#1280

@daschw
Copy link
Member

daschw commented Nov 16, 2019

Thanks for the hint, @non-Jedi !
@ViralBShah posted above that for him @time using Plots went up by 2-4 seconds. I think considering the reduction of 10-12 seconds in @time display(plot(...)) for the first plot, this still is a good deal.

@daschw
Copy link
Member

daschw commented Nov 16, 2019

Am I right that Revise does not work anymore for Plots development with these changes? That's pretty annoying. Is there anything that can be done about that?

@timholy
Copy link
Contributor

timholy commented Nov 16, 2019

Aside from timholy/Revise.jl#316 (and fix coming in timholy/Revise.jl#395), there is no real reason it shouldn't work. The backend files that don't get automatically included will need special handling, though. Aside from manually calling Revise.track, two options seem to be:

  • add each backend file as an include_dependency. For people not using Revise, one downside is that any edits to those files will require recompilation of the whole package.
  • have the backend-loading code call Revise.track directly if Revise is loaded. (You already use Requires so this isn't impossible.)

@ViralBShah
Copy link
Contributor

ViralBShah commented Nov 16, 2019

I should note that there are accompanying additions to precompile statements in Base and GR.jl that will give another slight bump in time to first plot. My timings for increased load time of Plots.jl from 2 to 4 seconds included these additional precompiles in GR.jl, and the Base system image having the additional precompiles.

@yhls should be able to say more.

@daschw
Copy link
Member

daschw commented Nov 18, 2019

Thanks a lot for the hints for conditionally loaded backends @timholy ! I just tested again, and you are right, Revise works with just fine with Plots. I'm not sure what the issue was two days ago.

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.

9 participants