Flexible memo dropping behavior #874
Conversation
✅ 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 11.01%Comparing Summary
Benchmarks breakdown
|
Veykril
force-pushed
the
veykril/push-proytsnlytqr
branch
from
ce1baa4 to
c715adb
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Veykril
force-pushed
the
veykril/push-proytsnlytqr
branch
2 times, most recently
from
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.
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.
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
Veykril
force-pushed
the
veykril/push-proytsnlytqr
branch
from
01799e6 to
66c99be
Compare
Veykril
force-pushed
the
veykril/push-proytsnlytqr
branch
5 times, most recently
from
361410b to
d89e9ae
Compare
Veykril
changed the title
|
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. |
Veykril
force-pushed
the
veykril/push-proytsnlytqr
branch
2 times, most recently
from
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 ... |
Veykril
force-pushed
the
veykril/push-proytsnlytqr
branch
from
67280df to
053d779
Compare
Veykril
force-pushed
the
veykril/push-proytsnlytqr
branch
from
053d779 to
d5b58fb
Compare
Veykril
force-pushed
the
veykril/push-proytsnlytqr
branch
from
d5b58fb to
afa442f
Compare
Veykril
force-pushed
the
veykril/push-proytsnlytqr
branch
from
afa442f to
da94ca1
Compare
Veykril
force-pushed
the
veykril/push-proytsnlytqr
branch
from
da94ca1 to
7ab9dba
Compare
3 participants
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.