Separate compilation: Link only what we need #1378

hhugo · 2023-01-05T13:50:13Z

Fix #943 and #1375

js_of_ocaml link now accept a --linkall flags.
cma files are now compiled one compilation unit / module at a time. We no longer perform cross-module optimization with separate compilation. All compilation units of a cma are still stored in a single file.
We only warn about `Warning: your program contains effect handlers; you should probably run js_of_ocaml with option '--enable=effects' if the offending module end up being linked.
The resulting files should be much reduced (sourcemap is shrinked as well)
In addition, this PR fix sourcemap with separate compilation and fix Compiler: Remove mutable fields from Source_map.t #1316

ISSUE

The dune rules for separate compilation currently completely ignore -linkall information. In practice, with this PR, it means that inline_tests are broken because the library containing the tests is no longer linked.

TODO

check that sourcemap are still correct.

hhugo · 2023-01-05T15:57:22Z

Dune integration discussed in ocaml/dune#6832

hhugo · 2023-01-06T11:58:48Z

@pmwhite @TyOverby, could you check this PR ? You should look at artifact sizes and compilation times with separate compilation. Compilation rules will need to be updated to propagate -linkall to js_of_ocaml link

pmwhite · 2023-01-06T14:41:37Z

This is an exciting patch. I can test this out, hopefully sometime today.

pmwhite · 2023-01-06T15:36:56Z

I'm getting the following build warnings from this patch:

Warning: '--source-map' is enabled but the bytecode program was compiled with no debugging information.
Warning: Consider passing '-g' option to ocamlc.

I'm assuming this patch has changed something to cause those messages to be printed, since we weren't getting these warnings before. Is this new warning expected? If so, what is the right response? One obvious answer is to add -g like the message says, but that has its own set of trade-offs.

pmwhite · 2023-01-06T15:38:38Z

Oh, it's worth saying that I didn't get to the point of adding the linkall flag to the build yet; maybe that error goes away if I add the flag?

hhugo · 2023-01-06T15:42:54Z

I'm getting the following build warnings from this patch:
Warning: '--source-map' is enabled but the bytecode program was compiled with no debugging information.
Warning: Consider passing '-g' option to ocamlc.
I'm assuming this patch has changed something to cause those messages to be printed, since we weren't getting these warnings before. Is this new warning expected? If so, what is the right response? One obvious answer is to add -g like the message says, but that has its own set of trade-offs.

This warning is not new. it used to trigger on empty cma but it was fixed quite sometime ago. I suspect that it now triggers because we compile module from a cma individually and one of the module doesn't have debug info.

pmwhite · 2023-01-06T15:46:49Z

This warning is not new. it used to trigger on empty cma but it was fixed quite sometime ago. I suspect that it now triggers because we compile module from a cma individually and one of the module doesn't have debug info.

I see. If I understand right, this is a bug in this PR, since it is okay for individual modules to not have any debug info.

hhugo · 2023-01-06T15:58:21Z

This warning is not new. it used to trigger on empty cma but it was fixed quite sometime ago. I suspect that it now triggers because we compile module from a cma individually and one of the module doesn't have debug info.

I see. If I understand right, this is a bug in this PR, since it is okay for individual modules to not have any debug info.

It's ok but could result in worse sourcemap. One could argue that the warning is correct. Is there a good reason for omitting the -g for some modules only ?

pmwhite · 2023-01-06T16:34:04Z

Is there a good reason for omitting the -g for some modules only ?

I'm assuming our build system doesn't selectively omit -g (I'll double-check this assumption; EDIT: assumption seems correct), so either

none of the modules have debug-info, which happens when the build is in release mode. EDIT: actually, we always pass -g, even in release mode
all of the modules have debug-info, but some modules are so small that their debug-info is empty. Is that possible?

hhugo · 2023-01-06T16:46:19Z

Is there a good reason for omitting the -g for some modules only ?

I'm assuming our build system doesn't selectively omit -g (I'll double-check this assumption; EDIT: assumption seems correct), so either

none of the modules have debug-info, which happens when the build is in release mode. EDIT: actually, we always pass -g, even in release mode

all of the modules have debug-info, but some modules are so small that their debug-info is empty. Is that possible?

Compiling an empty module doesn't trigger the warning. Could you give details about the command triggering the warning ?

pmwhite · 2023-01-06T16:52:32Z

Inside compiler/tests-jsoo, we run the command

../bin-js_of_ocaml/js_of_ocaml.exe -I . -o ./jsoo_testsuite.cma.js --enable with-js-error --source-map-inline --pretty ./jsoo_testsuite.cma

Which spits out the warning twice. A lot of other commands have the same warning, but this one is representative.

hhugo · 2023-01-06T16:55:01Z

Inside compiler/tests-jsoo, we run the command
../bin-js_of_ocaml/js_of_ocaml.exe -I . -o ./jsoo_testsuite.cma.js --enable with-js-error --source-map-inline --pretty ./jsoo_testsuite.cma
Which spits out the warning twice. A lot of other commands have the same warning, but this one is representative.

Are you still running ocaml 4.12 ?

hhugo · 2023-01-06T16:59:26Z

How is ./jsoo_testsuite.cma compiled ?

hhugo · 2023-01-06T17:16:01Z

@pmwhite, looking at old jenga-rules available on github, I suspect that you still don't pass -g when compiling .ml-gen to cmo.

pmwhite · 2023-01-06T17:17:01Z

Are you still running ocaml 4.12 ?

No, 4.14 now.

How is ./jsoo_testsuite.cma compiled ?

This is the slightly sanitize version of the command that the build system runs.

ocamlc.opt -w @a-4-29-40-41-42-44-45-48-52-56-58-59-66-67-69-70 -strict-sequence -short-paths -strict-formats -g -error-style short -bin-annot -compat-32 jsoo_testsuite__.cmo jsoo_testsuite__Calc_parser.cmo jsoo_testsuite__Calc_lexer.cmo jsoo_testsuite__Parser_1307.cmo jsoo_testsuite__Lexer_1307.cmo jsoo_testsuite__Gh_1307.cmo jsoo_testsuite__Test_bigarray.cmo jsoo_testsuite__Test_channel.cmo jsoo_testsuite__Test_floats.cmo jsoo_testsuite__Test_fma.cmo jsoo_testsuite__Test_ints.cmo jsoo_testsuite__Test_io.cmo jsoo_testsuite__Test_marshal.cmo jsoo_testsuite__Test_obj.cmo jsoo_testsuite__Test_parsing.cmo jsoo_testsuite__Test_rec_mod.cmo jsoo_testsuite.cmo -cclib -ljsoo_testsuite_stubs -a -o jsoo_testsuite.cma

hhugo · 2023-01-06T17:25:52Z

@pmwhite, looking at old jenga-rules available on github, I suspect that you still don't pass -g when compiling .ml-gen to cmo.

Check the command generating jsoo_testsuite__.cmo

pmwhite · 2023-01-06T18:00:56Z

@pmwhite, looking at old jenga-rules available on github, I suspect that you still don't pass -g when compiling .ml-gen to cmo.

Check the command generating jsoo_testsuite__.cmo

Yeah, this is probably it. The command that builds jsoo_testsuite.cmo was missing -g. After adding -g to the .ml-gen rule, the warnings disappeared.

I'm now running into a different set of issues that I'll report if I can't figure them out myself.

hhugo · 2023-01-07T12:15:50Z

@pmwhite, My last commit disable the feature by default so that the PR can be release without silently breaking inline_tests with dune. For testing you can enable it with the --enable auto-link flag to js_of_ocaml link.

pmwhite · 2023-01-09T15:03:40Z

I realized I may not be understanding what this PR actually does. I was under the impression that we already were trimming unnecessary files, and that this moved the granularity down to the module level instead (this is just based on the description - I haven't read the code in the PR). However, now I'm thinking that this actually merely extends the tree-shaking from whole-program compilation into separate compilation as well. Is that the right way to think about it? Are the inclusion semantics for whole and separate compilation now exactly the same?

hhugo · 2023-01-09T15:52:34Z

Are the inclusion semantics for whole and separate compilation now exactly the same?

This is the intent

However, now I'm thinking that this actually merely extends the tree-shaking from whole-program compilation into separate compilation as well. Is that the right way to think about it?

Both whole program compilation and separate compilation perform some deadcode elimination based on what they know. The DCE should not remove any side-effects.

When doing whole program compilation, we rely on the link ouput of ocamlc which does drop unused compilations units present in cma files. This PR implement the same drop logic inside jsoo when doing separate compilation.

pmwhite · 2023-01-20T21:08:50Z

Just as an update on the testing of this PR: I've not yet done all the testing I plan to, since it's a bit of work to get the build system to pass link-all in the right places. I'll update this thread when I eventually get to it.

hhugo · 2023-02-16T07:37:26Z

Just as an update on the testing of this PR: I've not yet done all the testing I plan to, since it's a bit of work to get the build system to pass link-all in the right places. I'll update this thread when I eventually get to it.

Any update?

pmwhite · 2023-02-21T13:55:28Z

@hhugo No, not yet. It's on my stack of things to do, but I haven't gotten around to it yet.

…s_of_ocaml-ppx_deriving_json, js_of_ocaml-ppx, js_of_ocaml-lwt and js_of_ocaml-compiler (5.1.0) CHANGES: ## Features/Changes * Lib: Added support for KeyboardEvent.getModifierState * Misc: bump min ocaml version to 4.08 * Misc: remove some old runtime files to support some external libs * Misc: switch to dune 3.7 * Effects: partial CPS transformation, resulting in much better performances, lower compilation time and smaller generated code * Compiler: separate compilation can now drops unused units when linking (similar to ocamlc). (ocsigen/js_of_ocaml#1378) * Compiler: specialize string to js-string conversion for all valid utf8 strings (previously just ascii) * Compiler: JavaScript files generated by `js_of_ocaml` are now UTF-8 encoded. * Compiler: use identifier for object literals when possible * Compiler: Cache function arity (the length prop of a function is slow with v8) * Compiler: The js lexer is now utf8 aware, recognize and emit utf8 ident * Compiler: Update the js lexer with new number literal syntax * Compiler: update js parser to support most es6 feature (ocsigen/js_of_ocaml#1391) * Compiler: stop parsing the builtin js runtime if not necessary * Compiler: improve js pretty printer (ocsigen/js_of_ocaml#1405) * Compiler: improve debug location and speedup compilation (ocsigen/js_of_ocaml#1407) * Toplevel: Enable separate compilation of toplevels * Runtime: js backtrace recording controled by OCAMLRUNPARAM * Runtime: support for zstd decompression of marshalled data (ocaml.5.1) (ocsigen/js_of_ocaml#12006) * Runtime: stub out custom runtime events symbols for OCaml 5.1 (ocsigen/js_of_ocaml#1414) ## Bug fixes * Effects: fix Js.export and Js.export_all to work with functions (ocsigen/js_of_ocaml#1417,ocsigen/js_of_ocaml#1377) * Sourcemap: fix incorrect sourcemap with separate compilation * Compiler: fix control flow analysis; some annotions were wrong in the runtime * Compiler: js backtrace recording respected in the js runtime and when using effects * Compiler: no longer fail on invalid source file (when the file is a directory) * Runtime: fix the compilation of some mutually recursive functions

hhugo force-pushed the link-what-we-need branch 2 times, most recently from 73978a0 to bfd6578 Compare January 5, 2023 14:42

hhugo mentioned this pull request Jan 5, 2023

Propagate linkall to js_of_ocaml for separate compilation ocaml/dune#6832

Merged

hhugo force-pushed the link-what-we-need branch from bfd6578 to 932fc94 Compare January 6, 2023 11:55

hhugo force-pushed the link-what-we-need branch from 932fc94 to 904cf10 Compare January 6, 2023 14:11

Example: extend separate compilation example

3ce8f04

hhugo force-pushed the link-what-we-need branch from 904cf10 to 4b66609 Compare January 6, 2023 15:36

hhugo force-pushed the link-what-we-need branch from ea612b4 to d901089 Compare January 6, 2023 23:30

hhugo added 5 commits January 7, 2023 00:51

Compiler: new test for sourcemap

514a02d

Compiler: extend jsoo link cmdline interface

e2bc890

Compiler: instrument the linking pass

c67cdd7

Compiler: only link needed units

8bdce5e

Compile: do not rely on file extension for jsoo link

4348943

hhugo added 2 commits January 7, 2023 00:53

Changes

8b5130a

Compiler: remove mutable field in sourcemap

c948306

hhugo force-pushed the link-what-we-need branch from d901089 to c948306 Compare January 6, 2023 23:54

hhugo added 2 commits January 7, 2023 01:27

CI: fix

c5365c7

Compiler: disable auto-link for now

a755f6b

Misc: minor cleanup

b56680d

hhugo merged commit 3b2b311 into master Jan 7, 2023

hhugo deleted the link-what-we-need branch January 7, 2023 12:32

hhugo mentioned this pull request Jan 7, 2023

False positive your program contains effect handlers warning? #1375

Closed

hhugo mentioned this pull request Mar 7, 2023

[new release] js_of_ocaml, js_of_ocaml-tyxml, js_of_ocaml-toplevel, js_of_ocaml-ppx_deriving_json, js_of_ocaml-ppx, js_of_ocaml-lwt and js_of_ocaml-compiler (5.1.0) ocaml/opam-repository#23466

Merged

sim642 mentioned this pull request Jul 24, 2024

[BUG] Runtile failure with -linkall since 5.1.0 #1641

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate compilation: Link only what we need #1378

Separate compilation: Link only what we need #1378

hhugo commented Jan 5, 2023 •

edited

Loading

hhugo commented Jan 5, 2023

hhugo commented Jan 6, 2023

pmwhite commented Jan 6, 2023

pmwhite commented Jan 6, 2023

pmwhite commented Jan 6, 2023

hhugo commented Jan 6, 2023

pmwhite commented Jan 6, 2023

hhugo commented Jan 6, 2023

pmwhite commented Jan 6, 2023 •

edited

Loading

hhugo commented Jan 6, 2023

pmwhite commented Jan 6, 2023

hhugo commented Jan 6, 2023

hhugo commented Jan 6, 2023

hhugo commented Jan 6, 2023

pmwhite commented Jan 6, 2023

hhugo commented Jan 6, 2023

pmwhite commented Jan 6, 2023 •

edited

Loading

hhugo commented Jan 7, 2023

pmwhite commented Jan 9, 2023

hhugo commented Jan 9, 2023 •

edited

Loading

pmwhite commented Jan 20, 2023

hhugo commented Feb 16, 2023

pmwhite commented Feb 21, 2023

Separate compilation: Link only what we need #1378

Separate compilation: Link only what we need #1378

Conversation

hhugo commented Jan 5, 2023 • edited Loading

hhugo commented Jan 5, 2023

hhugo commented Jan 6, 2023

pmwhite commented Jan 6, 2023

pmwhite commented Jan 6, 2023

pmwhite commented Jan 6, 2023

hhugo commented Jan 6, 2023

pmwhite commented Jan 6, 2023

hhugo commented Jan 6, 2023

pmwhite commented Jan 6, 2023 • edited Loading

hhugo commented Jan 6, 2023

pmwhite commented Jan 6, 2023

hhugo commented Jan 6, 2023

hhugo commented Jan 6, 2023

hhugo commented Jan 6, 2023

pmwhite commented Jan 6, 2023

hhugo commented Jan 6, 2023

pmwhite commented Jan 6, 2023 • edited Loading

hhugo commented Jan 7, 2023

pmwhite commented Jan 9, 2023

hhugo commented Jan 9, 2023 • edited Loading

pmwhite commented Jan 20, 2023

hhugo commented Feb 16, 2023

pmwhite commented Feb 21, 2023

hhugo commented Jan 5, 2023 •

edited

Loading

pmwhite commented Jan 6, 2023 •

edited

Loading

pmwhite commented Jan 6, 2023 •

edited

Loading

hhugo commented Jan 9, 2023 •

edited

Loading