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

Provide a "universal" VSIX, with the LSP server compiled in JavaScript #93

Merged
merged 9 commits into from
Feb 3, 2025

Conversation

bclement-ocp
Copy link
Contributor

This is a first step towards fixing #76 (but does not entirely fixes it). It avoids the need to install an external superbol executable, and should work on all platforms.

The bundled superbol-free.bc.js is used if the option superbol.path is set to null, which is the new default. Some performance investigation is needed -- if we find out that the js_of_ocaml version is too slow for practical use, we can make this not be the default.

Copy link
Collaborator

@nberth nberth left a comment

Choose a reason for hiding this comment

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

This seems quite promising. We'll just have to check the impacts on performance (we can see to edit some of the NIST85 sources to check that 😉 — to be configured with dialect = "cobol85").

We'll refrain from merging right now, though: I'd rather let drom continue to automatically manage/generate some of the files edited here.

src/lsp/superbol-free/dune Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
Makefile.header Outdated Show resolved Hide resolved
@bclement-ocp
Copy link
Contributor Author

FWIW, I have checked on a bunch of files from NIST85 and the performance difference, if any, is not noticeable on my machine — except that with the js_of_ocaml version I get stack overflows when computing semantic tokens on large files, which is an issue.

This is probably due to tail recursion not being handled properly by js_of_ocaml outside of mutually recursive nests, and I guess we have tail recursion that doesn't fall in this category (some CPS stuff maybe?). I have tried the --enable=effects flag from js_of_ocaml but there the performance difference is sadly noticeable :(

@nberth
Copy link
Collaborator

nberth commented Oct 27, 2023

FWIW, I have checked on a bunch of files from NIST85 and the performance difference, if any, is not noticeable on my machine — except that with the js_of_ocaml version I get stack overflows when computing semantic tokens on large files, which is an issue.

Yes I just made the same observations… do you think adding some tailcall annotations may help identify where the issues could be?

@bclement-ocp
Copy link
Contributor Author

FWIW, I have checked on a bunch of files from NIST85 and the performance difference, if any, is not noticeable on my machine — except that with the js_of_ocaml version I get stack overflows when computing semantic tokens on large files, which is an issue.

Yes I just made the same observations… do you think adding some tailcall annotations may help identify where the issues could be?

As far as I know the @tailcall annotation requires the compiler to fail if the the call can't be optimized, it won't help here — js_of_ocaml doesn't perform tail call elimination (except in very specific cases, cf https://ocsigen.org/js_of_ocaml/latest/manual/tailcall )

@nberth
Copy link
Collaborator

nberth commented Oct 27, 2023

As far as I know the @tailcall annotation requires the compiler to fail if the the call can't be optimized, it won't help here — js_of_ocaml doesn't perform tail call elimination (except in very specific cases, cf https://ocsigen.org/js_of_ocaml/latest/manual/tailcall )

Ah ok. That was in case that annotation could also be checked by js_of_ocaml itself. It seams only some LSP requests appear to suffer from the issue… is it possible to add --enable=effects to only some libraries? or is it a flag that only applies globally?

@bclement-ocp
Copy link
Contributor Author

js_of_ocaml takes bytecode as input — I don't think that the annotation is preserved until there in the compilation chain. --enable=effects applies a CPS transformation and I believe already tries to keep code in direct style when possible, so I think we are kind of stuck with the #76 suggestion of using platform-dependent extensions for now. Will discuss with @ddeclerck what is the best way to build those.

@bclement-ocp bclement-ocp marked this pull request as draft October 27, 2023 15:24
@bclement-ocp
Copy link
Contributor Author

Marking as draft because it seems I made a mistake somewhere — I thought this was using the bundled node from vscode but it actually is not, so I will have to investigate more, if we do want to do this.

@GitMensch
Copy link
Contributor

Several changes done here (at least the parts that change hard-wired entries to variables) seem reasonable in any case. Can those get it with a direct commit/separate PR so that the actual "javascript server" PR will include less changes?

@nberth nberth added history PR or issue that is out-dated and will probably never be merged and removed ok to review As its name says labels Mar 5, 2024
@nberth
Copy link
Collaborator

nberth commented Mar 5, 2024

Labeling this as history. Performance issues with the Javascript+effects version seem to be too hard to deal with at the moment. Another way towards a "pure" web extension could be to compile the LSP server into WASM using wasm_of_ocaml (or another such compiler).

@nberth
Copy link
Collaborator

nberth commented Dec 20, 2024

De-historizing after more investigations

@nberth nberth removed the history PR or issue that is out-dated and will probably never be merged label Dec 20, 2024
.gitattributes Outdated Show resolved Hide resolved
Makefile.drom-tpl Outdated Show resolved Hide resolved
Makefile.drom-tpl Outdated Show resolved Hide resolved
Makefile.header Outdated Show resolved Hide resolved
opam/cobol_data.opam Show resolved Hide resolved
bclement-ocp and others added 4 commits January 27, 2025 10:45
This is a first step towards fixing OCamlPro#76 (but does not entirely fixes
it).  It avoids the need to install an external superbol executable, and
should work on all platforms.

The bundled superbol-free.bc.js is used if the option `superbol.path` is
set to `null`, which is the new default. Some performance investigation
is needed -- if we find out that the js_of_ocaml version is too slow for
practical use, we can make this not be the default.
@nberth nberth changed the title Included a js_of_ocaml version of superbol in the .vsix Include a js_of_ocaml version of superbol in the .vsix Jan 28, 2025
@nberth nberth changed the title Include a js_of_ocaml version of superbol in the .vsix Provide a "universal" VSIX, with the LSP server compiled in JavaScript Jan 28, 2025
@nberth nberth force-pushed the mr-superbol.js branch 3 times, most recently from 48e6c77 to f54ffbe Compare January 31, 2025 12:31
@nberth nberth marked this pull request as ready for review February 3, 2025 13:15
@nberth nberth merged commit e52e732 into OCamlPro:master Feb 3, 2025
4 checks passed
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.

5 participants