-
Notifications
You must be signed in to change notification settings - Fork 184
Flexible memo dropping behavior #874
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for salsa-rs canceled.
|
8237471
to
ce1baa4
Compare
CodSpeed Performance ReportMerging #874 will not alter performanceComparing Summary
|
ce1baa4
to
c715adb
Compare
This comment was marked as outdated.
This comment was marked as outdated.
7be2574
to
bcca0a3
Compare
|
||
/// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't know it was legal to write unsafe fn(NonNull). I'm guessing that if you pass it a safe closure, this will fail to compile?
No, safe functions/closures can be coerced to unsafe ones just fine. After all calling a safe function in an unsafe context isn't problematic. Only the other direction won't work for obvious reasons.
The other two questions won't be relevant anymore once im finished writing out the new approach
01799e6
to
66c99be
Compare
361410b
to
d89e9ae
Compare
Okay, new approach. The user is now in charge of things. We now have two dropping modes, synchronous (what we currently have) and channel based, where the user will receive a receiver on storage construction that they can poll to drop things as they like. |
ba28ff2
to
67280df
Compare
So, delaying the LRU clears is actually a bit more difficult and as it turns out, my previous approach was even unsound. We need to remove the values immediately, as otherwise we might be racing with computations of the next revision. That is a bit unfortunate though, as the values are not necessarily allocated and so we can't just take the pointer as an opaque droppable thing. That leaves two options, allocate LRU'd values that are to be dropped elsewhere individually, or use a (paged) arena allocator and move the value into that. Neither approach seems nice to me ... |
67280df
to
053d779
Compare
053d779
to
d5b58fb
Compare
d5b58fb
to
afa442f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but I think I'd prefer someone from Astral to also approve this—conflict of interest and all.
This allows users to opt-into receiving a channel receiver on storage creation that can be used to decide when and where to drop destructors of stale memo results.
Notably this does not yet handle LRU evictions, so those are still synchronous. Handling that is more complicated or will have to incur allocations so I am not yet sure how to best approach this. We could maybe consider a double buffer (triple buffer) approach for the deletion queues per ingredient instead and if those are exhausted fall back to blocking in case of too frequent revision bumps. That is something I will consider in a separate follow up PR though, for now this is ready for a review.
Note that the perf changes are noise due to allocation order differences.