Overlay V2 cleanup#5296
Open
drebelsky wants to merge 4 commits into
Open
Conversation
Contributor
Author
|
For many of the LRU caches without cleanup, it might be worth considering switching to some form of TTLCache (although, the decreased cache locality from lru probably isn't that substantial in our workload). |
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
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.
The goal of this PR is to create a more stable baseline for experiments to compare against. In particular, the goal is to reach a steady state with data sizes being capped and data being regularly cleaned up.
Changes
RemoveTxsFromMempool(which is called on ledger close) now also callsevict_expiredto remove stale TXsPendingRequests::process_timeoutsnow evicts the hashes that were given up onInvTrackerTxSetCacheand re-order eviction after insertion.Not changed since we're assuming non-malicious peers + a reasonably bounded number of peers
InvBatcherkeeps an entry in the map for every peer that ever existedSharedState::peer_streams(unbounded, but limited to total number of connected peers)App::pending_scp_state_requests(unbounded, but as long as nodes aren't falling out of sync, should only have at most one entry per peer (from the message sent on start up))App::{known_peers, peer_hostnames, configured_peers}: all of these are bounded by the number of configured peersApp::local_addrs: there shouldn't be too many local addressesThe following LRU cache sizes remain unchanged and cleanup remains just LRU eviction. The sizes are small enough that I think we should hit the steady state full usage relatively quickly (although, I'm still examining the sizes for
scp_seenandscp_sent_to)InvTracker's cache(s)SharedState::{scp_seen, tx_seen, scp_sent_to, tx_set_sources}TxBufferOtherwise unchanged:
SharedState::pending_txset_requests: if the request isn't responded to, these continue to take up space. For benchmarking, this should only happen if a peer disconnects, in which case they do get cleaned up, and we try to fetch from the next peer. This is probably worth addressing with some reasonable timeouts later.