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

Make resuming a continuation more efficient #1765

Merged
merged 7 commits into from
Dec 30, 2024
Merged

Make resuming a continuation more efficient #1765

merged 7 commits into from
Dec 30, 2024

Conversation

hhugo
Copy link
Member

@hhugo hhugo commented Dec 10, 2024

Fix #1658
See corresponding OCaml PR ocaml/ocaml#12735

  • some comments needs to be updated.
  • The wasm part has not been optimized

@hhugo hhugo marked this pull request as draft December 10, 2024 13:35
@hhugo
Copy link
Member Author

hhugo commented Dec 10, 2024

@vouillon, I looked at updating the runtime but I'm not familiar enough with this part.

dune Outdated Show resolved Hide resolved
@hhugo hhugo force-pushed the opt-fiber branch 4 times, most recently from cf30bbf to 93c0a04 Compare December 12, 2024 12:51
@hhugo hhugo marked this pull request as ready for review December 12, 2024 12:52
@hhugo hhugo requested a review from vouillon December 12, 2024 14:04
@hhugo hhugo changed the title Compiler: prepare compiler for 1658 Make resuming a continuation more efficient Dec 16, 2024
@hhugo
Copy link
Member Author

hhugo commented Dec 16, 2024

I've finally managed to find my way out int the runtime.
I've tried to change the current design as little as possible but It might not be the best approach.

runtime/js/effect.js Outdated Show resolved Hide resolved
@hhugo hhugo force-pushed the opt-fiber branch 2 times, most recently from ab3e6a5 to 80dcdff Compare December 17, 2024 20:54
@hhugo
Copy link
Member Author

hhugo commented Dec 19, 2024

@vouillon, I've reworked the implementation of effects in the js runtime so that it looks more like what's done upstream in the c runtime. Can you take a look ?

@hhugo
Copy link
Member Author

hhugo commented Dec 19, 2024

The deep_state benchmark (included in this PR in compiler/tests-jsoo/lib-effects/deep_state.ml) runs significantly faster with this PR

$ hyperfine -L v before,after --warmup 3 'node {v}.js 64 500_000'
Benchmark 1: node before.js 64 500_000
  Time (mean ± σ):      2.745 s ±  0.046 s    [User: 2.779 s, System: 0.036 s]
  Range (min … max):    2.671 s …  2.822 s    10 runs
 
Benchmark 2: node after.js 64 500_000
  Time (mean ± σ):      1.724 s ±  0.044 s    [User: 1.761 s, System: 0.033 s]
  Range (min … max):    1.639 s …  1.771 s    10 runs
 
Summary
  node after.js 64 500_000 ran
    1.59 ± 0.05 times faster than node before.js 64 500_000
$ hyperfine -L v before,after --warmup 3 'node {v}.js 128 500_000'
Benchmark 1: node before.js 128 500_000
  Time (mean ± σ):      5.348 s ±  0.096 s    [User: 5.386 s, System: 0.041 s]
  Range (min … max):    5.225 s …  5.473 s    10 runs
 
Benchmark 2: node after.js 128 500_000
  Time (mean ± σ):      3.636 s ±  0.247 s    [User: 3.691 s, System: 0.042 s]
  Range (min … max):    2.986 s …  3.845 s    10 runs
 
Summary
  node after.js 128 500_000 ran
    1.47 ± 0.10 times faster than node before.js 128 500_000

@hhugo
Copy link
Member Author

hhugo commented Dec 19, 2024

The latest commit (that implement perform and reperform separately) is even faster.

$ hyperfine -L v before,next --warmup 3 'node {v}.js 128 500_000'
Benchmark 1: node before.js 128 500_000
  Time (mean ± σ):      5.249 s ±  0.128 s    [User: 5.290 s, System: 0.045 s]
  Range (min … max):    5.117 s …  5.468 s    10 runs
 
Benchmark 2: node next.js 128 500_000
  Time (mean ± σ):      2.383 s ±  0.043 s    [User: 2.448 s, System: 0.040 s]
  Range (min … max):    2.286 s …  2.437 s    10 runs
 
Summary
  node next.js 128 500_000 ran
    2.20 ± 0.07 times faster than node before.js 128 500_000

@hhugo hhugo added this to the 6.0 milestone Dec 19, 2024
runtime/js/effect.js Outdated Show resolved Hide resolved
runtime/js/effect.js Outdated Show resolved Hide resolved
runtime/js/effect.js Outdated Show resolved Hide resolved
runtime/js/effect.js Outdated Show resolved Hide resolved
runtime/js/effect.js Outdated Show resolved Hide resolved
runtime/js/jslib.js Outdated Show resolved Hide resolved
@hhugo
Copy link
Member Author

hhugo commented Dec 20, 2024

@vouillon, thanks for the review. I've applied you suggestion and removed the caml_fiber_stack to only keep caml_current_stack.

@hhugo
Copy link
Member Author

hhugo commented Dec 20, 2024

@vouillon, should we merge in the current state or should we wait for the wasm runtime?

@vouillon
Copy link
Member

I would really prefer that #1461 get merged first, rather than asking @OlivierNicole to rebase it once more.
But I don't think we need to wait for the wasm runtime.

@smorimoto smorimoto requested a review from Copilot December 29, 2024 20:07

Choose a reason for hiding this comment

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

Copilot reviewed 4 out of 13 changed files in this pull request and generated no comments.

Files not reviewed (9)
  • compiler/lib/effects.ml: Language not supported
  • compiler/lib/parse_bytecode.ml: Language not supported
  • compiler/tests-check-prim/main.4.14.output: Language not supported
  • compiler/tests-check-prim/unix-Unix.4.14.output: Language not supported
  • compiler/tests-check-prim/unix-Win32.4.14.output: Language not supported
  • compiler/tests-jsoo/lib-effects/deep_state.ml: Language not supported
  • compiler/tests-jsoo/lib-effects/dune: Language not supported
  • runtime/wasm/effect.wat: Language not supported
  • compiler/tests-full/stdlib.cma.expected.js: Evaluated as low risk
Comments suppressed due to low confidence (4)

runtime/js/effect.js:87

  • The condition 'if (last === 0)' should be checked to ensure that 'last' is correctly initialized.
if (last === 0) {

runtime/js/effect.js:139

  • [nitpick] The variable name 'last_fiber' is ambiguous. It should be renamed to something more descriptive, such as 'current_fiber'.
var last_fiber = caml_current_stack;

runtime/js/effect.js:132

  • The new behavior introduced in the 'caml_perform_effect' function should be covered by tests.
function caml_perform_effect(eff, k0) {

runtime/js/effect.js:156

  • The new behavior introduced in the 'caml_reperform_effect' function should be covered by tests.
function caml_reperform_effect(eff, cont, last, k0) {
@hhugo hhugo merged commit 64f4619 into master Dec 30, 2024
28 checks passed
@hhugo hhugo deleted the opt-fiber branch December 30, 2024 22:05
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.

[FEATURE REQUEST] Make resuming a continuation more efficient
2 participants