eth, p2p: send txs and new blocks to peers in parallel #147
Conversation
|
LGTM, @benbjohnson could you please review too? |
|
I'll merge for now, but @benbjohnson would still be good to get your review. |
| @@ -201,37 +203,72 @@ func (p *peer) SendTransactions(txs types.Transactions) error { | |||
| return err | |||
| } | |||
|
|
|||
| // parallelPeers coordinates efficient parallel sends. | |||
| type parallelPeers []*peer | |||
There was a problem hiding this comment.
I don't think adding a new type adds much value. It seems easier and clearer just to add new functions.
e.g.
SendTransactionParallel(peers []*peer, tx *types.Transaction) error| }(p) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Firing off goroutines without managing their lifecycle makes me nervous but it's done quite a lot in the go-ethereum code base. That seems like a bigger discussion though and outside the scope of this PR.
There was a problem hiding this comment.
Agreed. Though if you dig in to (p *MsgPipeRW) WriteMsg(), it's not so bad. These will block ...until the receiver has consumed the message payload., and will cancel out and exit gracefully if the peer connection is closed.
It would be better if we kept track of them - we can always emit some kind of metrics.
Broadcast Tx, send new block, and send new block hash were all executing sends to multiple peers in serial and re-encoding identical message payloads each time. Now all three spawn goroutines so we 1) send in parallel, 2) share a single encoded payload, and 3) don't block the caller. With 1 &2 we're both doing less work (and allocating less memory), and doing it concurrently, which should be much faster in general, and prevent slow peers from affecting fast peers. With 3, we free up the caller to return almost immediately, which could otherwise e.g. clog up the rest of the event subscriber system.
Related to #138