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.

Sign up for GitHub

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

Jump to bottom

Upgrade to 5.1.1: use compiler-libs' compression functions #1714

Merged
merged 5 commits into from
Dec 1, 2023
Merged

Upgrade to 5.1.1: use compiler-libs' compression functions #1714

merged 5 commits into from
Dec 1, 2023

Conversation

voodoos
Copy link
Collaborator

@voodoos voodoos commented Nov 27, 2023

This should fix #1713

@trefis I decided to rely on the new Compression module from compiler-libs (in 2230f56). This greatly simplify the update. Does that feels right to you ? Not doing that would mean also vendoring the c-stubs for zstd and check how the compiler was configured...

@Octachron
Copy link
Member

Note that for 5.2, the Compression module should be available as a separate library distributed. So hopefully, using the module right now should offer a mostly painless upgrade path beyond 5.1.1.

@kit-ty-kate kit-ty-kate mentioned this pull request Nov 27, 2023
Merged
@voodoos
Copy link
Collaborator Author

voodoos commented Nov 28, 2023

So this is not ideal: linking compiler-libs lead to almost doubling Merlin's binary's size (10M -> 17M).

@kit-ty-kate
Copy link
Member

So this is not ideal: linking compiler-libs lead to almost doubling Merlin's binary's size (10M -> 17M).

is that an issue in terms of performance?

kit-ty-kate
kit-ty-kate reviewed Nov 29, 2023
View reviewed changes
src/ocaml/typing/cmi_format.ml
@@ -35,7 +35,7 @@ type cmi_infos = {
}

let input_cmi ic =
let (name, sign) = (input_value ic : header) in
let (name, sign) = (Ocaml_compression.input_value ic : header) in
Copy link
Member

Choose a reason for hiding this comment

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

Does that still work when compression is disabled?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's how it's done in the compiler: https://github.com/ocaml/ocaml/blob/5.1/file_formats/cmt_format.ml#L108

It's actually a simple call to Stdlib.input_value that looks like it will be able to input both compressed (if supported) and uncompressed data.

@kit-ty-kate kit-ty-kate mentioned this pull request Nov 29, 2023
Merged
@voodoos
Copy link
Collaborator Author

voodoos commented Nov 29, 2023 •
edited
Loading

is that an issue in terms of performance?

I don't think it should have a noticeable impact: the actual binary that need to start quickly is the one written in C. ocamlmerlin-server, the one that grows a lot here, is only started once per session.

We tried other approaches but could not get something reasonable so we are going to keep the solution in that PR and switch to an external library when it will be available.

I will cut a release tomorrow.

@voodoos voodoos merged commit 3dd2198 into ocaml:501 Dec 1, 2023
0 of 2 checks passed
voodoos added a commit to voodoos/opam-repository that referenced this pull request Dec 1, 2023
@voodoos
[new release] merlin (3 packages) (4.13.1-501)
908be25 
CHANGES:

