perf(p2p): Change channel.recentlySent to be based on num_messages #2957
Conversation
ValarDragon
changed the title
ValarDragon
changed the title
melekes
added
backport-to-v0.37.x
|
We'll need to test this somehow through our e2e framework or local net to avoid relying on pure reasoning. |
|
Agreed, I also think that in practice this may also need #2955 |
There was a problem hiding this comment.
The problem is not solved by this PR.
The original code compute priorities based on the number of bytes sent in each channel.
The proposed changes update the computation to be based on the number of packets sent in each channel. While it is true that not every packet is full (i.e., contains MaxPacketMsgPayloadSize, default to 1024 bytes), when full packets are exchanged, there is not difference in behavior.
In any case, messages are split into multiple packets to be sent in the channel. If a message is 10x larger than the other, it would be split into about 10x more packets than the other, and the proposed changes won't have any real effect.
| @@ -0,0 +1,3 @@ | |||
| - `[p2p]` Improve the prioritization of which channel to gossip from, by changing channel deprioritization | |||
There was a problem hiding this comment.
| - `[p2p]` Improve the prioritization of which channel to gossip from, by changing channel deprioritization | |
| - `[p2p]` Change the prioritization of which channel to send packets from, by having de-prioritization |
| @@ -0,0 +1,3 @@ | |||
| - `[p2p]` Improve the prioritization of which channel to gossip from, by changing channel deprioritization | |||
| to be based on the number of messages in the channel, rather than the size of messages communicated on the channel. | |||
There was a problem hiding this comment.
| to be based on the number of messages in the channel, rather than the size of messages communicated on the channel. | |
| to be based on the number of messages rather than the byte size of packets recently sent on the channel. |
| @@ -755,7 +757,10 @@ type Channel struct { | |||
| sendQueueSize int32 // atomic. | |||
| recving []byte | |||
| sending []byte | |||
| recentlySent int64 // exponential moving average | |||
| // recentlySent is an exponential moving average of the number of messages sent. | |||
| // This is used by the mconnection for choosing what channel to send from next. | |||
There was a problem hiding this comment.
| // This is used by the mconnection for choosing what channel to send from next. | |
| // This is used for choosing from which channel to send the next packet. |
| recentlySent int64 // exponential moving average | ||
| // recentlySent is an exponential moving average of the number of messages sent. | ||
| // This is used by the mconnection for choosing what channel to send from next. | ||
| // The exponential decay is done in updateStats. |
There was a problem hiding this comment.
| // The exponential decay is done in updateStats. | |
| // The exponential decay is applied every updateStats inverval. |
| @@ -856,7 +861,8 @@ func (ch *Channel) writePacketMsgTo(w io.Writer) (n int, err error) { | |||
| err = ErrPacketWrite{Source: err} | |||
| } | |||
|
|
|||
| atomic.AddInt64(&ch.recentlySent, int64(n)) | |||
| // increment recentlySent by 1, to denote we sent another message. | |||
There was a problem hiding this comment.
But this is not a message, but a packet. A message is composed of multiple packets.
| @@ -31,7 +31,9 @@ const ( | |||
| numBatchPacketMsgs = 10 | |||
| minReadBufferSize = 1024 | |||
| minWriteBufferSize = 65536 | |||
| updateStats = 2 * time.Second | |||
|
|
|||
| updateStats = 2 * time.Second | |||
There was a problem hiding this comment.
| updateStats = 2 * time.Second | |
| recentlySentDecayInterval = 2 * time.Second |
There was a problem hiding this comment.
Need to rename elsewhere in the code.
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Closes #2954
This should make block part gossip not be bottom priority. (though more work is needed to make it match the prioritization expectation)
PR checklist
.changelog(we use unclog to manage our changelog)docs/orspec/) and code comments