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

Run all tests with double translation too #1821

Merged
merged 15 commits into from
Feb 3, 2025
Merged
8 changes: 8 additions & 0 deletions .github/workflows/build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,11 @@ jobs:
with:
ocaml-compiler: ${{ matrix.ocaml-compiler }}

# Pin Dune to a version which supports the new --effects flag
# (https://github.com/ocaml/dune/pull/11222).
- name: Pin Dune
run: opam pin add --no-action dune https://github.com/OlivierNicole/dune.git#jsoo-effects

# Work-around a race between reinstalling mingw-w64-shims
# (because of conf-pkg-config optional dep) and installing other
# packages that implicitly depend on mingw-w64-shims.
Expand Down Expand Up @@ -151,6 +156,9 @@ jobs:
- run: opam exec -- dune build @all @runtest @runtest-js --profile with-effects
if: ${{ !matrix.skip-effects }}

- run: opam exec -- dune build @all @runtest @runtest-js --profile with-effects-double-translation
if: ${{ !matrix.skip-effects }}

- run: opam exec -- git diff --exit-code
if: ${{ !matrix.skip-test }}

Expand Down
15 changes: 0 additions & 15 deletions compiler/tests-dynlink-js/dune
Original file line number Diff line number Diff line change
Expand Up @@ -4,27 +4,12 @@
(libraries js_of_ocaml)
(link_flags
(:standard -linkall))
(js_of_ocaml
(flags
(:standard)
(:include effects_flags.sexp))
(build_runtime_flags
(:standard)
(:include effects_flags.sexp))
(link_flags (:standard)))
(modes js byte))

(executable
(name effects_flags)
(modules effects_flags))

(rule
(target effects_flags.sexp)
(action
(with-stdout-to
%{target}
(run ./effects_flags.exe sexp %{profile}))))

(rule
(target effects_flags.txt)
(action
Expand Down
19 changes: 8 additions & 11 deletions compiler/tests-dynlink-js/effects_flags.ml
Original file line number Diff line number Diff line change
@@ -1,19 +1,16 @@
let enable b n =
let f = if b then "--enable" else "--disable" in
[ f; n ]

let () =
let major = String.split_on_char '.' Sys.ocaml_version |> List.hd |> int_of_string in
let has_effect l =
match l with
| [ "with-effects" ] -> major >= 5
| _ -> false
let effects_flags l =
match l, major >= 5 with
| [ "with-effects-double-translation" ], true -> [ "--effects"; "double-translation" ]
| [ "with-effects" ], true -> [ "--enable"; "effects" ]
| _, true -> [ "--disable"; "effects" ]
| _, false -> [ "--disable"; "effects" ]
in
let aux l = enable (has_effect l) "effects" in
match Sys.argv |> Array.to_list |> List.tl with
| "txt" :: rest -> List.iter print_endline (aux rest)
| "txt" :: rest -> List.iter print_endline (effects_flags rest)
| "sexp" :: rest ->
print_endline "(";
List.iter print_endline (aux rest);
List.iter print_endline (effects_flags rest);
print_endline ")"
| _ -> assert false
63 changes: 0 additions & 63 deletions compiler/tests-jsoo/lib-effects-2/dune

This file was deleted.

2 changes: 2 additions & 0 deletions compiler/tests-jsoo/lib-effects/dune
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
(env
(with-effects-double-translation)
(with-effects)
(_
(js_of_ocaml
(flags
Expand Down
6 changes: 3 additions & 3 deletions compiler/tests-ocaml/effect-syntax/dune
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
(env
(with-effects-double-translation)
(with-effects)
(_
(js_of_ocaml
(flags
(:standard --enable=effects))
(build_runtime_flags
(:standard --enable=effects)))))
(:standard --enable effects)))))

(tests
(names
Expand Down
66 changes: 0 additions & 66 deletions compiler/tests-ocaml/effects-2/dune

This file was deleted.

6 changes: 3 additions & 3 deletions compiler/tests-ocaml/effects/dune
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
(env
(with-effects-double-translation)
(with-effects)
(_
(js_of_ocaml
(flags
(:standard --enable=effects))
(build_runtime_flags
(:standard --enable=effects)))))
(:standard --enable effects)))))