Fri Dec  1 15:00:42 CET 2023

  + merlin binary
    - Fix a follow-up issue to the preference of non-ghost nodes introduced in ocaml/merlin#1660 (ocaml/merlin#1690, fixes ocaml/merlin#1689)
    - Add `-cache-lifespan` flag, that sets cache invalidation period. (ocaml/merlin#1698,
      ocaml/merlin#1705)
    - Ignore the new 5.1 `cmi-file` flag instead of rejecting it (ocaml/merlin#1710, fixes
      ocaml/merlin#1703)
    - Fix Merlin locate not fallbacking on the correct file in case of ambiguity
      (@goldfirere, ocaml/merlin#1699)
    - Fix Merlin reporting errors provoked by the recovery itself (ocaml/merlin#1709, fixes
      ocaml/merlin#1704)
    - Add support for OCaml 5.1.1 (ocaml/merlin#1714)
  + editor modes
    - vim: load merlin when Vim is compiled with +python3/dyn (e.g. MacVim)
    - emacs: highlight only first error line by default (ocaml/merlin#1693, fixes ocaml/merlin#1663)
voodoos added a commit to voodoos/opam-repository that referenced this pull request Dec 1, 2023
@voodoos
[new release] merlin-lib and merlin (4.13.1-501)
692e0d8 
CHANGES:

Fri Dec  1 15:00:42 CET 2023

  + merlin binary
    - Fix a follow-up issue to the preference of non-ghost nodes introduced in ocaml/merlin#1660 (ocaml/merlin#1690, fixes ocaml/merlin#1689)
    - Add `-cache-lifespan` flag, that sets cache invalidation period. (ocaml/merlin#1698,
      ocaml/merlin#1705)
    - Ignore the new 5.1 `cmi-file` flag instead of rejecting it (ocaml/merlin#1710, fixes
      ocaml/merlin#1703)
    - Fix Merlin locate not fallbacking on the correct file in case of ambiguity
      (@goldfirere, ocaml/merlin#1699)
    - Fix Merlin reporting errors provoked by the recovery itself (ocaml/merlin#1709, fixes
      ocaml/merlin#1704)
    - Add support for OCaml 5.1.1 (ocaml/merlin#1714)
  + editor modes
    - vim: load merlin when Vim is compiled with +python3/dyn (e.g. MacVim)
    - emacs: highlight only first error line by default (ocaml/merlin#1693, fixes ocaml/merlin#1663)

[new release] merlin-lib and merlin (4.13-501)

CHANGES:

Fri Dec  1 15:00:42 CET 2023

  + merlin binary
    - Fix a follow-up issue to the preference of non-ghost nodes introduced in ocaml/merlin#1660 (ocaml/merlin#1690, fixes ocaml/merlin#1689)
    - Add `-cache-lifespan` flag, that sets cache invalidation period. (ocaml/merlin#1698,
      ocaml/merlin#1705)
    - Ignore the new 5.1 `cmi-file` flag instead of rejecting it (ocaml/merlin#1710, fixes
      ocaml/merlin#1703)
    - Fix Merlin locate not fallbacking on the correct file in case of ambiguity
      (@goldfirere, ocaml/merlin#1699)
    - Fix Merlin reporting errors provoked by the recovery itself (ocaml/merlin#1709, fixes
      ocaml/merlin#1704)
  + editor modes
    - vim: load merlin when Vim is compiled with +python3/dyn (e.g. MacVim)
    - emacs: highlight only first error line by default (ocaml/merlin#1693, fixes ocaml/merlin#1663)

[new release] merlin-lib and merlin (4.13-414)

CHANGES:

Fri Dec  1 15:00:42 CET 2023

  + merlin binary
    - Fix a follow-up issue to the preference of non-ghost nodes introduced in ocaml/merlin#1660 (ocaml/merlin#1690, fixes ocaml/merlin#1689)
    - Add `-cache-lifespan` flag, that sets cache invalidation period. (ocaml/merlin#1698,
      ocaml/merlin#1705)
    - Fix Merlin locate not fallbacking on the correct file in case of ambiguity
      (@goldfirere, ocaml/merlin#1699)
    - Fix Merlin reporting errors provoked by the recovery itself (ocaml/merlin#1709, fixes
      ocaml/merlin#1704)
  + editor modes
    - vim: load merlin when Vim is compiled with +python3/dyn (e.g. MacVim)
    - emacs: highlight only first error line by default (ocaml/merlin#1693, fixes ocaml/merlin#1663)
@voodoos voodoos mentioned this pull request Dec 1, 2023
Merged
nberth pushed a commit to nberth/opam-repository that referenced this pull request Jun 18, 2024
@voodoos @nberth
[new release] merlin-lib and merlin (4.13.1-501)
adad0b5 
CHANGES:

Fri Dec  1 15:00:42 CET 2023

  + merlin binary
    - Fix a follow-up issue to the preference of non-ghost nodes introduced in ocaml/merlin#1660 (ocaml/merlin#1690, fixes ocaml/merlin#1689)
    - Add `-cache-lifespan` flag, that sets cache invalidation period. (ocaml/merlin#1698,
      ocaml/merlin#1705)
    - Ignore the new 5.1 `cmi-file` flag instead of rejecting it (ocaml/merlin#1710, fixes
      ocaml/merlin#1703)
    - Fix Merlin locate not fallbacking on the correct file in case of ambiguity
      (@goldfirere, ocaml/merlin#1699)
    - Fix Merlin reporting errors provoked by the recovery itself (ocaml/merlin#1709, fixes
      ocaml/merlin#1704)
    - Add support for OCaml 5.1.1 (ocaml/merlin#1714)
  + editor modes
    - vim: load merlin when Vim is compiled with +python3/dyn (e.g. MacVim)
    - emacs: highlight only first error line by default (ocaml/merlin#1693, fixes ocaml/merlin#1663)

[new release] merlin-lib and merlin (4.13-501)

CHANGES:

Fri Dec  1 15:00:42 CET 2023

  + merlin binary
    - Fix a follow-up issue to the preference of non-ghost nodes introduced in ocaml/merlin#1660 (ocaml/merlin#1690, fixes ocaml/merlin#1689)
    - Add `-cache-lifespan` flag, that sets cache invalidation period. (ocaml/merlin#1698,
      ocaml/merlin#1705)
    - Ignore the new 5.1 `cmi-file` flag instead of rejecting it (ocaml/merlin#1710, fixes
      ocaml/merlin#1703)
    - Fix Merlin locate not fallbacking on the correct file in case of ambiguity
      (@goldfirere, ocaml/merlin#1699)
    - Fix Merlin reporting errors provoked by the recovery itself (ocaml/merlin#1709, fixes
      ocaml/merlin#1704)
  + editor modes
    - vim: load merlin when Vim is compiled with +python3/dyn (e.g. MacVim)
    - emacs: highlight only first error line by default (ocaml/merlin#1693, fixes ocaml/merlin#1663)

[new release] merlin-lib and merlin (4.13-414)

CHANGES:

Fri Dec  1 15:00:42 CET 2023

  + merlin binary
    - Fix a follow-up issue to the preference of non-ghost nodes introduced in ocaml/merlin#1660 (ocaml/merlin#1690, fixes ocaml/merlin#1689)
    - Add `-cache-lifespan` flag, that sets cache invalidation period. (ocaml/merlin#1698,
      ocaml/merlin#1705)
    - Fix Merlin locate not fallbacking on the correct file in case of ambiguity
      (@goldfirere, ocaml/merlin#1699)
    - Fix Merlin reporting errors provoked by the recovery itself (ocaml/merlin#1709, fixes
      ocaml/merlin#1704)
  + editor modes
    - vim: load merlin when Vim is compiled with +python3/dyn (e.g. MacVim)
    - emacs: highlight only first error line by default (ocaml/merlin#1693, fixes ocaml/merlin#1663)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kit-ty-kate kit-ty-kate kit-ty-kate left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@voodoos @Octachron @kit-ty-kate