CAT Part 1: assemble subcomponents: cache's, store and request manager #941
Conversation
evan-forbes
requested review from
a team,
mojtaba-esk and
rootulp
and removed request for
a team and
mojtaba-esk
There was a problem hiding this comment.
only had one question beyond that of what was already covered in #935
I still like the separate cache for evictions a lot 🤷 lol
Co-authored-by: CHAMI Rachid <[email protected]>
There was a problem hiding this comment.
I am currently going through some of the pull requests that are related to my upcoming tasks, to familiarize myself with the codebase. I'm not able to give a detailed review at this time, as I am still gathering more information and background. I'll make some non-critical suggestions and comments as I read through the code. :)
The PR looks good to me!
mempool/cat/cache.go
Outdated
| func NewEvictedTxCache(size int) *EvictedTxCache { | ||
| return &EvictedTxCache{ | ||
| staticSize: size, | ||
| cache: make(map[types.TxKey]*EvictedTxInfo), |
There was a problem hiding this comment.
Can you please explain why don't we allocate the cache size according to the supplied size? similar to what is done for the LRUTxCache.
There was a problem hiding this comment.
Yeah good point. I will add the capacity.
Note that the capacity should be size + 1 because we add before running the check to evict the last element.
We could also do the same thing as the other cache and add a list here to avoid needing to run a for loop over all items whenever we need to evict but this is not on the hot path so I don't think the tradeoff makes sense.
There was a problem hiding this comment.
We could also do the same thing as the other cache and add a list here to avoid needing to run a for loop over all items whenever we need to evict but this is not on the hot path so I don't think the tradeoff makes sense.
If the cache becomes full, then this linear search i.e., the loop takes place for every push operation, and the performance degradation MAY be noticeable. However, it depends on how frequent elements are pushed to the EvictedTxCache as well as the cache size.
There was a problem hiding this comment.
Btw, I am not suggesting to implement the cache optimization in this PR :) , just something to have in mind for the future.
There was a problem hiding this comment.
Yeah it seemed apparent from the initial design of the priority mempool that eviction performance wasn't an important characteristic. For example, you may notice that for eviction, the mempool also perform a sort.Slice on ALL txs currently in the mempool to know which transactions have the lowest priority and should therefore be evicted.
We have metrics tracking the number of evicted transactions so we can track how frequent this occurs and if it becomes a problem in the future, then we can modify these parts
There was a problem hiding this comment.
We have metrics tracking the number of evicted transactions so we can track how frequent this occurs and if it becomes a problem in the future, then we can modify these parts
That would be helpful.
There was a problem hiding this comment.
I'd suggest creating a GH issue (if we don't have one already) to keep track of any performance improvements we come across for our future reference.
mempool/cat/cache.go
Outdated
| } | ||
|
|
||
| type timestampedPeerSet struct { | ||
| peers map[uint16]bool |
There was a problem hiding this comment.
The uint16 value maps to the index of a peer in the peerMap field of a mempoolIDs, right? If we could use the p2p.ID instead (as it seems to uniquely identify a peer),then we could avoid situations where there are two entires for the same peer in the peer set e.g., the connection of a peer drops and it re-connects.
There was a problem hiding this comment.
We should just clean up all ephemeral peer data like this whenever the connection drops.
If reconnection was a common occurrence we could look into keeping the mapping around for a period after the connection closed and thus giving the peer the same uint16 if it were to reconnect.
There was a problem hiding this comment.
We should just clean up all ephemeral peer data like this whenever the connection drops.
Right
If reconnection was a common occurrence we could look into keeping the mapping around for a period after the connection closed and thus giving the peer the same uint16 if it were to reconnect.
This works too, just need an extra book keeping for the index <> p2p.ID mapping (compared to using just p2p.ID and no index)
| staticSize int | ||
|
|
||
| mtx tmsync.Mutex | ||
| // cacheMap is used as a quick look up table | ||
| cacheMap map[types.TxKey]*list.Element | ||
| // list is a doubly linked list used to capture the FIFO nature of the cache | ||
| list *list.List |
There was a problem hiding this comment.
I think based on a similar separation in other structs in this PR, the immutability is an agreement but is not enforceable. If that is the case, then I'd suggest making it explicit in the struct description as well:
| staticSize int | |
| mtx tmsync.Mutex | |
| // cacheMap is used as a quick look up table | |
| cacheMap map[types.TxKey]*list.Element | |
| // list is a doubly linked list used to capture the FIFO nature of the cache | |
| list *list.List | |
| // these fields are immutable | |
| staticSize int | |
| // these fields are protected by the mutex | |
| mtx tmsync.Mutex | |
| // cacheMap is used as a quick look up table | |
| cacheMap map[types.TxKey]*list.Element | |
| // list is a doubly linked list used to capture the FIFO nature of the cache | |
| list *list.List |
| // simple, thread-safe in memory store for transactions | ||
| type store struct { | ||
| mtx sync.RWMutex | ||
| bytes int64 |
There was a problem hiding this comment.
| bytes int64 | |
| bytes int64 // size of store in bytes |
| if tx, exists := s.txs[wtx.key]; !exists || tx.height == -1 { | ||
| s.txs[wtx.key] = wtx | ||
| s.bytes += wtx.size() | ||
| return true | ||
| } | ||
| return false |
There was a problem hiding this comment.
Just a suggestion, as a best practice in Go, it's generally considered a good idea to not have extra indentation in the "happy path" of your code. It's not a blocking issue though!
| if tx, exists := s.txs[wtx.key]; !exists || tx.height == -1 { | |
| s.txs[wtx.key] = wtx | |
| s.bytes += wtx.size() | |
| return true | |
| } | |
| return false | |
| tx, exists := s.txs[wtx.key] | |
| if exists && tx.height != -1 { | |
| return false | |
| } | |
| s.txs[wtx.key] = wtx | |
| s.bytes += wtx.size() | |
| return true | |
| } | |
| } | ||
|
|
||
| func (s *store) set(wtx *wrappedTx) bool { | ||
| if wtx == nil { |
There was a problem hiding this comment.
I'm wondering if the nil value is something we're expecting or if it might be indicating an error in the state of the calling function. If it's an error, it might be helpful to not silence it and instead return an error message along with the bool so the calling function knows exactly what's going on.
There was a problem hiding this comment.
Yeah that's a good point. I think it's just a defensive thing but you're right that if it did happen for some reason it would go undetected. Perhaps it's better if we panic.
|
I'm going to merge this now and will address some of the minor comments in the next PR. |
ref: #935