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

Replace polysemy by effectful #2663

Merged
merged 33 commits into from Mar 21, 2024
Merged

Replace polysemy by effectful #2663

merged 33 commits into from Mar 21, 2024

Conversation

janmasrovira
Copy link
Collaborator

@janmasrovira janmasrovira commented Feb 20, 2024

The following benchmark compares juvix 0.6.0 with polysemy and a new version (implemented in this pr) which replaces polysemy by effectful.

Typecheck standard library without caching

hyperfine --warmup 2 --prepare 'juvix-polysemy clean' 'juvix-polysemy typecheck Stdlib/Prelude.juvix' 'juvix-effectful typecheck Stdlib/Prelude.juvix'
Benchmark 1: juvix-polysemy typecheck Stdlib/Prelude.juvix
  Time (mean ± σ):      3.924 s ±  0.143 s    [User: 3.787 s, System: 0.084 s]
  Range (min … max):    3.649 s …  4.142 s    10 runs

Benchmark 2: juvix-effectful typecheck Stdlib/Prelude.juvix
  Time (mean ± σ):      2.558 s ±  0.074 s    [User: 2.430 s, System: 0.084 s]
  Range (min … max):    2.403 s …  2.646 s    10 runs

Summary
  juvix-effectful typecheck Stdlib/Prelude.juvix ran
    1.53 ± 0.07 times faster than juvix-polysemy typecheck Stdlib/Prelude.juvix

Typecheck standard library with caching

hyperfine --warmup 1 'juvix-effectful typecheck Stdlib/Prelude.juvix' 'juvix-polysemy typecheck Stdlib/Prelude.juvix' --min-runs 20
Benchmark 1: juvix-effectful typecheck Stdlib/Prelude.juvix
  Time (mean ± σ):      1.194 s ±  0.068 s    [User: 0.979 s, System: 0.211 s]
  Range (min … max):    1.113 s …  1.307 s    20 runs

Benchmark 2: juvix-polysemy typecheck Stdlib/Prelude.juvix
  Time (mean ± σ):      1.237 s ±  0.083 s    [User: 0.997 s, System: 0.231 s]
  Range (min … max):    1.061 s …  1.476 s    20 runs

Summary
  juvix-effectful typecheck Stdlib/Prelude.juvix ran
    1.04 ± 0.09 times faster than juvix-polysemy typecheck Stdlib/Prelude.juvix

@janmasrovira janmasrovira self-assigned this Feb 20, 2024
@janmasrovira janmasrovira force-pushed the replace-polysemy branch 4 times, most recently from d261ed8 to 433ccd5 Compare February 27, 2024 15:12
@janmasrovira janmasrovira force-pushed the replace-polysemy branch 2 times, most recently from 8544f91 to ac4e7bf Compare March 1, 2024 10:24
@arybczak
Copy link

arybczak commented Mar 2, 2024

I was curious about this PR because this is a nice example of effectful vs polysemy, but I'm very surprised at the second benchmark results.

I suspect something's not right with the translation and I was curious enough that I tried to reproduce this locally and I can, but when I tried to build the project with profiling to see what's going on, GHC fails to compile the repline/haskeline module because you do some weird things there and apparently GHC gets confused 😕

@janmasrovira
Copy link
Collaborator Author

janmasrovira commented Mar 5, 2024

I was curious about this PR because this is a nice example of effectful vs polysemy, but I'm very surprised at the second benchmark results.

I suspect something's not right with the translation and I was curious enough that I tried to reproduce this locally and I can, but when I tried to build the project with profiling to see what's going on, GHC fails to compile the repline/haskeline module because you do some weird things there and apparently GHC gets confused 😕

We were also surprised with that benchmark and we are investigating the cause. I'll share the findings as soon as we have them.
Ufortunately, I can't help you with the profiling problem. We are able to compile with profiling flags in linux but not in mac.

@janmasrovira janmasrovira force-pushed the replace-polysemy branch 6 times, most recently from 7590a93 to 42b0bad Compare March 20, 2024 11:38
@janmasrovira janmasrovira changed the base branch from main to rename-dynamic March 20, 2024 11:51
Base automatically changed from rename-dynamic to main March 20, 2024 17:47
@janmasrovira janmasrovira marked this pull request as ready for review March 20, 2024 17:48
@janmasrovira
Copy link
Collaborator Author

janmasrovira commented Mar 20, 2024

@arybczak it seems like b311abd solved the problem. I've updated the results to reflect that. We believe the gains in the test with caching are close to null because 90+ percent of the execution time is spent doing IO.

@paulcadman paulcadman merged commit 3a4cbc7 into main Mar 21, 2024
4 of 8 checks passed
@paulcadman paulcadman deleted the replace-polysemy branch March 21, 2024 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants