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

Rethink @requires dependencies #476

Open
rafaqz opened this issue Jan 20, 2022 · 2 comments · May be fixed by #478
Open

Rethink @requires dependencies #476

rafaqz opened this issue Jan 20, 2022 · 2 comments · May be fixed by #478

Comments

@rafaqz
Copy link

rafaqz commented Jan 20, 2022

The startup time for Blink.jl almost entirely consists of the requires blocks here.

The WebSockets block is unnecessary because its a dependency anyway. Mux.jl is a small package that shares many dependencies. It's much faster to depend on it here than to include it in requires.

Blink.jl already depends on WebIO.jl - the code should just live in Blink.jl where it can be precompiled.

All up this reduces the package load time of Blink.jl (on my laptop) from 5 seconds to 1 second.

See: JuliaGizmos/Blink.jl#288

If people agree I can PR Blink and WebIO with the changes.

@rafaqz rafaqz linked a pull request Jan 20, 2022 that will close this issue
@halleysfifthinc
Copy link
Contributor

I will second rethinking @require dependencies. The current issue which has me looking at WebIO and related packages is dependencies on WebSockets.jl, which is forcing HTTP.jl to be downgraded to <v1.

I might be misreading the code, but afaict, WebSockets isn't actually a hard dependency. So it should be in the [extras] section of the Project file, along with IJulia and Blink.

If removing @requireing dependencies, I would vote for putting glue code into glue packages. Since Blink already depends on WebIO, a glue package wouldn't be needed in that case. A partial solution might be to put the glue code into subpackages that are still @required. This is supposed to be precompilable, which might help package load time, but I haven't tried this and its not currently in use in any registered packages (code search).

@ufechner7
Copy link

What is missing to get this merged?

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.

3 participants