Unify pending object and clear expired items from pending queue #3135
Conversation
raychu86
changed the title
Signed-off-by: Raymond Chu <14917648+raychu86@users.noreply.github.com>
node/bft/src/helpers/pending.rs
Outdated
| pub fn clear_expired_callbacks(&self) { | ||
| let items = self.pending.read().keys().copied().collect::<Vec<T>>(); | ||
| let now = OffsetDateTime::now_utc().unix_timestamp(); | ||
| for item in items.into_iter() { |
There was a problem hiding this comment.
once this loop is done, it might make sense to call .shrink_to_fit on self.pending, as long as we don't call this method too often
There was a problem hiding this comment.
@ljedrz We call clear_expired_callbacks quite often - on an interval, so I'd imagine we'd need some way to determine when it'd be appropriate to call shrink_to_fit.
There was a problem hiding this comment.
I don't recall having seen this collection in the past heap profiles, but I'll keep it in mind, then 👍.
| let item = item.into(); | ||
|
|
||
| // Acquire the pending lock. | ||
| let mut pending = self.pending.write(); |
There was a problem hiding this comment.
this lock is acquired in a loop; it might make sense to tweak the API so that it's done just once for the duration of the clear_expired_callbacks loop
There was a problem hiding this comment.
Resolved in ProvableHQ@964599b.
This also removes a use of collect.
@ljedrz Can you review the new change?
There was a problem hiding this comment.
Looks good 👍; we can now consider if we still need the clear_expired_callbacks_for_item method.
There was a problem hiding this comment.
We still use the clear_expired_callbacks_for_item on individual calls to num_sent_requests, num_callbacks, and insert. Primarily to clear the expired callbacks in order to fetch the correct output number.
Co-authored-by: ljedrz <ljedrz@users.noreply.github.com> Signed-off-by: Raymond Chu <14917648+raychu86@users.noreply.github.com>
|
LGTM |
Motivation
This PR unifies the
pendingandcallbacksobjects in thePendingstruct for better tracking/access.This PR also adds additional logic to properly clear the items from the
Pendingqueue. Previously, we would clear the expired callbacks, but not clear the items from thependingqueue; this leave an artifact in the queue for every unique element that was notremovedexplicitly.In addition, we were only clearing items when an
insert,remove, ornum_callbacksfunction was called for that particular item. So if for any reason, the nodes are unable to complete the pending request, the item/callbacks will stay in the queue until the node is restarted.The new changes will properly clear the expired items and periodically clear the pending queue of the expired callbacks and items. This should mitigate the incremental memory growth and also the redundant requests issue.
The
CALLBACK_EXPIRATION_IN_SECSwas also updated to prevent truncation, which should address the case where the callback expired but the fetch timeout hasn't.Related PRs
https://github.com/AleoHQ/snarkOS/pull/3088