superbol-studio-oss icon indicating copy to clipboard operation
superbol-studio-oss copied to clipboard

Included a js_of_ocaml version of superbol in the .vsix

Open bclement-ocp opened this issue 2 years ago • 8 comments

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.

bclement-ocp avatar Oct 27 '23 12:10 bclement-ocp

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 :(

bclement-ocp avatar Oct 27 '23 14:10 bclement-ocp

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?

nberth avatar Oct 27 '23 14:10 nberth

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 )

bclement-ocp avatar Oct 27 '23 15:10 bclement-ocp

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?

nberth avatar Oct 27 '23 15:10 nberth

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 avatar Oct 27 '23 15:10 bclement-ocp

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.

bclement-ocp avatar Oct 27 '23 15:10 bclement-ocp

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?

GitMensch avatar Oct 29 '23 08:10 GitMensch

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 avatar Mar 05 '24 13:03 nberth

De-historizing after more investigations

nberth avatar Dec 20 '24 14:12 nberth