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.

Sign up for GitHub

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

Jump to bottom

Compiler: refactor sourcemap generation, use Index sourcemap #1715

Merged
merged 1 commit into from
Oct 22, 2024
Merged

Compiler: refactor sourcemap generation, use Index sourcemap #1715

merged 1 commit into from
Oct 22, 2024

Conversation

hhugo
Copy link
Member

@hhugo hhugo commented Oct 17, 2024 •
edited
Loading

This PR leverages the recent support for index sourcemap to make sourcemap generation faster.
We no longer decode or edit mappings.

When compiling cma to js. We emit one sourcemap section per compilation unit. This allows to copy one compilation unit with its mappings without having to decode/change its mapping.

Perf

Compiling js_of_ocaml with itself (dune build compiler/bin-js_of_ocaml/js_of_ocaml.bc.js).
The final link time goes from 660ms on master to 440ms with this PR

Building the toplevel in toplevel/examples/lwt_toplevel/,
The final link time goes from 1350ms (620 ms processing sourcemap) on master to 730ms (90ms processing sourcemap) with this PR

X-link

build on #1714
replace #1617
fix #1446

@hhugo
Copy link
Member Author

hhugo commented Oct 17, 2024

@OlivierNicole, do you want to take a look at this ?

hhugo
hhugo commented Oct 17, 2024
View reviewed changes
compiler/lib/link_js.ml Outdated Show resolved Hide resolved
hhugo
hhugo commented Oct 17, 2024
View reviewed changes
compiler/lib/link_js.ml Outdated Show resolved Hide resolved
@OlivierNicole
Copy link
Contributor

Sure, I’ll take a look.

@hhugo hhugo mentioned this pull request Oct 17, 2024
Closed
@hhugo
Copy link
Member Author

hhugo commented Oct 17, 2024

@rickyvetter, would you be able to test this PR ?

@hhugo hhugo mentioned this pull request Oct 17, 2024
Merged
@OlivierNicole
Copy link
Contributor

What happens to lines that can be added, such as here? Don’t they require editing the mappings?

js_of_ocaml/compiler/lib/link_js.ml

Line 123 in 81aec6d

output_string t.oc (Printf.sprintf "//# %d %S\n" lnum fname);

@OlivierNicole
Copy link
Contributor

