Implement multithreading for snapshot generation #7987

joshuawarner32 · 2025-07-09T23:10:16Z

Extract commonality in snapshot tests between single-file and multi-file snapshots
Implement multithreading for snapshot generation

bhansconnect · 2025-07-10T01:27:11Z

src/base/parallel.zig

+const Thread = std.Thread;
+
+/// Maximum number of threads that can be spawned
+const MAX_THREADS = 128;


Why? Systems can have more cores then this.

To simplify the setup and have a stack-allocated array of reasonable size. But fair point, it's not hard to fix up.

bhansconnect · 2025-07-10T01:27:39Z

src/base/parallel.zig

+    max_threads: usize = 0,
+
+    /// Force single-threaded execution
+    single_threaded: bool = false,


Why not just use max_threads = 1 for this?

In my mind there's a distinction between "use one [other] thread", and "use this thread". Usually not important, but for example being on the main thread can be important for certain things on macos. Also somewhat simplifies interaction with a debugger. But yeah, max_threads=1 makes sense and is a smaller api.

Ah. I was thing that max_threads=1 would just skip spawning the threads.

bhansconnect · 2025-07-10T01:29:02Z

src/base/parallel.zig

+    return struct {
+        allocator: Allocator,
+        work_items: []const T,
+        index: *std.atomic.Value(usize),


Can we make an AtomicUsize alias?

bhansconnect · 2025-07-10T01:34:59Z

src/base/parallel.zig

I'm not a fan of putting this in base necessarily. Different concurrent work would want to be mapped differently. Though maybe this is the dfault type for large compiler work and fine.

I guess a chunk size setting could also be added later

Different concurrent work would want to be mapped differently.

100% agree. I'm not intending this to be a be-all-end-all; just serve this particular simple need at the moment.

bhansconnect · 2025-07-10T01:39:06Z

src/base/parallel.zig

+/// Internal worker thread context
+fn WorkerContext(comptime T: type) type {
+    return struct {
+        allocator: Allocator,


I don't think there is a guarantee that all allocators are thread safe. Though I guess all the ones we use are...so 🤷

This is why Rust is nice 😭

bhansconnect · 2025-07-10T01:52:59Z

src/snapshot.zig

@@ -115,6 +117,8 @@ pub fn main() !void {
            generate_html = true;
        } else if (std.mem.eql(u8, arg, "--debug")) {
            debug_mode = true;
+        } else if (std.mem.eql(u8, arg, "--singlethread")) {


super nit: --single-threaded?

Also probably add an arg to set the number of threads. Like I often want one core free so my cpu doesn't hang up from parallel workers like this.

src/snapshot.zig

bhansconnect · 2025-07-10T01:54:48Z

src/snapshot.zig

    const duration_ms = timer.read() / std.time.ns_per_ms;

-    std.log.info("processed {d} snapshots in {d} ms.", .{ total_success, duration_ms });
+    std.log.info(


nit: ditto with the collected part of this log. I think it should be a debug log only.

This was very useful in that it made it clearly obvious to me that it was not worth trying to optimize the collection step. I'm tempted to leave it in in some respect, but open to rewording / simplifying.

I guess this is snapshot.zig...so it doesn't really matter. Just feels like info that would be behind a --time flag to get extra detail.

bhansconnect · 2025-07-10T01:55:49Z

src/snapshot.zig

-    // Parse the content
-    var ast = parse.parse(&module_env, roc_content);
-    defer ast.deinit(allocator);
+    // Parse the source code based on node type


Changes around here don't feel like they belong in this PR?

Happy to break this out into a separate PR (it's already a separate commit). This is just unifying a couple pieces of redundant code.

Ah, makes sense. Sounds fine. I just wanted to make sure we didn't pull in accidental changes.

bhansconnect · 2025-07-10T01:56:44Z

src/snapshot.zig

+        .max_threads = 0, // Auto-detect
+    };
+
+    const result = try parallel.processParallel(


nit: can we rename the function to just proccess. so it is called as parallel.process?

Same for any other duplicate names like that.

bhansconnect · 2025-07-10T02:00:15Z

I think some form of this is likely worth merging especially for high level dependency free stuff like format and snapshots.

…ile snapshots

bhansconnect · 2025-07-11T03:12:13Z

Anyway, all my comments are definitely minor. Clean up as you please and feel free to just merge.

joshuawarner32 force-pushed the multithread-snapshots branch from e5a4a0c to 0590de8 Compare July 9, 2025 23:37

bhansconnect previously approved these changes Jul 10, 2025

View reviewed changes

joshuawarner32 dismissed bhansconnect’s stale review via 2fac629 July 10, 2025 16:31

joshuawarner32 force-pushed the multithread-snapshots branch 3 times, most recently from 704d16c to 50ec5cd Compare July 10, 2025 19:09

joshuawarner32 added 6 commits July 10, 2025 15:54

Extract commonality in snapshot tests between single-file and multi-f…

d9af183

…ile snapshots

Implement multithreading for snapshot generation

1c72499

Simplify parallel processing abstraction to a more kernel-like system

f6cfc41

Switch to --threads argument

388d027

Minor leak fix / making things obviously correct

40208c2

Switch to arena allocation to appease valgrind

697a712

joshuawarner32 force-pushed the multithread-snapshots branch from 21efa1e to 697a712 Compare July 10, 2025 22:55

bhansconnect approved these changes Jul 11, 2025

View reviewed changes

Uh oh!

Implement multithreading for snapshot generation #7987

Are you sure you want to change the base?

Implement multithreading for snapshot generation #7987

Conversation

joshuawarner32 commented Jul 9, 2025

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!

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!

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!

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!

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!

bhansconnect commented Jul 10, 2025

Uh oh!

bhansconnect commented Jul 11, 2025

Uh oh!

Uh oh!