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

strip out old precompilation code #958

Merged
merged 1 commit into from
Feb 22, 2017
Merged

Conversation

tlnagy
Copy link
Member

@tlnagy tlnagy commented Feb 22, 2017

AFAIK this isn't necessary anymore in recent versions of Julia. I can't detect any regressions and this dramatically increases precompilation speed:

Master:

julia> @time Base.compilecache("Gadfly")
INFO: Recompiling stale cache file /Users/tamasnagy/.julia/lib/v0.5/Gadfly.ji for module Gadfly.
 32.664419 seconds (310.71 k allocations: 12.912 MB)
"/Users/tamasnagy/.julia/lib/v0.5/Gadfly.ji"

julia> @time include("../test/points.jl")
  6.441102 seconds (8.14 M allocations: 338.343 MB, 5.44% gc time)

julia> @time include("../test/points.jl")
  0.005091 seconds (649 allocations: 56.553 KB)

This PR:

julia> @time Base.compilecache("Gadfly")
INFO: Recompiling stale cache file /Users/tamasnagy/.julia/lib/v0.5/Gadfly.ji for module Gadfly.
  7.503891 seconds (310.71 k allocations: 12.912 MB)
"/Users/tamasnagy/.julia/lib/v0.5/Gadfly.ji"

julia> @time include("../test/points.jl")
  6.620339 seconds (8.52 M allocations: 352.255 MB, 4.56% gc time)

julia> @time include("../test/points.jl")
  0.004820 seconds (649 allocations: 56.553 KB)

@tlnagy tlnagy mentioned this pull request Feb 22, 2017
@tlnagy
Copy link
Member Author

tlnagy commented Feb 22, 2017

@bjarthur, @Mattriks, @shashi can you guys checkout this PR and give it a whirl? I wasn't able to get it to break on julia 0.5. (0.6 is broken regardless).

AFAIK this isn't necessary anymore in recent versions of Julia
@codecov-io
Copy link

Codecov Report

Merging #958 into master will increase coverage by 14.42%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master     #958       +/-   ##
===========================================
+ Coverage   65.47%   79.89%   +14.42%     
===========================================
  Files          35       34        -1     
  Lines        4814     3930      -884     
===========================================
- Hits         3152     3140       -12     
+ Misses       1662      790      -872
Impacted Files Coverage Δ
src/Gadfly.jl 74.13% <ø> (+0.05%)
src/bincount.jl 94.84% <ø> (-1.08%)
src/statistics.jl 90.58% <ø> (-0.2%)
src/geom/subplot.jl 94.15% <ø> (-0.08%)
src/guide.jl 84.18% <ø> (+0.09%)
src/aesthetics.jl 77.44% <ø> (+0.57%)
src/ticks.jl 92.08% <ø> (+0.71%)
src/mapping.jl 20.31% <ø> (+0.78%)
src/geom/label.jl 99.03% <ø> (+0.92%)
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f315cfe...c6ade7e. Read the comment docs.

@bjarthur
Copy link
Member

i notice a ~15% slow down the first time one draws to a file without src/precompile:

master:

jjulia> @time draw(PDF("/tmp/foo.pdf",4inch,3inch), plot(y=[1,2,3],Geom.point))
 15.890677 seconds (14.52 M allocations: 614.291 MB, 1.47% gc time)

julia> using Gadfly

julia> @time draw(PDF("/tmp/foo.pdf",4inch,3inch), plot(y=[1,2,3],Geom.point))
  0.029691 seconds (21.00 k allocations: 906.945 KB)

julia> @time draw(PDF("/tmp/foo.pdf",4inch,3inch), plot(y=[1,2,3],Geom.point))
  0.028999 seconds (21.00 k allocations: 906.945 KB)

this PR:

julia> @time draw(PDF("/tmp/foo.pdf",4inch,3inch), plot(y=[1,2,3],Geom.point))
 19.093611 seconds (20.67 M allocations: 859.435 MB, 2.48% gc time)

julia> @time draw(PDF("/tmp/foo.pdf",4inch,3inch), plot(y=[1,2,3],Geom.point))
  0.025143 seconds (21.02 k allocations: 907.836 KB)

julia> @time draw(PDF("/tmp/foo.pdf",4inch,3inch), plot(y=[1,2,3],Geom.point))
  0.028570 seconds (21.02 k allocations: 907.836 KB)

as subsequent draws are comparable, i'd say merge it.

should probably reference #921

@tlnagy
Copy link
Member Author

tlnagy commented Feb 22, 2017

Yeah, Gadfly still struggles with the first draw and we've had issues with that as long as I've worked on this project. @aviks' suggestion might be hitting on the problem in #921

@tlnagy
Copy link
Member Author

tlnagy commented Feb 22, 2017

Let's do this

@tlnagy tlnagy merged commit 6019753 into master Feb 22, 2017
@tlnagy tlnagy deleted the tn/strip-old-precompilation branch February 22, 2017 21:15
@tlnagy tlnagy mentioned this pull request Feb 22, 2017
@bjarthur
Copy link
Member

should src/precompile be removed in Compose as well?

tlnagy added a commit to GiovineItalia/Compose.jl that referenced this pull request Feb 22, 2017
@tlnagy
Copy link
Member Author

tlnagy commented Feb 22, 2017

Good point, PR here: GiovineItalia/Compose.jl#236. Increases initial precompilation speed by ~3x.

@timholy
Copy link
Collaborator

timholy commented Mar 20, 2017

In the old days those files dropped the time-to-first-plot by a factor of 2, and so the time spent precompiling seemed well worth it. (You precompile "once," unless you're a developer, but you run many times.) Sounds like the advantage diminished with time?

@tlnagy
Copy link
Member Author

tlnagy commented Mar 20, 2017

Looks like it. Gadfly's time-to-first-plot is still embarrassingly slow (#921) and we still haven't been able to track down the problem. This PR only affects this first plot, the rest are basically instantaneous.

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.

4 participants