Ok I understand, in the end, it’s fine because it never results in adding lines in the middle of a file, but this mechanism (that sometimes add a //# <file> <line> comment when changing files in Line_writer.write) seems brittle and makes the code unclear. Could we do this outside of Line_writer.write?

@hhugo
Copy link
Member Author

hhugo commented Oct 18, 2024

Ok I understand, in the end, it’s fine because it never results in adding lines in the middle of a file, but this mechanism (that sometimes add a //# <file> <line> comment when changing files in Line_writer.write) seems brittle and makes the code unclear. Could we do this outside of Line_writer.write?

I've just simplified the logic a bit.

@rickyvetter
Copy link
Contributor

I'm seeing similar improvements to as #1617. I ran across 6 of our bigger apps and they all averaged 45% reduction in linking time.

OlivierNicole
OlivierNicole approved these changes Oct 19, 2024
View reviewed changes
Copy link
Contributor

@OlivierNicole OlivierNicole left a comment

Choose a reason for hiding this comment

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

This looks good to me, except for the fact that the sourcemap handling in Compile is hard to understand and review, and I think it would really help to add three documentation comments at the right places.

compiler/bin-js_of_ocaml/compile.ml Outdated Show resolved Hide resolved
compiler/bin-js_of_ocaml/compile.ml Outdated Show resolved Hide resolved
compiler/lib/source_map.mli Outdated Show resolved Hide resolved
@hhugo
Copy link
Member Author

hhugo commented Oct 21, 2024

@OlivierNicole, may I request another round of review ?

OlivierNicole
OlivierNicole reviewed Oct 21, 2024
View reviewed changes
Copy link
Contributor

@OlivierNicole OlivierNicole left a comment

Choose a reason for hiding this comment

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

Looks good to me

@hhugo hhugo merged commit 374405c into master Oct 22, 2024
20 checks passed
@hhugo hhugo deleted the smsmsm branch October 22, 2024 05:23
@hhugo hhugo mentioned this pull request Oct 22, 2024
Merged
@hhugo hhugo mentioned this pull request Nov 22, 2024
Merged
hhugo added a commit to hhugo/opam-repository that referenced this pull request Nov 22, 2024
@hhugo
[new release] js_of_ocaml (7 packages) (5.9.0)
b49edaf 
CHANGES:

## Features/Changes
* Misc: update testsuite to OCaml 5.2
* Misc: CI uses opam.2.2 and no longer use sunset repo
* Misc: yojson is no longer optional
* Misc: reduce the diff with the wasm_of_ocaml fork
* Misc: finalize support for OCaml 5.3
* Compiler: speedup global_flow/global_deadcode pass on large bytecode
* Compiler: improved global dead code elimination (ocsigen/js_of_ocaml#2206)
* Compiler: speedup json parsing, relying on Yojson.Raw (ocsigen/js_of_ocaml#1640)
* Compiler: Decode sourcemap mappings only when necessary (ocsigen/js_of_ocaml#1664)
* Compiler: mark [TextEncoder] as reserved
* Compiler: add support for the Wasm backend in parts of the pipeline, in
  prevision for the merge of wasm_of_ocaml
* Compiler: introduce a Targetint module
  that follows the semantic of the backend (js or wasm)
* Compiler: warn on joo_global_object
* Compiler: revisit static env handling (ocsigen/js_of_ocaml#1708)
* Compiler: Emit index source_map to avoid changing mappings (ocsigen/js_of_ocaml#1714, ocsigen/js_of_ocaml#1715)
* Compiler: improved source map generation (ocsigen/js_of_ocaml#1716)
* Runtime: change Sys.os_type on windows (Cygwin -> Win32)
* Runtime: backtraces are really expensive, they need to be explicitly
  requested at compile time (--enable with-js-error) or at startup (OCAMLRUNPARAM=b=1)
* Runtime: allow dynlink of precompiled js with separate compilation (ocsigen/js_of_ocaml#1676)
* Runtime: reimplement the runtime of weak and ephemeron (ocsigen/js_of_ocaml#1707)
* Lib: Modify Typed_array API for compatibility with WebAssembly
* Lib: add details element and toggle event (ocsigen/js_of_ocaml#1728)
* Toplevel: no longer set globals for toplevel initialization
* Runtime: precompute constants used in `caml_lxm_next` (ocsigen/js_of_ocaml#1730)
* Runtime: cleanup runtime

## Bug fixes
* Runtime: fix parsing of unsigned integers (0u2147483648) (ocsigen/js_of_ocaml#1633, ocsigen/js_of_ocaml#1666)
* Runtime: fix incorrect pos_in after unmarshalling
* Runtime: make float_of_string stricter (ocsigen/js_of_ocaml#1609)
* Toplevel: fix missing primitives with separate compilation
* Compiler: fix link of packed modules with separate compilation
* Compiler: Fixed the static evaluation of some equalities (ocsigen/js_of_ocaml#1659)
* Compiler: fix global analysis bug (subsumes ocsigen/js_of_ocaml#1556)
hhugo added a commit to hhugo/opam-repository that referenced this pull request Nov 23, 2024
@hhugo
[new release] js_of_ocaml (7 packages) (5.9.0)
5cd5170 
CHANGES:

## Features/Changes
* Misc: update testsuite to OCaml 5.2
* Misc: CI uses opam.2.2 and no longer use sunset repo
* Misc: yojson is no longer optional
* Misc: reduce the diff with the wasm_of_ocaml fork
* Misc: finalize support for OCaml 5.3
* Compiler: speedup global_flow/global_deadcode pass on large bytecode
* Compiler: improved global dead code elimination (ocsigen/js_of_ocaml#2206)
* Compiler: speedup json parsing, relying on Yojson.Raw (ocsigen/js_of_ocaml#1640)
* Compiler: Decode sourcemap mappings only when necessary (ocsigen/js_of_ocaml#1664)
* Compiler: mark [TextEncoder] as reserved
* Compiler: add support for the Wasm backend in parts of the pipeline, in
  prevision for the merge of wasm_of_ocaml
* Compiler: introduce a Targetint module
  that follows the semantic of the backend (js or wasm)
* Compiler: warn on joo_global_object
* Compiler: revisit static env handling (ocsigen/js_of_ocaml#1708)
* Compiler: Emit index source_map to avoid changing mappings (ocsigen/js_of_ocaml#1714, ocsigen/js_of_ocaml#1715)
* Compiler: improved source map generation (ocsigen/js_of_ocaml#1716)
* Runtime: change Sys.os_type on windows (Cygwin -> Win32)
* Runtime: backtraces are really expensive, they need to be explicitly
  requested at compile time (--enable with-js-error) or at startup (OCAMLRUNPARAM=b=1)
* Runtime: allow dynlink of precompiled js with separate compilation (ocsigen/js_of_ocaml#1676)
* Runtime: reimplement the runtime of weak and ephemeron (ocsigen/js_of_ocaml#1707)
* Lib: Modify Typed_array API for compatibility with WebAssembly
* Lib: add details element and toggle event (ocsigen/js_of_ocaml#1728)
* Toplevel: no longer set globals for toplevel initialization
* Runtime: precompute constants used in `caml_lxm_next` (ocsigen/js_of_ocaml#1730)
* Runtime: cleanup runtime

## Bug fixes
* Runtime: fix parsing of unsigned integers (0u2147483648) (ocsigen/js_of_ocaml#1633, ocsigen/js_of_ocaml#1666)
* Runtime: fix incorrect pos_in after unmarshalling
* Runtime: make float_of_string stricter (ocsigen/js_of_ocaml#1609)
* Toplevel: fix missing primitives with separate compilation
* Compiler: fix link of packed modules with separate compilation
* Compiler: Fixed the static evaluation of some equalities (ocsigen/js_of_ocaml#1659)
* Compiler: fix global analysis bug (subsumes ocsigen/js_of_ocaml#1556)
hhugo added a commit to hhugo/opam-repository that referenced this pull request Nov 26, 2024
@hhugo
[new release] js_of_ocaml (7 packages) (5.9.0)
a7c3c0d 
CHANGES:

## Features/Changes
* Misc: update testsuite to OCaml 5.2
* Misc: CI uses opam.2.2 and no longer use sunset repo
* Misc: yojson is no longer optional
* Misc: reduce the diff with the wasm_of_ocaml fork
* Misc: finalize support for OCaml 5.3
* Compiler: speedup global_flow/global_deadcode pass on large bytecode
* Compiler: improved global dead code elimination (ocsigen/js_of_ocaml#2206)
* Compiler: speedup json parsing, relying on Yojson.Raw (ocsigen/js_of_ocaml#1640)
* Compiler: Decode sourcemap mappings only when necessary (ocsigen/js_of_ocaml#1664)
* Compiler: mark [TextEncoder] as reserved
* Compiler: add support for the Wasm backend in parts of the pipeline, in
  prevision for the merge of wasm_of_ocaml
* Compiler: introduce a Targetint module
  that follows the semantic of the backend (js or wasm)
* Compiler: warn on joo_global_object
* Compiler: revisit static env handling (ocsigen/js_of_ocaml#1708)
* Compiler: Emit index source_map to avoid changing mappings (ocsigen/js_of_ocaml#1714, ocsigen/js_of_ocaml#1715)
* Compiler: improved source map generation (ocsigen/js_of_ocaml#1716)
* Runtime: change Sys.os_type on windows (Cygwin -> Win32)
* Runtime: backtraces are really expensive, they need to be explicitly
  requested at compile time (--enable with-js-error) or at startup (OCAMLRUNPARAM=b=1)
* Runtime: allow dynlink of precompiled js with separate compilation (ocsigen/js_of_ocaml#1676)
* Runtime: reimplement the runtime of weak and ephemeron (ocsigen/js_of_ocaml#1707)
* Lib: Modify Typed_array API for compatibility with WebAssembly
* Lib: add details element and toggle event (ocsigen/js_of_ocaml#1728)
* Toplevel: no longer set globals for toplevel initialization
* Runtime: precompute constants used in `caml_lxm_next` (ocsigen/js_of_ocaml#1730)
* Runtime: cleanup runtime

## Bug fixes
* Runtime: fix parsing of unsigned integers (0u2147483648) (ocsigen/js_of_ocaml#1633, ocsigen/js_of_ocaml#1666)
* Runtime: fix incorrect pos_in after unmarshalling
* Runtime: make float_of_string stricter (ocsigen/js_of_ocaml#1609)
* Toplevel: fix missing primitives with separate compilation
* Compiler: fix link of packed modules with separate compilation
* Compiler: Fixed the static evaluation of some equalities (ocsigen/js_of_ocaml#1659)
* Compiler: fix global analysis bug (subsumes ocsigen/js_of_ocaml#1556)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OlivierNicole OlivierNicole OlivierNicole approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEATURE REQUEST] Use source-map sections in js_of_ocaml link command.
3 participants
@hhugo @OlivierNicole @rickyvetter