Use Eio for parallelizing some analysis commands #7840

zth · 2025-09-04T10:27:49Z

This introduces OCaml multicore (Eio) to the analysis bin, and leverages it for 2 of the editor tooling commands:

rename
Find all references

Benching this on my local machine, in a repo of ~350k lines of ReScript and ~1800 files, both commands end up about 2.5x faster.

nojaf

Exciting stuff! Do you have any good resources on Eio?
I looked at it a while ago and didn't really find any beginner friendly stuff on it. The main repo was an overwhelming read for me.

I hope we can also integrate eio-trace and opentelemetry at some point.

nojaf · 2025-09-04T11:37:05Z

analysis/src/Commands.ml

-  print_endline
-    (if allLocs = [] then Protocol.null
-     else "[\n" ^ (allLocs |> String.concat ",\n") ^ "\n]")
+  Eio_main.run (fun env ->


Would a future goal be to have this Eio_main at the main entry point level of analysis tool?

Yup! I moved it there now as well, but just for the relevant sub commands.

nojaf · 2025-09-04T11:38:27Z

analysis/src/References.ml


-let forLocalStamp ~full:{file; extra; package} stamp (tip : Tip.t) =
+(* Single helper for parallel work distribution over a list. *)
+let parallel_map ~domain_mgr ~items ~f =


A little wild that this isn't somewhere part of the Eio library.

Maybe it is. This one sprung from trying to tune the exact use case in this PR a bit. If something better exists we can switch to that as we uncover more types of "loads" as we expand using this.

pkg-pr-new · 2025-09-05T08:42:36Z

Open in StackBlitz

rescript

npm i https://pkg.pr.new/rescript-lang/rescript@7840

@rescript/darwin-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-arm64@7840

@rescript/darwin-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-x64@7840

@rescript/linux-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-arm64@7840

@rescript/linux-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-x64@7840

@rescript/runtime

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/runtime@7840

@rescript/win32-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/win32-x64@7840

commit: 4d74fcc

zth · 2025-09-05T08:51:33Z

.github/workflows/ci.yml

-          # Verify that the compiler still builds with the oldest OCaml version we support.
-          - os: ubuntu-24.04
-            ocaml_compiler: ocaml-variants.4.14.2+options,ocaml-option-static
-            node-target: linux-x64
-            rust-target: x86_64-unknown-linux-musl
-


This was discussed in Discord that it's OK for us to move to 5.3.0+.

analysis.opam

cknitt · 2025-09-05T09:00:11Z

dune-project

+ (synopsis "ReScript compiler")
+ (depends
+  (ocaml
+   (>= 5.3))))


Why do we need that here now?

Turns out we don't. Removed.

scripts/copyExes.js

cknitt · 2025-09-05T09:03:19Z

.github/workflows/ci.yml

-        run: opam exec -- dune build --display quiet --profile static
+        run: |
+          arch=$(dpkg-architecture -qDEB_HOST_MULTIARCH)
+          C_INCLUDE_PATH="/usr/include:/usr/include/$arch" CPATH="/usr/include:/usr/include/$arch" opam exec -- dune build --display quiet --profile static


Rather than duplicating these env var definitions, maybe we could have a single step that sets them in GITHUB_ENV?

…ler itself

Copilot

Pull Request Overview

This PR introduces OCaml multicore (Eio) parallelization to the ReScript analysis bin, targeting the rename and find all references commands for approximately 2.5x performance improvements on large codebases.

Key changes:

Updates OCaml version requirement from 4.14 to 5.3 across all packages
Adds Eio dependencies for multicore support
Implements parallel processing for reference finding operations
Adds thread-safe access to shared state using mutexes

Reviewed Changes

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

Show a summary per file

File	Description
dune-project	Updates OCaml version requirement and adds Eio dependencies
*.opam files	Generated package files with updated OCaml version and Eio deps
analysis/src/dune	Adds Eio library dependency
analysis/src/SharedTypes.ml	Adds StateSync module with mutex for thread-safe access
analysis/src/References.ml	Implements parallel_map function and updates reference finding to use multicore
analysis/src/ProcessCmt.ml	Adds thread-safe caching with double-checked locking
analysis/src/Packages.ml	Updates package retrieval functions to use thread-safe access
analysis/src/Commands.ml	Updates command functions to accept Eio environment and use parallel operations
.github/workflows/ci.yml	Updates CI to use OCaml 5.3 and removes 4.14 compatibility check
CHANGELOG.md	Documents the new multicore feature

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

