Move memo dropping off to another thread #874
Draft
Conversation
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
✅ Deploy Preview for salsa-rs canceled.
|
Veykril
force-pushed
the
veykril/push-proytsnlytqr
branch
2 times, most recently
from
8237471 to
ce1baa4
Compare
CodSpeed Performance ReportMerging #874 will degrade performances by 10.07%Comparing Summary
Benchmarks breakdown
|
Veykril
force-pushed
the
veykril/push-proytsnlytqr
branch
from
ce1baa4 to
c715adb
Compare
|
Regressions are kind of expected given we no longer keep the boxcar allocation from before. |
Veykril
force-pushed
the
veykril/push-proytsnlytqr
branch
2 times, most recently
from
7be2574 to
bcca0a3
Compare
davidbarsky
reviewed
May 27, 2025
| false, | ||
| if tracing::enabled!(Level::DEBUG) { | ||
| Some(Box::new(|event| { | ||
| tracing::debug!("salsa_event({:?})", event) |
There was a problem hiding this comment.
Since you're touching this:
Suggested change
| tracing::debug!("salsa_event({:?})", event) | |
| tracing::debug!(?event, "salsa_event") |
| @@ -1,9 +1,18 @@ | |||
| use std::mem; | |||
|
|
|||
| use crossbeam_utils::sync::{Parker, Unparker}; | |||
There was a problem hiding this comment.
might be too much of a lift, but is there any chance you can replace these with shuttle's park/unpark primitives? I assume you'll need to replicate crossbeam_utils in the sync module, unfortunately.
|
|
||
| /// An untyped memo that can only be dropped. | ||
| #[derive(Debug)] | ||
| pub struct MemoDrop(NonNull<DummyMemo>, unsafe fn(NonNull<DummyMemo>)); |
There was a problem hiding this comment.
Three things:
- I didn't know it was legal to write
unsafe fn(NonNull<DummyMemo>). I'm guessing that if you pass it a safe closure, this will fail to compile? - Can you make these named fields instead of tuple structs?
- I really think this should be tested with Shuttle. if you're okay with me pushing to your branch, i can do that?
Veykril
force-pushed
the
veykril/push-proytsnlytqr
branch
from
01799e6 to
66c99be
Compare
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.
Design considerations to be solved:
Still needs a test