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

Wasm text file preprocessor #1822

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

Wasm text file preprocessor #1822

wants to merge 11 commits into from

Conversation

vouillon
Copy link
Member

@vouillon vouillon commented Jan 30, 2025

For a start, it is only used to manage runtime differences between OCaml versions, and to provide some syntactic sugar for strings. But I plan to use it for a WASI version of the runtime and to implement the use-js-string flag.

| ATOM s ->
parse_list lexbuf toplevel start_loc ({ loc = loc, loc'; desc = Atom s } :: acc)

let parse lexbuf =
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this the second sexp parser in this lib ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, but they are quite differents.

The first one is used internally to read library metadata. This is used for linking, so it's performance critical. And the metadata can be quite large (for core, for instance). Atoms are arbitrary strings, so it is convenient to serialize / deserialize data. I would be happy to use sexplib instead but that would add an external dependency.

This one is use to build a runtime. So the performance is not critical. On the other hand, since it parses user code, it has to deal properly with syntax errors, in a user-friendly way. Since it is used to rewrite Wasm text files, it also keeps a lot of location information that would not be useful for the other use case. Also, atoms are Wasm keywords, not arbitrary strings. I suppose we could implement a function that strips the location information and parses the keywords, but this adds a lot of overhead.

Copy link
Member

Choose a reason for hiding this comment

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

Sexplib / Parsexp are probably fine to add. The major downside is that new releases require ocaml >= 5.1.

The first one is used internally to read library metadata. This is used for linking, so it's performance critical. And the metadata can be quite large (for core, for instance). Atoms are arbitrary strings, so it is convenient to serialize / deserialize data. I would be happy to use sexplib instead but that would add an external dependency.

Couldn't we use marshal for this ?

We can probably keep the 2 sexp implementation for now if it's easier but I'd like to clarify one aspect, the syntax for comments. In addition to line comments, wa_preprocess has block comments but it seems to use a syntax different from parsexp/sexplib. Does the syntax exist elsewhere ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sexplib / Parsexp are probably fine to add. The major downside is that new releases require ocaml >= 5.1.

I think its fine to use an older version for OCaml < 5.1

Couldn't we use marshal for this ?

A text format is much more practical for debugging

We can probably keep the 2 sexp implementation for now if it's easier but I'd like to clarify one aspect, the syntax for comments. In addition to line comments, wa_preprocess has block comments but it seems to use a syntax different from parsexp/sexplib. Does the syntax exist elsewhere ?

This is not standard indeed. It seems to be only used by the Wasm text format.

Copy link
Member

Choose a reason for hiding this comment

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

This is not standard indeed. It seems to be only used by the Wasm text format.

Can you add a reference to the spec somewhere ?

@vouillon vouillon force-pushed the preprocess branch 5 times, most recently from 22aa8ab to 408fc17 Compare January 31, 2025 11:54
@vouillon vouillon marked this pull request as ready for review January 31, 2025 12:57
@hhugo
Copy link
Member

hhugo commented Feb 1, 2025

Have you considered using this to add sugars to help write control flows (e.g having a switch instruction ) ?

@vouillon
Copy link
Member Author

vouillon commented Feb 3, 2025

Have you considered using this to add sugars to help write control flows (e.g having a switch instruction ) ?

That seems a good idea, indeed!

@@ -0,0 +1,17 @@
(executable
(name wasmoo_util)
Copy link
Member

Choose a reason for hiding this comment

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

Have you consider having subcommands inside wasm_of_ocaml instead ?

Copy link
Member Author

Choose a reason for hiding this comment

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

We have the issue that wasmoo_util link is used to build the runtime which is then included in wasm_of_ocaml.
But maybe I can have an internal binary to do that and also expose the same functionality as a wasm_of_ocaml subcommand.

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't the runtime be linked later ? If we want to have it conditional on flags such as use-js-strings ?

Copy link
Member Author

Choose a reason for hiding this comment

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

My current plan is to have a set of precompiled runtimes for the most common options, and to compile runtimes on the fly otherwise.
It's a bit slow to compile and link the runtime. That's especially noticeable when we have a large number of executables, like with the Js_of_ocaml test suite.
But maybe we could share the runtimes produced by wasm_of_ocaml build-runtime between executables, using a similar trick in dune as for ppxs?

@vouillon vouillon force-pushed the preprocess branch 4 times, most recently from 6087ee5 to 7dd3b8c Compare February 5, 2025 15:24
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.

2 participants