feat(deferred-persist): Part 1: unpersisted gauge #1116
Merged
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
rkuris
added
observability
rkuris
changed the title
rkuris
force-pushed
the
rkuris/track-unwritten-nodes
branch
from
dff9f97 to
aaf968f
Compare
This comment was marked as resolved.
This comment was marked as resolved.
rkuris
force-pushed
the
rkuris/track-unwritten-nodes
branch
3 times, most recently
from
56826d7 to
e53e961
Compare
rkuris
force-pushed
the
rkuris/track-unwritten-nodes
branch
from
308f06a to
7b4c667
Compare
rkuris
force-pushed
the
rkuris/track-unwritten-nodes
branch
from
7b4c667 to
bb4826f
Compare
demosdemon
reviewed
Jul 24, 2025
Comment on lines
+372
to
+375
| unwritten_nodes: AtomicUsize::new( | ||
| self.unwritten_nodes | ||
| .load(std::sync::atomic::Ordering::Relaxed), | ||
| ), |
Contributor
There was a problem hiding this comment.
Confirming that this is intentional the the cloned committed is using a new atomic integer and isn't re-using the integer from the previous cloned. The diverging committed nodes will have different unwritten_nodes counts.
Member
Author
There was a problem hiding this comment.
Good catch, it's not correct. I'll remove this code as it's dead anyway; we never clone Committed.
demosdemon
reviewed
Jul 24, 2025
|
|
||
| let immutable_proposal = | ||
| Arc::into_inner(nodestore.kind).expect("no other references to the proposal"); | ||
| // Use ManuallyDrop to prevent Drop from running since we're replacing the proposal |
Contributor
There was a problem hiding this comment.
can you expand the comment to explain the implications of not using ManuallyDrop here?
demosdemon
approved these changes
Jul 24, 2025
demosdemon
reviewed
Jul 24, 2025
storage/src/nodestore/persist.rs
Outdated
| // Decrement gauge for writes that have actually completed | ||
| if completed_writes > 0 { | ||
| #[expect(clippy::cast_possible_truncation)] | ||
| #[expect(clippy::cast_possible_truncation, clippy::cast_precision_loss)] |
Contributor
There was a problem hiding this comment.
not understanding the lint problems here. some targets are triggering it and some aren't.
Contributor
There was a problem hiding this comment.
let's switch to allow and I'll research this next week (#1135)
Added a gauge to track the number of unpersisted nodes at any time. These can get used later by the revision manager to decide whether or not to persist a proposal (among other possible strategies). It's also helpful to know how many nodes are still in memory that could be persisted. When increasing this counter, we do it all at once, but we decrement one by one (or by the number of entries from the kernel's completion queue). This is because the writes are slower than the adds, and there is some integer to floating point math that happens each time the counter is adjusted.
rkuris
force-pushed
the
rkuris/track-unwritten-nodes
branch
from
8dcfe36 to
57cdd41
Compare
These seem to confuse clippy in CI
rkuris
force-pushed
the
rkuris/track-unwritten-nodes
branch
from
900779f to
1ea7a63
Compare
bernard-avalabs
pushed a commit
that referenced
this pull request
Aug 10, 2025
Co-authored-by: Brandon LeBlanc <brandon.leblanc@avalabs.org>
rkuris
added a commit
that referenced
this pull request
Sep 26, 2025
The use of ManuallyDrop seems to cause a leak. Let's fix this correctly later.
rkuris
added a commit
that referenced
this pull request
Sep 26, 2025
The use of ManuallyDrop seems to cause a leak. Let's fix this correctly later.
demosdemon
pushed a commit
that referenced
this pull request
Sep 26, 2025
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.
No description provided.