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

Simplify/automate the compilation and use of a system image #12

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

ffevotte
Copy link
Collaborator

The following PR introduces features helping compile and use custom system images for LanguageServer and SymbolServer. The expected benefits are reduced start-up times for the language server. On my (old-ish) machine, start-up times are reduced from 20 seconds to less than 2. On a newer machine where I also tested this, times went from 15 seconds down to less than 1.

The idea is the following:

  • eglot-jl.jl looks at the EGLOT_JL_TEST environment variable; when it is set, the server is configured to read from an IOBuffer that sends it an exit instruction as soon as it is able to process requests. This allows eglot-jl.jl to be used as a precompile_execution_file that exercises the (language) server before gracefully exiting.
  • a compile.jl script automates the generation of a system image, named after the julia version (and having the relevant extension depending on the OS).

This is what the first commit in this PR introduces, alongside with instructions in README.org explaining how to compile the system image, and set eglot-jl-julia-args to use it.

The second commit goes a step further: it automates the search for a suitable system image at server startup, in order to use it if one is found.

  • a first call to julia allows getting the version number. This involves something like 0.1s additional latency before the server is run, which I think is acceptable but is nevertheless quite a large amount of time for such an insignificant-looking task
  • a suitable system image is looked for, based on the version number; if one is found, the corresponding --sysimage command-line switch is generated. If not, nothing happens; no system image is used, so users who have not generated any system image will not be impacted by this.
  • the whole process handling system images can be deactivated using a new eglot-jl-enable-sysimage customizable option. This allows users to opt out of the system, even if they have previously generated a system image (for example in case it would become stale for some reason). It also completely removes the julia version overhead.

Additionally, a new autoloaded command allows running the system image generation script from within emacs. The same command could be used to re-generate a system image if the previous one caused problems for any reason.

This is documented in README.org. I've tested this with Julia 1.3 and 1.4, on Windows and Linux, without encountering any problem (I don't have access to a Mac).

I know you were reluctant to handle system images in eglot-jl, and were (rightfully) especially concerned about possible ways that a system image would become obsolete. I tried making this implementation as robust as possible, but I might very well have overlooked something. So please do not hesitate to tell me if something bothers you with this proposal.

@non-Jedi
Copy link
Owner

Could you rebase on master, and I'll try to review in the next few days?

@ffevotte
Copy link
Collaborator Author

OK, the PR is now rebased on the current master, and everything seems to work on my system.

There were several conflicts though; I'll try and test more in the coming days, and report back if anything goes wrong.

Copy link
Owner

@non-Jedi non-Jedi left a comment

Choose a reason for hiding this comment

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

First commit I'm good with. Still need to review second one.

eglot-jl.jl Show resolved Hide resolved
Copy link
Owner

@non-Jedi non-Jedi left a comment

Choose a reason for hiding this comment

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

I still need to spend a bit more time looking through this commit, but I realized that I would never get to this if I waited until I had time to review the whole thing, so consider this a partial review of the second commit.

eglot-jl.el Outdated Show resolved Hide resolved
eglot-jl.el Outdated Show resolved Hide resolved
eglot-jl.el Outdated Show resolved Hide resolved
eglot-jl.el Outdated Show resolved Hide resolved
@ffevotte
Copy link
Collaborator Author

As always, thanks for your insightful comments, that are thought-provoking.

To sum everything up, my current position would be along the lines of:

  • computing a system image name that incorporates both the Julia version and a SHA1 hash of the Manifest
  • implementing this only in Julia, and re-using the implementation by calling Julia from elisp (but I'm open to having implementations both in elisp and in julia to reduce the overhead of spawning a julia process if you feel that this is desirable)
  • switching to an opt-in mechanism solely based on the eglot-jl-enable-sysimage cusomization variable, that is set to nil by default so that users who don't opt-in would have absolutely no overhead. And users who opt in wouldn't have to manually call eglot-jl-compile-sysimage: if no suitable sysimage is found when eglot starts, they would be prompted to launch the compilation of one in the background.

What would you think about that?

@non-Jedi
Copy link
Owner

non-Jedi commented Jul 27, 2020

That sounds like a good path forward to me. :) I will note that if using the custom sysimage requires spawning multiple julia processes, it should definitely be opt-in rather than opt-out, but that's the plan anyway.

As an alternative, maybe eglot-jl-enable-sysimage could cause the sysimage to be compiled on eglot-jl-init so that compiling the sysimage doesn't run afoul of eglot-connect-timeout? Then invoking eglot either errors or shows a warning if the sysimage isn't available? We would want to make sure this is happening asynchronously of course.

eglot-jl.el Outdated Show resolved Hide resolved
eglot-jl.el Show resolved Hide resolved
@non-Jedi
Copy link
Owner

non-Jedi commented Aug 8, 2020

Related: JuliaLang/julia#35794

@ffevotte
Copy link
Collaborator Author

ffevotte commented Aug 28, 2020