analysis/src/References.ml

Copilot · 2025-09-05T10:37:21Z

analysis/src/References.ml

+    if doms <= 1 || len < small_threshold then f items
+    else
+      let chunk_count = min doms len in
+      let chunk_size = max 1 ((len + chunk_count - 1) / chunk_count) in


The magic number 10 for small_threshold should be extracted to a named constant or configuration parameter to improve maintainability and make the threshold adjustable.

analysis/src/ProcessCmt.ml

Copilot · 2025-09-05T10:37:21Z

.github/workflows/ci.yml

        if: runner.os == 'Linux'
        uses: awalsh128/cache-apt-pkgs-action@v1.4.3
        with:
          # See https://github.com/ocaml/setup-ocaml/blob/b2105f9/packages/setup-ocaml/src/unix.ts#L9


The addition of linux-libc-dev dpkg-dev packages should include a comment explaining why these specific packages are needed for the OCaml 5.3/Eio build requirements.

Can you fix this Copilot?

nojaf

Looks good to me, I don't know enough about eio, so I will let the others do the approval here.

nojaf · 2025-09-05T11:12:32Z

analysis/bin/main.ml

  | [_; "format"; path] ->
    Printf.printf "\"%s\"" (Json.escape (Commands.format ~path))
-  | [_; "test"; path] -> Commands.test ~path
+  | [_; "test"; path] -> Eio_main.run (fun env -> Commands.test ~env ~path)


This is used because reference tests require the env now right?
The tests themselves are still sequential, right?

The specific tests of commands that use Eio will still use it in the tests as well, but yeah, the tests themselves are sequential.

cknitt · 2025-09-05T13:07:46Z

Could you address Copilot's comments? Other than that, looks good to me! 👍

cristianoc

I thought it was all straightforward until I saw the locks.
Now the question:
why did you add locks and how do you know that we don't have deadlocks (locking too much) or data races (some locks are still missing)

Worth spending some time of this ahead of time before getting into bugs that are very hard to reproduce.

cristianoc · 2025-09-06T03:52:08Z

analysis/bin/main.ml

