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

Improve incrementality of jsoo build #7389

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

hhugo
Copy link
Collaborator

@hhugo hhugo commented Mar 23, 2023

Instead of compiling *.cma files to JavaScript. We can compile individual *.cmos to javascript and link all *.cmo.js together.
This results in a much better incremental build as jsoo no longer recompile compilation units that were not modified.

Linking *.cmo.js into a cma.js should be much faster than any compilation with jsoo.

This can only be done for local libraries as "installed" ones are not required to provide individual cmo. This is fine.

This is following https://discuss.ocaml.org/t/replace-jsoo-link-with-cat-or-esbuild/11600/13?u=hhugo

@rgrinberg
Copy link
Member

Does this affect dead code elimination?

@hhugo
Copy link
Collaborator Author

hhugo commented Mar 27, 2023

It should not affect dead code elimination.

@hhugo
Copy link
Collaborator Author

hhugo commented Mar 27, 2023

This PR requires a new release of jsoo so that js_of_ocaml link can understand the -a flag. We'll need to add some check to know if the flag is accepted.

@rgrinberg
Copy link
Member

I see. I think I discussed doing something like this with @nojb as well to speed up native/bytecode builds and dead code elimination was the sour point as to why this didn't work.

@hhugo
Copy link
Collaborator Author

hhugo commented Mar 27, 2023

I see. I think I discussed doing something like this with @nojb as well to speed up native/bytecode builds and dead code elimination was the sour point as to why this didn't work.

I don't understand.. this PR is about making jsoo compilation similar to ocamlc. Maybe you're thinking about another optimization .. ?

@rgrinberg
Copy link
Member

Oh I see. I didn't read the PR implementation. I thought you meant producing the final .js from the .cmo.js files while completely side-stepping .cma. I see that you're just speeding up the production of your .cma.js here.

@hhugo
Copy link
Collaborator Author

hhugo commented Mar 27, 2023

I see. If you wanted to not produce cma files and pass cmos directly, you could implement the "dead code" logic in dune directly. It's not very difficult (jsoo does it for its link command)

@hhugo
Copy link
Collaborator Author

hhugo commented Mar 27, 2023

I see. If you wanted to not produce cma files and pass cmos directly, you could implement the "dead code" logic in dune directly. It's not very difficult (jsoo does it for its link command)

It's actually more complicated for dune as you need to be able to deal with arbitrary compiler version.

@rgrinberg
Copy link
Member

It's actually more complicated for dune as you need to be able to deal with arbitrary compiler version.

How come? Wouldn't this knowledge come from ocamlep?

@hhugo
Copy link
Collaborator Author

hhugo commented Mar 28, 2023

It's actually more complicated for dune as you need to be able to deal with arbitrary compiler version.

How come? Wouldn't this knowledge come from ocamlep?

I don't think you want to base the logic on ocamldep output. You should let ocamlc decide if a module depend on the implementation of another one. You probably want to just use reloc info inside cmo file format and replicate the small logic for what unit should be linked

@hhugo hhugo force-pushed the jsoo-incr branch 3 times, most recently from dca1805 to 710cad3 Compare August 7, 2023 16:38
~src:(Path.build src)
~obj_dir
~config:None
List.map Jsoo_rules.Config.all ~f:(fun config ->
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@rgrinberg, it'd be better if we could construct rules dynamically based on path (like done for ppxes executables) instead of relying on a restricted list of variants (e.g. Jsoo_rules.Config.all). I think you told me in the past that this feature was planned for later. Was there any progress made on that front ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ping @rgrinberg

Copy link
Member

Choose a reason for hiding this comment

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

The feature still hasn't yet made it in. But you could copy the trick that we use for ppx.

@hhugo
Copy link
Collaborator Author

hhugo commented Nov 7, 2024

@vouillon, I've rebased this PR on top of the wasmoo support but I don't know if this workflow is supported. Can you take look ?

@vouillon
Copy link
Member

vouillon commented Nov 7, 2024

@hhugo Yes, this should work. Could you retry the failing check? I have recreated the wasm_of_ocaml branch used by this workflow.

Signed-off-by: Hugo Heuzard <hugo.heuzard@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants