Included a js_of_ocaml version of superbol in the .vsix #93

bclement-ocp · 2023-10-27T12:01:12Z

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.

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

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.

nberth · 2023-10-27T12:40:23Z

src/lsp/superbol-free/dune

Please investigate whether that can be achieved via fields of the corresponding package.toml. Same for superbol_free_lib/dune.

I have changed the drom files. Running drom build generates a bunch of main.ml files everywhere tho, not sure what's up about that. Not included in this commit, in spite of drom "helpfully" (not at all) deciding to add its changes to the index.

Might depend on drom's version. I typically use opam exec -- drom project to generated the files instead.

I, too, hate the way drom interacts with git. (Fabrice mentioned he welcomed PRs on drom, notably to disable auto-indexing).

Makefile

nberth · 2023-10-27T12:41:52Z

package.json

+            "null"
+          ],
+          "default": null,
+          "description": "Path to the external `superbol` command. If `null`, use the bundled `superbol-free`."


Note this conflicts with pending edits in #72 .

nberth · 2023-10-27T14:15:21Z

src/vscode/superbol-vscode-platform/dune

I think in this case there is a dune.drom-tpl to edit instead.

I don't think this is a change I made — drom modified this file.

I saw something related to js_of_ocaml so I though it was edited manually. We should probably check those lines were not introduced on purpose.

We should ask @ddeclerck they were introduced in a commit "fix cross-compilation" (09b9ce7)

This dune.drom-tpl is based on https://github.com/OCamlPro/drom-share/blob/master/packages/js_program/dune_. I had to add it so I could add a missing !(package-dune-stanzas) (which comes from the package.toml file and disables compiling the VSCode extension when cross-compiling : js_of_ocaml is neither available nor needed in cross-compilation environments). Obviously, this is a temporary solution ; the template dune file should be modified in the aforementioned drom-share repository. If someone feels like contributing to drom.

nberth · 2023-10-27T14:27:25Z

Makefile.header

-	opam exec -- dune build $(SRCDIR)/$(PROJECT).bc.js --profile=release
-	mkdir -p _out
-	$(CP) _build/default/$(SRCDIR)/$(PROJECT).bc.js _out/
+build-release: _out/$(CLIENT_PROJECT).bc.js


Why not _out/$(SERVER_PROJECT).bc.js as well?

Good catch, I simply forgot :)

bclement-ocp · 2023-10-27T14:47:16Z

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 · 2023-10-27T14:56:55Z

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 · 2023-10-27T15:00:33Z

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 · 2023-10-27T15:06:13Z

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 · 2023-10-27T15:11:44Z

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 · 2023-10-27T15:24:53Z

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 · 2023-10-29T08:29:26Z

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 · 2024-03-05T13:47:10Z

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).

bclement-ocp added the ok to review As its name says label Oct 27, 2023

bclement-ocp requested a review from nberth October 27, 2023 12:01

bclement-ocp mentioned this pull request Oct 27, 2023

Support change of reference format while indenting #94

Merged

nberth requested changes Oct 27, 2023

View reviewed changes

Give drom its blood tribute

4f4a4a1

nberth reviewed Oct 27, 2023

View reviewed changes

bclement-ocp marked this pull request as draft October 27, 2023 15:24

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 mentioned this pull request Mar 5, 2024

Provide a web-only version of the extension by compiling the server into WASM #188

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Included a js_of_ocaml version of superbol in the .vsix #93

Included a js_of_ocaml version of superbol in the .vsix #93

bclement-ocp commented Oct 27, 2023

nberth left a comment

nberth Oct 27, 2023

bclement-ocp Oct 27, 2023

nberth Oct 27, 2023

nberth Oct 27, 2023

nberth Oct 27, 2023

nberth Oct 27, 2023

bclement-ocp Oct 27, 2023

nberth Oct 27, 2023

bclement-ocp Oct 27, 2023

ddeclerck Oct 27, 2023

nberth Oct 27, 2023

bclement-ocp Oct 27, 2023

bclement-ocp commented Oct 27, 2023

nberth commented Oct 27, 2023

bclement-ocp commented Oct 27, 2023

nberth commented Oct 27, 2023

bclement-ocp commented Oct 27, 2023

bclement-ocp commented Oct 27, 2023

GitMensch commented Oct 29, 2023

nberth commented Mar 5, 2024

Included a js_of_ocaml version of superbol in the .vsix #93

Are you sure you want to change the base?

Included a js_of_ocaml version of superbol in the .vsix #93

Conversation

bclement-ocp commented Oct 27, 2023

nberth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bclement-ocp commented Oct 27, 2023

nberth commented Oct 27, 2023

bclement-ocp commented Oct 27, 2023

nberth commented Oct 27, 2023

bclement-ocp commented Oct 27, 2023

bclement-ocp commented Oct 27, 2023

GitMensch commented Oct 29, 2023

nberth commented Mar 5, 2024