Skip to content

Conversation

@Veykril
Copy link
Member

@Veykril Veykril commented Oct 17, 2025

Introduces a CancellationToken that can be used to cancel a specific database computation opposed to cancelling the whole runtime. This is traced by storing a second cancellation state on ZalsaLocal itself opposed to the runtime cancel flag.

When a thread gets cancelled, it will unwind as usual but with a different payload than pending write cancellation, threads blocked on such a cancelled thread will instead of propagating the cancellation run the computation they are blocked on themselves.

The cancellation state of the database gets reset when we exit the database TLS.

@netlify
Copy link

netlify bot commented Oct 17, 2025

Deploy Preview for salsa-rs canceled.

Name Link
🔨 Latest commit e0ad0ff
🔍 Latest deploy log https://app.netlify.com/projects/salsa-rs/deploys/69012e858c6870000820c693

@codspeed-hq
Copy link

codspeed-hq bot commented Oct 17, 2025

CodSpeed Performance Report

Merging #1007 will degrade performances by 9.87%

Comparing Veykril:push-kwpwsmmosonq (e0ad0ff) with master (cdd0b85)

Summary

❌ 4 (👁 2) regressions
✅ 9 untouched

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark BASE HEAD Change
👁 amortized[Input] 2.2 µs 2.3 µs -6.69%
amortized[InternedInput] 2.2 µs 2.3 µs -4.22%
👁 amortized[SupertypeInput] 2.8 µs 3.1 µs -9.87%
mutating[30] 14.4 µs 15.1 µs -4.33%

@Veykril
Copy link
Member Author

Veykril commented Oct 17, 2025

Looks like allocator noise from the new arc alloc

@MichaReiser
Copy link
Contributor

Interesting. Is the idea to cancel a single thread rather than all threads? Won't this cancell all threads that currently block on any query executing on the thread being cancelled?

@Veykril
Copy link
Member Author

Veykril commented Oct 17, 2025

Is the idea to cancel a single thread rather than all threads?

Yes, the thought is that this could allow implementing client side request cancellation for LSP requests for example.

Won't this cancell all threads that currently block on any query executing on the thread being cancelled?

That is the thing I will have to investigate. We unwind with the payload, we don't panic so I believe this does not set the panicking state of the thread actually. So this might actually just work (though I doubt it).

@MichaReiser
Copy link
Contributor

That is the thing I will have to investigate. We unwind with the payload, we don't panic so I believe this does not set the panicking state of the thread actually. So this might actually just work (though I doubt it).

Oh, so other threads would reclaim and re-execute the queries then. Interesting.

@Veykril
Copy link
Member Author

Veykril commented Oct 17, 2025

That would be the ideal I think

@Veykril Veykril force-pushed the push-kwpwsmmosonq branch 3 times, most recently from db715b7 to 47b7a7d Compare October 19, 2025 16:45
@Veykril
Copy link
Member Author

Veykril commented Oct 19, 2025

I was wrong, unwinding does set the panicking flag after all (which probably makes more sense ...) hmm

@Veykril Veykril force-pushed the push-kwpwsmmosonq branch 12 times, most recently from f97346e to de32cab Compare October 26, 2025 15:52
@Veykril Veykril marked this pull request as ready for review October 26, 2025 15:52
@Veykril Veykril force-pushed the push-kwpwsmmosonq branch 3 times, most recently from ba0f832 to 2318de4 Compare October 26, 2025 16:32
@Veykril Veykril requested a review from MichaReiser October 26, 2025 17:14
@MichaReiser
Copy link
Contributor

Would mind explaining the approach in the pr summary. I'm mainly interested in how it works if other threads are blocked on a query that gets cancelled (including if they participate in a cycle)

@Veykril Veykril force-pushed the push-kwpwsmmosonq branch 3 times, most recently from d0fee43 to d2edc9b Compare October 28, 2025 20:57
@Veykril
Copy link
Member Author

Veykril commented Oct 29, 2025

Turns out I forgot to actually implemen the relevant retry part before 🤦 I did add the bool return type but didn't fix up the usages. To my surprise my test I added worked either way, which I think makes sense? Even if we don't retry if we don't propagate the panic we will just notice the value still missing in fetch cold and recompute again. Though I think this will cause two databases to compute the same query if they both were blocked. Should probably add a test for that.

Likewise I am not yet sure how to handle this for fix points yet (and need to cook up a test there as well)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants