Introduce RefUnwindSafe StorageHandle#675
Merged
Veykril merged 2 commits intosalsa-rs:masterfrom Feb 17, 2025
Merged
Conversation
✅ Deploy Preview for salsa-rs canceled.
|
CodSpeed Performance ReportMerging #675 will not alter performanceComparing Summary
|
25154f7 to
4e44751
Compare
davidbarsky
approved these changes
Feb 17, 2025
Contributor
davidbarsky
left a comment
There was a problem hiding this comment.
sorry for the delay; approved modulo nits.
4e44751 to
6fef6b2
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
David noticed in rust-lang/rust-analyzer#18964 that
Storageisn't unwind safe (on a related note it also obviously isn'tsyncdue to the local state). So this is a proposed design at solving this by introducing a non-local state handle (which hence disallows running computations on it). This would allow you to have a non-local state handle, move that across unwind boundaries, threads, etc and once you need todo computations you can turn it into a handle with local state.In rust-analyzer I would imagine us holding such a non-local state handle in the main global state and whenever we dispatch a request, clone it, move the clone off to the dispatching thread and then materialize it as a local-state hanlde (
Storage).We could directly implement
UnwindSafeonStorage(note how we used to implementRefUnwindSafealready, which is the wrong trait at this stage of the setup), given that if we have no references outstanding the query stack will be empty anyways and so we wouldn't run into issues with moving it across an unwind boundary. That doesn't sit quite right with me though so instead I've opted for this setup, as it also feels like a natural extension to the API to me. A side effect of this is that the non-local handle isSync, though that is likely not too useful as there is no reason to wrap it inside (interior) mutability.I've added respective
RefUnwindSafeimplementations where required to make the auto trait impls work as expected with comments explaining the rationale as to why they ought to be correct.cc @davidbarsky