network/p2p/gossip: refactor Set.Add to accept multiple elements #2986
Conversation
lebdron
requested review from
danlaine,
joshua-kim and
StephenButtolph
as code owners
StephenButtolph
added
networking
network/p2p/gossip/gossipable.go
Outdated
| @@ -29,3 +29,20 @@ type Set[T Gossipable] interface { | |||
| // corresponding salt. | |||
| GetFilter() (bloom []byte, salt []byte) | |||
| } | |||
|
|
|||
| type AddManySet[T Gossipable] interface { | |||
There was a problem hiding this comment.
I know this would increase the scope of this change (especially because changing interfaces that are depended on by Coreth creates annoy circular dependencies)... But should we just be updating the Set interface to require this rather than adding this optional interface?
It seems like the X-chain and the P-chain could also try and avoid grabbing their locks multiple times.
There was a problem hiding this comment.
Sure, that was exactly my concern, trying to add this feature in a way without modifying the existing interface. But if it's OK, then I can just refactor Add to take a slice instead of a single element.
There was a problem hiding this comment.
I think we should probably push for just having one Add. Let me know if you have any questions on how to handle this weird circular import.
There was a problem hiding this comment.
+1. Having an interface that has both Add and the AddMany signature is going to be an awkward API IMO. I would recommend an function signature that uses a variadic parameter like so on the Set interface.
Add(gossipables ...Gossipable) []error
Allows potential optimized implementations for adding multiple elements to sets of Gossipables.
lebdron
changed the title
vms/platformvm/network/gossip.go
Outdated
| g.lock.Lock() | ||
| defer g.lock.Unlock() |
There was a problem hiding this comment.
I don't think this is going to work as expected if multiple transactions are provided. The defer will be executed on function return - not when the loop iterates.
There was a problem hiding this comment.
Whoops, overlooked that, thanks! Regarding X/P-Chain mempools, I'd rather put all the checks and tx verification inside the mempool not to acquire RLock for every lookup. Plus, GetDropReason acquires RW lock on mutex inside LRU cache, which would also be possible to rearrange if we do the verification inside Add. It's possible to pass txVerifier as a dependency to mempool I think.
| reset, errs[i] = gossip.ResetBloomFilterIfNeeded(g.bloom, g.Mempool.Len()*bloomChurnMultiplier) | ||
| if errs[i] != nil { | ||
| hasErr = true | ||
| continue |
There was a problem hiding this comment.
We should add tests for multiple txs - I think this can continue to the next iteration with the lock held.
Would it make sense to keep the single tx addition as a method so that we can keep using defer g.lock.Unlock()?
|
Do you happen to have any benchmarks/profile that indicate that this is giving us any measurable change in performance? I think unless there is some non-trivial benchmark that shows that this is an improvement we should close this because returning |
|
Closing for the above reason, feel free to re-open. |
Allows potential optimized implementations for adding multiple elements to sets of Gossipables.
Why this should be merged
Coreth currently has to pass every received transaction to the mempool in a separate Add call, requiring exclusive mutex acquire and release on every call.
https://github.com/ava-labs/coreth/blob/8d22112c7033658eb83b64e8b31d2f9c69676e28/plugin/evm/gossip.go#L192-L194
https://github.com/ava-labs/coreth/blob/8d22112c7033658eb83b64e8b31d2f9c69676e28/core/txpool/legacypool/legacypool.go#L1108-L1110
This interface will allow an optimized implementation to avoid individual Add calls for Coreth.
How this works
How this was tested