refactor: replace PersistSemaphore with PersistChannel#1710
refactor: replace PersistSemaphore with PersistChannel#1710RodrigoVillar wants to merge 9 commits intomainfrom
PersistSemaphore with PersistChannel#1710Conversation
PersistSemaphore with locks and condvars
a0d2f34 to
d63cb45
Compare
9757aa7 to
0a22f43
Compare
PersistSemaphore with locks and condvarsPersistSemaphore with PersistChannel
bernard-avalabs
left a comment
There was a problem hiding this comment.
A few minor suggestions.
a1dab9d to
b72ad76
Compare
b72ad76 to
4c33dee
Compare
4c33dee to
8154a8c
Compare
rkuris
left a comment
There was a problem hiding this comment.
Mostly nits and some questions. Please re-request review if you make substantial changes.
8154a8c to
9590622
Compare
This version might be slightly more efficient and has tighter backpressure at the cost of a higher synchronization complexity. IMO the channel version is more flexible and straightforward for message ordering and multiple work types. It does have more moving parts and ends out creating a bit more work for the worker thread.
9590622 to
fef1e1f
Compare
demosdemon
left a comment
There was a problem hiding this comment.
just nits on overflowing arithmetic
| let persist_interval = NonZeroU64::new(commit_count.get().div_ceil(2)) | ||
| .expect("a nonzero div_ceil(2) is always positive"); | ||
| let persist_threshold = commit_count.get().saturating_sub(persist_interval.get()); |
There was a problem hiding this comment.
nits:
- the
uwnrapshould not lint anymore inside theconst { ... }block anddiv_ceilis defined forNonZero<_>that returns aNonZero<_>. - wrapping_sub is fine here because we logically know that
persist_intervalis strictly less than or equal tocommit_countso the subtraction will never wrap.
| let persist_interval = NonZeroU64::new(commit_count.get().div_ceil(2)) | |
| .expect("a nonzero div_ceil(2) is always positive"); | |
| let persist_threshold = commit_count.get().saturating_sub(persist_interval.get()); | |
| let persist_interval = commit_count.div_ceil(const { NonZeroU64::new(2).unwrap() }); | |
| let persist_threshold = commit_count.get().wrapping_sub(persist_interval.get()); |
It's important to be aware of the mathematical operations you're doing and not blanket chose saturating_, wrapping_, or checked_. saturating and checked operations have overhead that we can and should skip if we know our logical operations won't trigger overflow.
| } | ||
|
|
||
| state.latest_committed = Some(revision); | ||
| state.permits_available = state.permits_available.saturating_sub(1); |
There was a problem hiding this comment.
another example here. The compiler might actually skip the extra work from saturation because we have an earlier check that permits_available is not zero. However, that condition is not exclusive to letting control flow to here, so the compiler might conservatively check again. However, because we know that because state.shutdown is false at this point, we also know that state.permits_available is non-zero and saturation is extra work.
Why this should be merged
Quoting #1694:
How this works
Replaces the
PersistSemaphore+crossbeam::channelwithPersistChannel, a channel-like abstraction around the locks/condvars mentioned in #1694. By usingPersistChannel, we no longer have the issue of thePersistWorkerand thePersistLoophaving their own notions of progress as all progress is managed viaPersistChannelState.This PR also adds a drop guard to the
PersistLoop, such that if the background thread exits for whatever reason, the system is marked as shutdown which prevents thePersistWorkerfrom hanging.How this was tested
CI + existing deferred persistence UTs