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

Open
wants to merge 27 commits into
base: master
Choose a base branch
from
Open

Make resuming a continuation more efficient #1765

wants to merge 27 commits into from

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.

@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.

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