-
Notifications
You must be signed in to change notification settings - Fork 2.2k
fix(prune): avoid extra iterator consumption #19758
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
fix(prune): avoid extra iterator consumption #19758
Conversation
mattsse
left a comment
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.
this looks correct
I have one style suggestion
crates/prune/prune/src/db_ext.rs
Outdated
| let done; | ||
| loop { |
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.
can we changes this so that we use .peekable on keys.into_iter().peekable()
and then
while keys.peek().is_some() {
if limiter.is_limit_reached() {
...
key = keys.next()
}
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.
agreed, because I think with these changes we also incorrectly set done = false; if both the limit is reached and there's no more keys left to process.
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.
can we changes this so that we use
.peekableonkeys.into_iter().peekable()and then
while keys.peek().is_some() { if limiter.is_limit_reached() { ... key = keys.next() }
Like this? corrected
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.
@mattsse maybe we can even use https://doc.rust-lang.org/std/iter/struct.Peekable.html#method.next_if here?
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.
yeah that's also an option @Forostovec
CodSpeed Performance ReportMerging #19758 will not alter performanceComparing Summary
|
mattsse
left a comment
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.
this lgtm, ty!
This reverts commit cf96d67.
What was wrong:
Double consumption on early exit: the for key in &mut keys loop consumes the next item before the body runs. If the limiter triggers at loop start, we break, but the current key is already consumed. The subsequent keys.next().is_none() after the loop consumed yet another key.
Misleading done flag: done was derived by advancing the iterator after the loop, not by whether the loop actually exhausted it.
Why this happens
forimplicitly callsnext()before each iteration.The extra
next()to determine done advanced the iterator once more, skipping work and misreporting completion.What changed
for key in &mut keyswith an explicit loop that checkslimiter.is_limit_reached()before calling keys.next().done = trueonly when keys.next() returnsNone.done = false.