Drop memos on separate thread #660
Conversation
✅ Deploy Preview for salsa-rs canceled.
|
CodSpeed Performance ReportMerging #660 will not alter performanceComparing Summary
|
Veykril
force-pushed
the
veykril/push-proytsnlytqr
branch
3 times, most recently
from
0bb7e71 to
b740ae4
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
fe9f121 to
50dc4c9
Compare
|
I removed the LRU changes from this PR that were unrelated to the eviction changes, slimming down things a bit -> #664 |
Veykril
force-pushed
the
veykril/push-proytsnlytqr
branch
2 times, most recently
from
8522755 to
d4aa44b
Compare
Veykril
changed the title
Veykril
force-pushed
the
veykril/push-proytsnlytqr
branch
from
d4aa44b to
1d5beef
Compare
|
Rebased on top of https://github.com/salsa-rs/salsa/pull/673/files which was split out of this PR given its a soundness fix |
Veykril
force-pushed
the
veykril/push-proytsnlytqr
branch
3 times, most recently
from
5fdf4c8 to
b37e8a9
Compare
|
It's somewhat interesting that this changes the performance so significantly (maybe just noise?) |
|
The numbers seem to be consistent (judging across my rebases), so I think this might just be due to more favorable code layout due to the changes, given the only thing that really changed for the benches is one branch being removed in a hot path (none of them bench eviction/lru/dropping of the database) |
|
Two more things I'd like to follow up with, setting the LRU capacity should require mutable database access. There is no reason I can see as to why one should be allowed to change the limit mid computation. That change allows to remove an atomic load. The second thing is we should be able to trigger eviction without bumping the revision. I won't include those changes here though as there is already enough in this PR. |
Veykril
force-pushed
the
veykril/push-proytsnlytqr
branch
from
b37e8a9 to
164ed5a
Compare
Makes sense. I assume that would still require a |
|
Yes, otherwise there might be some computation still going on that could be borrowing from a memo. |
This change makes us LRU evict once a new revision starts, instead of at the very moment we reach the limit while computing tracked functions. Notably the previous behavior was unsound as we were immediately clearing the value (without going through the delayed deletion buffer) while there could have been outstanding references.
Callers of these functions need to ensure that the value will be dropped when the revision ends
Veykril
force-pushed
the
veykril/push-proytsnlytqr
branch
from
164ed5a to
ce4af66
Compare
Veykril
changed the title
| } | ||
|
|
||
| pub fn memo_drop_channel() -> (MemoDropSender, MemoDropReceiver) { | ||
| let (tx, rx) = std::sync::mpsc::channel(); |
There was a problem hiding this comment.
Should we use a sync_channel instead to apply back pressure if the drop thread is getting overwhelmed?
What happens if a drop implementatio panics? I assume that leaves the entire database in a poisoned state because the drop thread is now "broken"?
There was a problem hiding this comment.
Good question regarding backpressure, unsure :)
What happens if a drop implementatio panics? I assume that leaves the entire database in a poisoned state because the drop thread is now "broken"?
Bad things. I have actually forgotten about destructors panicking, right now that will tear down the dropper thread and cause us to panic when evicting. It should be simple enough to workaround though with a catch_unwind, the bigger question then though becomes if its fine for us to silently eat panic payloads in destructors ...
|
I didn't split the PR into two as the second PR would be stacked either way but I've come to the realization yesterday that the dropping change has a lot of extra implications that need to be discussed more so I'll split this PR in two later |
Veykril
changed the title
|
Split out #684 |
Stacked on top of #684
Notably this replaces the
SeqQueuewith the standard libraries mpsc which is also implemented as a seqmented list, this simplified things a bit. We might want to consider moving to a different channel implementation for performance reasons, be itcrossbeamor something else.