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

Lazy-loading DataFrames.jl reduces the package loading time #154

Merged
merged 3 commits into from
Aug 17, 2020

Conversation

mirkobunse
Copy link
Contributor

This PR uses Requires.jl to load DataFrames.jl only when actually needed. It reduces the output of @time using PGFPlots from 19s to 16s, on my machine. The script which makes the following time measurements is given in issue #130.

Trial 0/3: 28.656163 seconds (30.13 M allocations: 1.544 GiB, 1.53% gc time)
Trial 1/3: 15.671989 seconds (27.89 M allocations: 1.435 GiB, 2.67% gc time)
Trial 2/3: 15.979555 seconds (27.89 M allocations: 1.435 GiB, 2.62% gc time)
Trial 3/3: 16.170125 seconds (27.89 M allocations: 1.435 GiB, 2.62% gc time)
Mean @time (ignoring the first output due to possibly required pre-compilation):
 15.940556333333333 seconds (27.89 M allocations: 1.4349999999999998 GiB, 2.6366666666666667% gc time)

Lazy-loading DataFrames.jl also makes perfect sense from a design perspective because the dependency is only needed to implement wrappers around the actual functions. Users only need these wrappers if they have loaded DataFrames, already.

Since the package is only needed to implement wrappers around the actual functions, there's no immediate drawback of loading it dynamically; any user will only need the wrappers after loading DataFrames, too. Perfect use case for Requires.jl.
@mykelk
Copy link
Member

mykelk commented Aug 16, 2020

It is strange that it is failing on ERROR: LoadError: LoadError: UndefVarError: hasproperty not defined in Julia 1.0 on Travis, but it seems to have passed in @mossr 's most recent commit. I'm probably missing something?

@mossr
Copy link
Member

mossr commented Aug 17, 2020

Odd...hasproperty came from Compat when using NBInclude but something I'm also missing is going on. Regardless, I removed the dependency on hasproperty in master.

@mykelk
Copy link
Member

mykelk commented Aug 17, 2020

Thanks @mossr . @mirkobunse do you think you can pull in his most recent change? Then we can get this merged. Thank you for your contribution!

@mirkobunse
Copy link
Contributor Author

It worked. Thanks, @mossr !

@mossr
Copy link
Member

mossr commented Aug 17, 2020

@mirkobunse You're very welcome! Thanks for the contribution! I'll merge this now.

@mossr mossr merged commit c0ab7df into JuliaTeX:master Aug 17, 2020
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.

3 participants