I'v finally gotten around to working on this again. The latest commit should implement the strategy discussed above:

  • Opt-in mechanism:
    • eglot-jl-enable-sysimage is nil by default. No additional latency is introduced unless this setting is manually changed.
    • Setting eglot-jl-enable-sysimage automatically triggers sysimage compilation on eglot-jl-init when no suitable sysimage is found.
    • If eglot-jl-enable-sysimage is set and a suitable system image is found, it is automatically used.

  • System image naming convention:
    • new naming convention: ${DIR}/eglot-jl_${VERSION}_${SLUG}.{EXT}
      • ${DIR}: path to the active project (should be the one defined by eglot-jl-language-server-project). Defaults to eglot-jl-base for Julia versions <1.4 where Pkg.project() is not defined (more on that hereafter).
      • ${VERSION}: Julia version string. This should allow avoiding problems with the sysimage becoming incorrect when Julia gets updated.
      • ${SLUG}: sha1 checksum of the contents of ${DIR}/Manifest.toml. This should allow avoiding problems with the sysimage becoming out-of-date when eglot-jl and its Julia dependencies get updated.
      • ${EXT}: suitable extension depending on the OS.
    • names following this convention are computed by a small julia function defined in sysimage-path.jl. When needed in Emacs, a Julia process is spawned for this (never happens unless eglot-jl-enable-sysimage is set).

@non-Jedi When you have some time to look at this (no rush!), I'd love to hear what you think.

@ffevotte
Copy link
Collaborator Author

ffevotte commented Aug 28, 2020

There is a small gotcha regarding the use of system images in conjunction with customized eglot-jl-language-server-project: although the Emacs side of the process knows which LanguageServer project should be activated, we currently don't forward that explicitly to Julia. And introspection capabilities have only been recently introduced in Pkg, so that for older Julia versions, there is no way (that I know of) of retrieving the currently active project in order to correctly encode the SHA1 of its Manifest in the sysimage name.

There are several ways of dealing with this that I can think of:

  • leave things like this, and document the fact that eglot-jl-enable-sysimage and eglot-jl-language-server-project only work well together for Julia 1.4 and above
  • use an environment variable to forward eglot-jl-language-server-project to the Julia process; this would probably be the easiest solution to implement, requiring minimal changes to the current code. But "hiding" this in an environment variable does not look too clean to me.
  • pass eglot-jl-language-server-project as an additional (optional) command-line argument to eglot-jl.jl. But I fear this will make calling eglot-jl.jl more complex, for maybe not too much of a benefit. EDIT: come to think of it, eglot-jl.jl does not have to know anything about the system image name; only compile.jl. So I definitely prefer this option!

I think my preference goes to this last option, but I'd love to hear your thoughts about it if you have some...

@ffevotte
Copy link
Collaborator Author

I went ahead and fixed the issues with eglot-jl-language-server-project and eglot-jl-enable-sysimage being used together in the latest commit. I'm happy with this solution, since it does not affect eglot-jl.jl in any way for users not opting in the sysimage feature.

@non-Jedi
Copy link
Owner

non-Jedi commented Feb 12, 2021

I haven't had an opportunity to use Julia for a while to test-drive this, but at this point if you've been using this without issue, I'm fine with you merging it. Sorry for the extremely long review delay.

The compilation process is also streamlined, allowing it to be run directly from
Emacs with `M-x eglot-jl-compile-sysimage`.
* Opt-in mechanism:
  - `eglot-jl-enable-sysimage` is nil by default. No additional latency is
    introduced unless this setting is manually changed.
  - Setting `eglot-jl-enable-sysimage` automatically triggers sysimage
    compilation on `eglot-jl-init` when no suitable sysimage is found.
  - If `eglot-jl-enable-sysimage` is set and a suitable system image is found,
    it is automatically used.

* System image naming convention:
  - new naming convention: ${DIR}/eglot-jl_${VERSION}_${SLUG}.{EXT}
    + ${DIR}: path to the active project (should be the one defined by
        `eglot-jl-language-server-project`). Defaults to `eglot-jl-base` for
        Julia versions <1.4 where Pkg.project() is not defined.
    + ${VERSION}: Julia version string. This should allow avoiding problems with
        the sysimage becoming incorrect when Julia gets updated.
    + ${SLUG}: sha1 checksum of the contents of ${DIR}/Manifest.toml. This
        should allow avoiding problems with the sysimage becoming out-of-date
        when eglot-jl and its Julia dependencies get updated.
    + ${EXT}: suitable extension depending on the OS.
  - names following this convention are computed by a small julia function
    defined in `sysimage-path.jl`. When needed in Emacs, a Julia process is
    spawned for this (never happens unless `eglot-jl-enable-sysimage` is set).
Make `eglot-jl-enable-sysimage` and `eglot-jl-language-server-project`
compatible with each other in all cases, for all Julia versions.

This works by propagating the value of `eglot-jl-language-server-project` from
the Emacs side to the Julia side via command-line arguments when necessary:
- in order to determine the correct system image name to look for, and
- in order to compile the system image itself.
@ffevotte
Copy link
Collaborator Author

This new version is rebased on the current master. It also incorporates (minor) modifications related to the need to resolve environments when switching Julia version.

I'm planning to test this in daily life for a few days in order to check that nothing went wrong with those last changes.

@chriselrod
Copy link
Contributor

chriselrod commented Jan 17, 2023

This would still be nice to have IMO, even with the improvements in 1.9.

@linwaytin
Copy link

Same here. The startup time is still quite long for me. Hopefully this can be 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 this pull request may close these issues.

4 participants