-      ~pos:(int_of_string line, int_of_string col)
-      ~debug
+    Eio_main.run (fun env ->
+        Commands.references ~env ~path


why does references need to take an env now and not before?

Because it's using Eio, and that requires the Eio env.

zth · 2025-09-06T08:12:03Z

I thought it was all straightforward until I saw the locks. Now the question: why did you add locks and how do you know that we don't have deadlocks (locking too much) or data races (some locks are still missing)

Worth spending some time of this ahead of time before getting into bugs that are very hard to reproduce.

There were races in accessing/writing to the cmt cache, that caused random failures of the command. I'll do another round of reasoning about the locks etc.

cristianoc · 2025-09-06T08:39:15Z

I thought it was all straightforward until I saw the locks. Now the question: why did you add locks and how do you know that we don't have deadlocks (locking too much) or data races (some locks are still missing)
Worth spending some time of this ahead of time before getting into bugs that are very hard to reproduce.

There were races in accessing/writing to the cmt cache, that caused random failures of the command. I'll do another round of reasoning about the locks etc.

How about making sure that transitively no mutable global state is accessed? Is that too hard to establish?

zth · 2025-09-06T08:41:34Z

How about making sure that transitively no mutable global state is accessed? Is that too hard to establish?

No that's probably a great idea. I'll give it a shot! It's great if we figure these things out now, because this type of Eio thing will hopefully be the foundation of quite a few things going forward.

cristianoc · 2025-09-06T08:47:24Z

How about making sure that transitively no mutable global state is accessed? Is that too hard to establish?

No that's probably a great idea. I'll give it a shot! It's great if we figure these things out now, because this type of Eio thing will hopefully be the foundation of quite a few things going forward.

So no need for locks and things would be much safer.
Though: one needs to be careful not to introduce a slowdown.

zth · 2025-09-06T12:27:01Z

@cristianoc 4352708 removes mutexes in favor of domain local caches. No meaningful regression in perf from doing this. But can't help to wonder if there's a better way.

We'll of course continue improving on this as we integrate more features with the same functionality. This first version needs to be safe and clear/understandable though of course.

nojaf · 2025-09-06T12:28:16Z

analysis/src/AnalysisCache.ml

@@ -0,0 +1,7 @@
+(* Helpers for domain-local caches *)


Any links to what a domain-local cache even is?

Domains in Eio are essentially threads but managed by Eio. So, a domain local cache is a cache that's valid just for a thread (domain).

Previously, everything was sequential and within a single thread. We can now use multiple threads (domains) for parallelizing work, but that means interacting with the cache isn't safe if it's shared between all threads (races etc). So, by doing domain local caches, we dodge that issue.

For the exact tasks in this PR (rename and find references), the cache doesn't matter that much I suspect. But since it was already in place, and it's used in a bunch of other places (where we don't use Eio yet, and maybe won't ever do), it made sense to make them domain specific.

This should be "backwards compatible" too from what I understand, in that the cache will work the same way as it did before for the non-Eio stuff.

Thanks a bunch for explaining this!

Domains in Eio are essentially threads but managed by Eio. So, a domain local cache is a cache that's valid just for a thread (domain).

Previously, everything was sequential and within a single thread. We can now use multiple threads (domains) for parallelizing work, but that means interacting with the cache isn't safe if it's shared between all threads (races etc). So, by doing domain local caches, we dodge that issue.

For the exact tasks in this PR (rename and find references), the cache doesn't matter that much I suspect. But since it was already in place, and it's used in a bunch of other places (where we don't use Eio yet, and maybe won't ever do), it made sense to make them domain specific.

This should be "backwards compatible" too from what I understand, in that the cache will work the same way as it did before for the non-Eio stuff.

Can you try something: create intentionally some shared mutable state and use domain local cache and check that they don't interfere.
Maybe in a file of only a few lines.

use Eio for parllelising the find references command

8c01864

zth requested a review from Copilot September 4, 2025 10:29

This comment was marked as outdated.

Sign in to view

properly lock

951b61d

nojaf reviewed Sep 4, 2025

View reviewed changes

zth added 8 commits September 4, 2025 13:55

refactor

efec169

add eio to dune

ea3636f

move required OCaml version to 5.3

4fc600e

add dep

63ceb38

attempt to fix Eio setup

2357c2e

try to fix CI

58d1393

ci

966836b

ci

b956b14

zth commented Sep 5, 2025

View reviewed changes

cknitt reviewed Sep 5, 2025

View reviewed changes

zth added 5 commits September 5, 2025 11:07

try removing explicit eio_main dep

0663fc5

revert removing eio_main, and try removing depends 5.3 from the compi…

b265377

…ler itself

tweak

1c1e299

unecessary change

97f9e2b

changelog

4d74fcc

zth changed the title ~~[WIP] Use Eio for parallelizing some analysis commands~~ Use Eio for parallelizing some analysis commands Sep 5, 2025

zth marked this pull request as ready for review September 5, 2025 10:35

zth requested review from cristianoc, cknitt, nojaf and Copilot September 5, 2025 10:36

Copilot AI reviewed Sep 5, 2025

View reviewed changes

nojaf reviewed Sep 5, 2025

View reviewed changes

cristianoc reviewed Sep 6, 2025

View reviewed changes

get rid of global locks in favor of domain local caches

4352708

nojaf reviewed Sep 6, 2025

View reviewed changes

zth added 2 commits September 8, 2025 09:04

cleanup

8d9d7f0

domain local cache for typeToString as well

a72edaf

Use Eio for parallelizing some analysis commands #7840

Are you sure you want to change the base?

Use Eio for parallelizing some analysis commands #7840

Uh oh!

Conversation

zth commented Sep 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

nojaf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pkg-pr-new bot commented Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Copilot AI Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nojaf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cknitt commented Sep 5, 2025

Uh oh!

cristianoc left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zth commented Sep 6, 2025

Uh oh!

cristianoc commented Sep 6, 2025

Uh oh!

zth commented Sep 6, 2025

Uh oh!

cristianoc commented Sep 6, 2025

Uh oh!

zth commented Sep 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zth commented Sep 4, 2025 •

edited

Loading

pkg-pr-new bot commented Sep 5, 2025 •

edited

Loading

cristianoc left a comment •

edited

Loading