rescript-compiler icon indicating copy to clipboard operation
rescript-compiler copied to clipboard

Use Eio for parallelizing some analysis commands

Open zth opened this issue 3 months ago • 7 comments

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.

zth avatar Sep 04 '25 10:09 zth

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

pkg-pr-new[bot] avatar Sep 05 '25 08:09 pkg-pr-new[bot]

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

cknitt avatar Sep 05 '25 13:09 cknitt

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.

zth avatar Sep 06 '25 08:09 zth

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?

cristianoc avatar Sep 06 '25 08:09 cristianoc

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.

zth avatar Sep 06 '25 08:09 zth

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.

cristianoc avatar Sep 06 '25 08:09 cristianoc

@cristianoc https://github.com/rescript-lang/rescript/pull/7840/commits/43527087e9794e76b0a85a607ea61ed10bb68173 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.

zth avatar Sep 06 '25 12:09 zth