(tests
(build_if
Expand Down
17 changes: 13 additions & 4 deletions dune
Original file line number Diff line number Diff line change
Expand Up @@ -9,18 +9,27 @@
(js_of_ocaml
(compilation_mode separate)
(flags
(:standard --enable effects))
(build_runtime_flags
(:standard --enable effects)))
(wasm_of_ocaml
(compilation_mode separate)
(flags
(:standard --enable effects))
(build_runtime_flags
(:standard --enable effects)))
(binaries
(tools/node_wrapper.exe as node)
(tools/node_wrapper.exe as node.exe)))
(with-effects-double-translation
(js_of_ocaml
(compilation_mode separate)
(flags
(:standard --effects double-translation))
(build_runtime_flags
(:standard --effects double-translation)))
(wasm_of_ocaml
;; Double transaction is not supported in wasm
(enabled_if false))
(binaries
(tools/node_wrapper.exe as node)
(tools/node_wrapper.exe as node.exe)))
(bench_no_debug
(flags
(:standard \ -g))
Expand Down
2 changes: 1 addition & 1 deletion lib/tests/test_fun_call.ml
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ let s x =
if(x === undefined)
return "undefined"
if(typeof x === "function")
return "function#" + x.length + "#" + x.l
return "function#" + (x.l > 0 ? x.l : x.length) + "#" + x.l
if(x.toString() == "[object Arguments]")
return "(Arguments: " + Array.prototype.slice.call(x).toString() + ")";
return x.toString()
Expand Down
32 changes: 11 additions & 21 deletions manual/effects.wiki
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,13 @@ The analysis is especially effective on monomorphic code. It is not so effective
We hope to improve on this by trying alternative compilation
strategies.

An alternative CPS transform is provided under the {{--effects=double-translation}} option. It keeps a direct-style version of the transformed functions in addition to the CPS version. The choice of running the CPS version is delayed to run time. Since CPS code is usually slower, this can avoid degradations. In addition, one can ensure that some code is run in direct style by using {{Jsoo_runtime.Effect.assume_no_perform}}. A caveat is that Dune does not know about {{--effects=double-translation}} yet and may try to link together files built with {{--enable=double-translation}} and files built with only {{--enable=effects}}, which gives an error. A work-around is to disable separate compilation by using the option {{(js_of_ocaml (compilation_mode whole_program))}}.
An alternative CPS transform is provided under the {{{--effects=double-translation}}} option. It keeps a direct-style version of the transformed functions in addition to the CPS version. The choice of running the CPS version is delayed to run time. Since CPS code is usually slower, this can avoid degradations. In addition, one can ensure that some code is run in direct style by using {{{Jsoo_runtime.Effect.assume_no_perform}}}.

=== Dune integration ===

We're still working on dune support for compiling js_of_ocaml programs
with effect handlers enabled.
Dune is aware of the {{{--enable effects}}} option. So, you can just add it to the Js_of_ocaml flags {{{(js_of_ocaml (flags ...))}}} wherever you want to use it.

For now, here are two possible setups.
We're still working on dune support for the {{{--effects}}} option. For now, here are two possible setups.

=== Whole dune workspace setup ===

Expand All @@ -27,8 +26,8 @@ Put the following in a {{{dune}}} (or {{{dune-workspace}}}) file at the root of
(env
(_
(js_of_ocaml
(flags (:standard --enable effects))
(build_runtime_flags (:standard --enable effects)))))
(flags (:standard --effects=double-translation))
(build_runtime_flags (:standard --effects=double-translation)))))
}}}

With this setup, one can use both separate and whole program compilation.
Expand All @@ -37,28 +36,19 @@ With this setup, one can use both separate and whole program compilation.
=== Sub directory setup ===

If you want to enable effect handlers for some binaries only, you'll
have to give-up separate compilation for now using the following in
your {{{dune}}} file.

{{{
(env
(_
(js_of_ocaml
(compilation_mode whole_program))))
}}}

Then pass the rights {{{js_of_ocaml}}} flags to the executable stanza

have to give-up separate compilation for now using:
{{{
(executable
(name main)
(js_of_ocaml (flags (:standard --enable effects)))
(js_of_ocaml
(compilation_mode whole_program)
(flags (:standard --effects=double-translation)))
)
}}}

Trying to use separate compilation would result in a error while attempting to link the final js file.
{{{
js_of_ocaml: Error: Incompatible build info detected while linking.
- test6.bc.runtime.js: effects=false
- .cmphash.eobjs/byte/dune__exe.cmo.js: effects=true
- test6.bc.runtime.js: effects=disabled
- .cmphash.eobjs/byte/dune__exe.cmo.js: effects=double-translation
}}}
3 changes: 2 additions & 1 deletion tools/ci_setup.ml
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,8 @@ let node_wrapper =
, {|(executable
(public_name node)
(name node_wrapper)
(libraries unix))|} )
(libraries unix))|} )
; "node_wrapper/node_wrapper_per_profile.ml", {|let args = []|}
; "node_wrapper/dune-project", "(lang dune 3.17)"
; "node_wrapper/node_wrapper.opam", ""
]
Expand Down
13 changes: 12 additions & 1 deletion tools/dune
Original file line number Diff line number Diff line change
@@ -1,8 +1,19 @@
(executable
(name node_wrapper)
(modules node_wrapper)
(modules node_wrapper node_wrapper_per_profile)
(libraries unix))

(executable
(name gen_node_wrapper_per_profile)
(modules gen_node_wrapper_per_profile))

(rule
(target node_wrapper_per_profile.ml)
(action
(with-stdout-to
%{target}
(run ./gen_node_wrapper_per_profile.exe %{profile}))))

(executable
(name ci_setup)
(modules ci_setup)
Expand Down
12 changes: 12 additions & 0 deletions tools/gen_node_wrapper_per_profile.ml
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
let profile = Sys.argv.(1)

let () =
match profile with
| "with-effects-double-translation" ->
(* We somehow need a larger stack with --effects=double-translation. *)
print_endline {|
let args = [ "--stack-size=3000" ]
|}
| _ -> print_endline {|
let args = [ ]
|}
Loading
Loading