Rewatch: replace wave scheduler with DAG + critical-path priority#8374
Rewatch: replace wave scheduler with DAG + critical-path priority#8374jfrolich wants to merge 4 commits intorescript-lang:masterfrom
Conversation
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a22a9437d3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if !is_clean { | ||
| dirty_set.insert(dep.clone()); | ||
| } |
There was a problem hiding this comment.
When a dependency finishes with !is_clean, this code only updates the local dirty_set; it does not immediately set module.compile_dirty on the dependent. Because the dispatcher stops spawning new work after the first compile error (has_errors), any dependents that were marked dirty but never scheduled in this run can remain compile_dirty = false in build_state, so a subsequent build may skip recompiling stale modules even though an upstream .cmi changed. The previous scheduler wrote compile_dirty = true directly during propagation, which avoided this incremental-build inconsistency after failed builds.
Useful? React with 👍 / 👎.
rescript
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
Instead of compiling modules in topologically-sorted waves (which stall on the slowest file per wave), schedule them from a priority queue on a work-stealing dispatcher. Priority is the length of the module's longest downstream dependency chain, so bottlenecks start first. Workers run inside a `rayon::in_place_scope` bounded to `rayon::current_num_threads()` so the heap ordering actually decides dispatch. Completions flow back over `std::sync::mpsc`; the dispatcher updates pending-dep counters, propagates the dirty flag, and pushes newly-ready modules back onto the heap. `BuildState` mutations happen after the scope exits so worker reads and main-thread writes stay disjoint. Cycle detection now triggers when the heap drains without completing the universe. Signed-Off-By: Jaap Frolich <jaap@tella.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Restore the `.unwrap()` on `package.namespace.to_suffix()` for mlmap
dispatch so a missing namespace suffix panics loudly instead of being
silently papered over.
- Drop the defensive `.unwrap_or_default()` on the post-completion
dependents lookup and the `if *count == 0 { continue }` guard on the
pending-deps decrement; both can only fire when the graph itself is
inconsistent, and hiding that would mask bugs. Align with the
unwrap-style assertions the old scheduler used.
- Sort `results_buffer` by module name before applying results so the
returned `compile_errors` / `compile_warnings` strings and the per-
package `.compiler.log` writes are deterministic across runs despite
non-deterministic completion order.
Signed-Off-By: Jaap Frolich <jaap@tella.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
When a predecessor's cmi changes the dispatcher only marks the local dirty_set; it used to not write compile_dirty = true back onto build_state. If the first compile error aborted dispatch before those dependents were scheduled, their compile_dirty stayed false in build_state, and the next incremental build would skip recompiling them even though an upstream .cmi had changed — producing stale output. Persist dirty_set onto build_state.modules[].compile_dirty right before applying compile results. The success path in the result loop still overrides compile_dirty = false for modules that actually recompiled cleanly in this run, so only the propagated-but-not-scheduled modules retain the flag. Signed-Off-By: Jaap Frolich <jaap@tella.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
rolandpeelen
left a comment
rolandpeelen
left a comment
There was a problem hiding this comment.
Awesome find, and this makes so much sense! Can't believe we didn't think of this before.
Left one comment / optimisation, which I think is a case that happens a lot.
| } | ||
|
|
||
| /// Compute the critical-path priority of every module in the universe: | ||
| /// `priority(m) = 1 + max(priority(d) for d in in-universe dependents of m)`. |
There was a problem hiding this comment.
I think this is good, but possibly, as an experiment, we can do better;
Right now, it only computes the longest chain it unlocks. Take these examples:
- One chain of files, Result.map -> Option.map -> Array.map -- We have a single file that uses result, result uses option, and option uses array
- One broader chain of files, List.map, where we have 100 files depending on List.
In this case, I think this calculates 1 first, even though, if it would compile 2 first, it can unlock 100 other files that can be computed in parallel.
There was a problem hiding this comment.
Perhaps we can weight this:
score(M) = max(critical_path_length(M) * N, downstream_count(M))
Where N is the bias towards downstream count. So 1 would be the winner in the old case, but if we put 10 there, then if the downstream count outweighs the path length by a factor of 10, it wins and will compute that first.
Signed-Off-By: Jaap Frolich <jaap@tella.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
3 participants
Summary
par_iterper wave) with a work-stealing DAG dispatcher. Waves stall on the slowest file per round; DAG scheduling lets a module start as soon as its own dependencies finish.rewatch/src/build/compile.rs; no changes tobsc, the wire protocol, orrescript.jsonsemantics.Design
rayon::in_place_scope, bounded torayon::current_num_threads()concurrent tasks so theBinaryHeapordering actually decides dispatch (spawning everything would lose priority to rayon's deque).std::sync::mpsc. The dispatcher on the main thread:pending_depscounters for dependents,&BuildState; allBuildStatemutations (compile_state, timestamps,compile_dirty) happen after the scope exits, keeping worker reads and main-thread writes disjoint without any interior mutability.dependency_cycle::find/formatpath — same error message as before.Test plan
cargo build --manifest-path rewatch/Cargo.tomlcargo clippy --manifest-path rewatch/Cargo.toml --all-targets --all-features -- -D warningscargo fmt --check --manifest-path rewatch/Cargo.tomlcargo test --manifest-path rewatch/Cargo.toml— 68 unit tests passmake test-rewatch— full integration suite passes, including:09-dependency-cycle(cycle snapshot unchanged)13-no-infinite-loop-with-cycle16-snapshots-unchanged)Notes
clean_modules(which the old code built but never read) is gone.🤖 Generated with Claude Code