Use Result over catch_unwind for cancellation and cycle handling
#572
Conversation
✅ Deploy Preview for salsa-rs canceled.
|
CodSpeed Performance ReportMerging #572 will not alter performanceComparing Summary
|
|
There's the additional loss of DevX for queries that today already return |
Definitely, and that's the biggest counterargument to using Result or was the primary motivation for using |
|
Hmm, not sure what the miri errors are about. |
| impl<T, E> HasOutput for std::result::Result<T, E> { | ||
| type Output = T; | ||
| } |
There was a problem hiding this comment.
It is too bad that Specialization is a nightly-only feature. It would allow us to implement HasOutput for T and specialize it for Result. It would allow the macro to support both non-result and result return types when adding a new into_result(self) -> salsa::Result<Self::Output> method (which is a no-op for salsa::Result and returns Ok(self) for all other types.
|
The primary motivation here is WASM support -- it's probably worth poking the WASM folks too to understand if there are other options. |
|
@nikomatsakis do you know any WASM folks we could reach out to? If I remember correctly, didn't you @BurntSushi mention in one of our 1:1s that you know a WASM expert? |
|
Yeah. I can do that but I've also been thinking over what async would look like. It might work out ok, though it's not obvious to me yet how to manage cycle recovery without something like panic. It's actually an interesting gap in Rust's future trait I hadn't considered: we would need some way to "peel" a layer off the top. I guess the fact that we control all tracked functions might help. Reviewing this PR definitely made me want to find alternatives for WASM support :) |
|
Our overall sentiment is that we want to explore an async/await-based solution. We can come back to this if that exploration is unsuccessful. |
Draft PR to switch Salsa from using
catch_unwindtoResultfor "unwinding" the stack when a query is canceled or when handling cycles.Relevant Zulip Discussion
The primary motivation for switching to
Resultis that WASM doesn't supportcatch_unwind. There are other upsides to usingResult:panic = abortto get better performance and smaller binariesResulthelpful in that it makes it more obvious to users where a salsa operation can fail. For example, creating inputs or reading the fields of an interned struct never fail. Creating a tracked struct or accessing a tracked struct's fields can fail. Returning aResultalso makes it more obvious that it may be necessary to handle the failure case to avoid logical errors (e.g. when creating multiple inputs and doing queries in-between)The primary downsides of switching to
Resultare:try?)salsa::Result<T>which leads to a less ergonomic APITODO:
Should we use one or multiple result types?
The current implementation uses a single
salsa::Resulttype with error variants for cycle-handling and cancellation. I generally prefer having a single error type because it relieves me of having to think about the rightResulttype for this specific method, nor do I have to change theResulttype if I add one more accessor. However, the downside is that accessors on salsa structs have a too genericResulttype.Should we use async await instead?
@davidbarsky has floated this idea multiple times. I don't understand async Rust well enough to outline a design using
asyncandawaitfor cancellation (I suspect that a design supporting cancellation should also work for handling cycles). Buck2's dice framework uses async await and seems to support cancellation. So this might be very interesting to explore. Usingasyncwould be even more interesting if we could use the same infrastructure to support concurrency (by scheduling work as task).The main downside of using async is that it's a very heavyweight dependency and probably also comes with a fair amount of overhead which might be especially problematic for fine-grained queries and getters on salsa structs.