ReadMsgHeader use bytes pool #1386
Conversation
| // use sync.Pool to avoid gc overhead. | ||
| var udpPool sync.Pool | ||
|
|
||
| func init() { |
| @@ -260,6 +261,13 @@ func (c *Client) exchangeContext(ctx context.Context, m *Msg, co *Conn) (r *Msg, | |||
| return r, rtt, err | |||
| } | |||
|
|
|||
| // use sync.Pool to avoid gc overhead. | |||
| var udpPool sync.Pool | ||
|
|
||
| func init() { | ||
| udpPool.New = makeUDPBuffer(DefaultMsgSize) |
There was a problem hiding this comment.
DefaultMsgSize is 512, a header is max 12 bytes, is this intentional?
|
your commit message also don't explain anything about this change, it's just a one-liner So it's on the client you want to use a buffer just like we do on the server side? Any data available (that should be in the ocmmit msg) on why and how this came about?? |
| return nil, err | ||
| } | ||
|
|
||
| p = make([]byte, length) | ||
| n, err = io.ReadFull(co.Conn, p) |
There was a problem hiding this comment.
This is wrong. You're ignoring the length which can be up to 64KiB. This would limit the max message size under TCP to whatever the pool returns.
| p = make([]byte, co.UDPSize) | ||
| } else { | ||
| p = make([]byte, MinMsgSize) | ||
| } |
There was a problem hiding this comment.
This code was important. UDPSize can be set larger than MinMsgSize. You're not handling that at all.
| return nil, ErrShortRead | ||
| } | ||
|
|
||
| p = p[:n] |
There was a problem hiding this comment.
This slice call is important, we only read n-bytes not the full length of p.
|
I'm sympathetic to the performance concerns here, but this isn't quite the way to go about it. We'd need a pool on the Client struct itself. But considering there's been no movement here in almost a year, I'm going to go ahead and close this PR. Feel free to re-open and address the issues above. |
I find the makeslice of ReadMsgHeader will cost much GC time. So I suggest to use bytes pool to avoid the GC cost.