Replace loom with shuttle by ibraheemdev · Pull Request #876 · salsa-rs/salsa

ibraheemdev · 2025-05-23T13:30:35Z

Loom is too slow to run even basic tests (#845), so shuttle should be a lot more useful for us. I added a CI check to run our parallel tests under shuttle. This also undoes some of the changes from #842, mainly shuttle does not require an UnsafeCell wrapper.

netlify · 2025-05-23T13:30:40Z

✅ Deploy Preview for salsa-rs canceled.

Name	Link
🔨 Latest commit	`b1f118a`
🔍 Latest deploy log	https://app.netlify.com/projects/salsa-rs/deploys/683091d7370e5e0008da2246

codspeed-hq · 2025-05-23T13:32:32Z

CodSpeed Performance Report

Merging #876 will not alter performance

_{Comparing ibraheemdev:ibraheem/shuttle (b1f118a) with master (f7b0856)}

Summary

✅ 12 untouched benchmarks

MichaReiser

This is awesome. I also love that this simplifies the code again. I've a small comment but I think this is ready to go

MichaReiser · 2025-05-23T13:38:27Z

src/table.rs

+        #[cfg(feature = "shuttle")]
+        let data = unsafe {
+            let data = (0..PAGE_LEN)
+                .map(|_| UnsafeCell::new(MaybeUninit::uninit()))
+                .collect::<Box<[PageDataEntry<T>]>>();


We could consider using this branch in debug builds because Rust doesn't inline the call without optimizations enabled. We've seen this in ty where the Project table takes 0.5 of stack frame!

MichaReiser · 2025-05-23T13:39:10Z

src/tracked_struct.rs

-            }
-            if current_deps.durability < data.durability {
-                data.revisions = C::new_revisions(current_deps.changed_at);
+        // UNSAFE: Marking as mut requires exclusive access for the duration of


I didn't review those changes too carefully because I understand that these are just reverting earlier changes?

Yeah the changes are mostly just indentation because the with closure is removed.

MichaReiser · 2025-05-23T13:44:35Z

tests/parallel/signal.rs

        // synchronous.
        if stage > 0 {
-            let mut v = self.value.lock();
+            let mut v = self.value.lock().unwrap();


I wonder if we should disable the signals in shuttle tests? The signals are to construct a very specific scenario, defeating the benefit of shuttle - testing many possible scenarios.

ibraheemdev · 2025-05-23T13:59:05Z

It looks like shuttle caught a failure in the cycle tests!

davidbarsky · 2025-05-23T14:14:35Z

Here's the failing tests:

test panicked in task 'main-thread'
failing schedule:
"
91028003c1bbbd9085d996cdd101000000004948a22449a22492922849924449122949122589
244952a22449[1249](https://github.com/salsa-rs/salsa/actions/runs/15212059093/job/42788411582?pr=876#step:4:1250)49942449922491242589922449a224499424491245922449929224499224
4992244992244992244992244992244992244992244992249122494a12498924499224495212
4949224992244592244952924489924892244949222581244992244992244992244992244992
244992244912
"
pass that string to `shuttle::replay` to replay the failure

thread 'cycle_a_t1_b_t2_fallback::the_test' panicked at tests/parallel/cycle_a_t1_b_t2_fallback.rs:64:9:
assertion `left == right` failed
  left: (1, 9)
 right: (1, 2)
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
2025-05-23T14:00:12.752542Z ERROR execution{i=241}:step{task=0}: generator::detail::gen: set panic inside generator    
2025-05-23T14:00:12.752688Z  INFO shuttle::scheduler::metrics: run finished iterations=242 steps=[min=295, max=384, avg=314.1] context_switches=[min=4, max=62, avg=48.7] preemptions=[min=0, max=54, avg=41.6] random_choices=[min=0, max=0, avg=0.0]

I dunno what you're planning on doing, but maybe we should just land this branch and fix failing tests as we build out the Shuttle test suite?

MichaReiser · 2025-05-23T14:15:23Z

I dunno what you're planning on doing, but maybe we should just land this branch and fix failing tests as we build out the Shuttle test suite?

I plan to look into this next week

tests/parallel/main.rs

tests/parallel/signal.rs

davidbarsky · 2025-05-23T14:19:25Z

tests/parallel/cycle_panic.rs

@@ -1,3 +1,6 @@
+// Shuttle doesn't like panics inside of its runtime.


how fixable is this limit in shuttle?

I'm not sure, it looks like shuttle has a panic hook that cancels the execution immediately if anything panics. It seems like this is pretty fundamental to replay support.

davidbarsky · 2025-05-23T14:23:16Z

I dunno what you're planning on doing, but maybe we should just land this branch and fix failing tests as we build out the Shuttle test suite?

I plan to look into this next week

sure! just so that I understand: do you want to fix the failing test prior to landing this PR or do you want this PR to fix the already-surfaced test failure?

(I ask because I'm sure Shuttle can find many more failing tests)

MichaReiser · 2025-05-23T14:27:48Z

sure! just so that I understand: do you want to fix the failing test prior to landing this PR or do you want this PR to fix the already-surfaced test failure?

It depends on how much progress I make ;) I'm also fine diabling this specific test for now and landing this PR

(I ask because I'm sure Shuttle can find many more failing tests)

Sure, but it depends on if we land new changes :)

davidbarsky · 2025-05-23T14:34:58Z

Sure, but it depends on if we land new changes :)

Sorry! I'm assuming that we'd be adding new tests/features to Salsa while CI remains red/broken due to failures surfaced by the Shuttle test job. I don't think this is meaningfully different from today's state.

ibraheemdev · 2025-05-23T15:19:25Z

We can ignore the one failing test for now.

MichaReiser · 2025-05-23T15:22:35Z

This is excellent. Thank you so much for working on this.

MichaReiser · 2025-05-23T17:31:59Z

I just noticed that the bug is with fallback immediate. We don't use that. I'll leave this to someone else to fix.

Veykril · 2025-05-23T19:48:59Z

Is the bug merely shuttle not liking how fallback immediate works or is there an actual bug that shuttle found? cc @ChayimFriedman2

MichaReiser · 2025-05-23T20:01:10Z

Is the bug merely shuttle not liking how fallback immediate works or is there an actual bug that shuttle found? cc @ChayimFriedman2

I'm fairly certain this is an actual bug

ChayimFriedman2 · 2025-05-24T19:36:37Z

@MichaReiser do you have more details?

MichaReiser · 2025-05-25T07:34:27Z

@MichaReiser do you have more details?

See #876 (comment)

You need to comment out the not shuttle gating in that test, then run it with the shuttle feature enabled

I didn't look into why it's crashing but the backtrace is very short.

ibraheemdev force-pushed the ibraheem/shuttle branch 3 times, most recently from 16c320c to e430505 Compare May 23, 2025 13:39

MichaReiser approved these changes May 23, 2025

View reviewed changes

ibraheemdev force-pushed the ibraheem/shuttle branch from e430505 to aaa43a5 Compare May 23, 2025 13:41

MichaReiser reviewed May 23, 2025

View reviewed changes

ibraheemdev force-pushed the ibraheem/shuttle branch 2 times, most recently from 249b2a1 to 273e915 Compare May 23, 2025 13:58

davidbarsky reviewed May 23, 2025

View reviewed changes

ibraheemdev force-pushed the ibraheem/shuttle branch 2 times, most recently from bc45ce4 to 0a03f77 Compare May 23, 2025 15:08

replace loom with shuttle

b21e5fb

ibraheemdev force-pushed the ibraheem/shuttle branch from 0a03f77 to b21e5fb Compare May 23, 2025 15:11

ibraheemdev added 2 commits May 23, 2025 11:16

inline empty_cycle_heads

fc38e7f

ignore failing shuttle test

b1f118a

MichaReiser enabled auto-merge May 23, 2025 15:22

MichaReiser added this pull request to the merge queue May 23, 2025

Merged via the queue into salsa-rs:master with commit 0414d89 May 23, 2025
12 checks passed

github-actions bot mentioned this pull request May 23, 2025

chore: release v0.23.0 #877

Merged

MichaReiser mentioned this pull request Jun 1, 2025

CycleStrategy::FallbackImmediate: Investigate shuttle failures #893

Closed

github-actions bot mentioned this pull request Jun 28, 2025

chore: release v0.24.0 #929

Merged

		@@ -1,3 +1,6 @@
		// Shuttle doesn't like panics inside of its runtime.

Comments

Conversation

ibraheemdev commented May 23, 2025

Uh oh!

netlify bot commented May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for salsa-rs canceled.

Uh oh!

codspeed-hq bot commented May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CodSpeed Performance Report

Merging #876 will not alter performance

Summary

Uh oh!

MichaReiser left a comment

Choose a reason for hiding this comment

Uh oh!

MichaReiser May 23, 2025

Choose a reason for hiding this comment

Uh oh!

MichaReiser May 23, 2025

Choose a reason for hiding this comment

Uh oh!

ibraheemdev May 23, 2025

Choose a reason for hiding this comment

Uh oh!

MichaReiser May 23, 2025

Choose a reason for hiding this comment

Uh oh!

ibraheemdev commented May 23, 2025

Uh oh!

davidbarsky commented May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MichaReiser commented May 23, 2025

Uh oh!

Uh oh!

Uh oh!

davidbarsky May 23, 2025

Choose a reason for hiding this comment

Uh oh!

ibraheemdev May 23, 2025 • 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!

davidbarsky commented May 23, 2025

Uh oh!

MichaReiser commented May 23, 2025

Uh oh!

davidbarsky commented May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ibraheemdev commented May 23, 2025

Uh oh!

MichaReiser commented May 23, 2025

Uh oh!

Uh oh!

MichaReiser commented May 23, 2025

Uh oh!

Veykril commented May 23, 2025

Uh oh!

MichaReiser commented May 23, 2025

Uh oh!

ChayimFriedman2 commented May 24, 2025

Uh oh!

MichaReiser commented May 25, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

netlify bot commented May 23, 2025 •

edited

Loading

codspeed-hq bot commented May 23, 2025 •

edited

Loading

davidbarsky commented May 23, 2025 •

edited

Loading

ibraheemdev May 23, 2025 •

edited

Loading

davidbarsky commented May 23, 2025 •

edited

Loading