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

stop running functions in init #607

Merged
merged 1 commit into from
Apr 24, 2020
Merged

Conversation

KristofferC
Copy link
Contributor

Zygote currently does the same thing as CSV used to do (JuliaData/CSV.jl#324) which is to run some representative functions in __init__ to make the first call look faster. In reality, this just shifts the latency in the first call to package load time. The problem is that Zygote can be loaded in a Julia session without necessarily getting called. In those scenarios, users have to pay the compilation cost anyway which makes it more costly to have Zygote as a dependency.

@DhairyaLGandhi
Copy link
Member

If we are removing the code to be called, then does precompile.jl need to be removed as well?

@IanButterworth
Copy link
Contributor

This makes sense, and would address the issue that motivated me to look into #606
Also, on reflection, the contents of precompile.jl is probably what the SnoopCompile bot should be running in #606 rather than the unit tests

@IanButterworth
Copy link
Contributor

As a case study, loading is currently quite painful on a modern aarch64 system

julia> @time using Zygote
[ Info: Precompiling Zygote [e88e6eb3-aa80-5325-afca-941959d7151f]
155.025100 seconds (76.22 M allocations: 3.948 GiB, 2.12% gc time)

julia> exit()
ian@nx1:~/Documents/julia-1.4.1/bin$ ./julia
               _
   _       _ _(_)_     |  Documentation: https://docs.julialang.org
  (_)     | (_) (_)    |
   _ _   _| |_  __ _   |  Type "?" for help, "]?" for Pkg help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 1.4.1 (2020-04-14)
 _/ |\__'_|_|_|\__'_|  |  Official https://julialang.org/ release
|__/                   |

julia> @time using Zygote
 90.218599 seconds (56.06 M allocations: 2.980 GiB, 2.39% gc time)

This PR improves things a lot (but aarch64 does seem disproportionately slow at loading for some reason..)

julia> @time using Zygote
[ Info: Precompiling Zygote [e88e6eb3-aa80-5325-afca-941959d7151f]
 58.374538 seconds (23.72 M allocations: 1.380 GiB, 0.99% gc time)

julia> exit()
ian@nx1:~/Documents/julia-1.4.1/bin$ ./julia
               _
   _       _ _(_)_     |  Documentation: https://docs.julialang.org
  (_)     | (_) (_)    |
   _ _   _| |_  __ _   |  Type "?" for help, "]?" for Pkg help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 1.4.1 (2020-04-14)
 _/ |\__'_|_|_|\__'_|  |  Official https://julialang.org/ release
|__/                   |

julia> @time using Zygote
 19.383780 seconds (22.75 M allocations: 1.336 GiB, 2.19% gc time)

And adding the SnoopCompile bot to run the precompile.jl script and provide baked in precompile statements should restore the benefit while keeping load time lower. New PR incoming, @aminya is working in upcoming changes to the SnoopCompile bot api

@CarloLucibello
Copy link
Member

so this definitely seems worth doing, and we can keep precompile.jl for SnoopCompile

bors r+

@bors
Copy link
Contributor

bors bot commented Apr 24, 2020

Build succeeded:

@bors bors bot merged commit 764a416 into FluxML:master Apr 24, 2020
@aminya aminya mentioned this pull request Apr 24, 2020
@MikeInnes
Copy link
Member

Yes, we may as well remove precompile.jl here too.

@aminya
Copy link

aminya commented May 5, 2020

@MikeInnes this is done in #615

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.

6 participants