Replace recursive approach with iterative approach for readPacket() #170
Conversation
| if err != nil { | ||
| errLog.Print(err.Error()) | ||
| mc.Close() | ||
| return nil, driver.ErrBadConn | ||
| } | ||
|
|
||
| // Make a copy since data becomes invalid with the next read | ||
| buf := make([]byte, len(data)) | ||
| copy(buf, data) |
There was a problem hiding this comment.
we don't need the copy without the recursion - data stays valid and appending copies it anyway
There was a problem hiding this comment.
good point! just added a commit to remove that.
|
This makes the common case (non splitted packets) a lot more expensive and destroys our current "zero allocations" approach. |
| data, err = mc.readPacket() | ||
| if err == nil { | ||
| return append(buf, data...), nil | ||
| if pktLen < maxPacketSize { |
There was a problem hiding this comment.
@julienschmidt is you critique still valid if the append above is removed and this if becomes
if pktLen >= maxPacketSize {
payload = append(payload, data...)
continue
}
return data, nilThere was a problem hiding this comment.
and (handwaving) the correct variable is returned every time... (not data if it should be payload)
There was a problem hiding this comment.
@julienschmidt I understood what you mean.
It's doing payload = append(nil, data) for common cases. (I thought golang will do payload=data for append(nil...) but obviously not.)
We can still disable the buffer copying for common case by doing what @arnehormann suggested.
|
@julienschmidt the logic after the new patch, for the common cases (non-splitting), will not invoke any allocation (like old approach); for large packets, avoids recursive calls and copying twice. |
| // Read packet body [pktLen bytes] | ||
| if data, err = mc.buf.readNext(pktLen); err == nil { | ||
| if pktLen < maxPacketSize { | ||
| // Zero-allocation for non-splitting packets |
There was a problem hiding this comment.
Maybe // zero allocations for non-split packets ?
I'm not a native speaker, but to me Zero-allocation sound like allocating a zero
|
The new approach looks fine to me 👍 |
|
thanks @julienschmidt, amended suggestions to previous commits. |
|
LGTM now, final review by @arnehormann |
| return nil, driver.ErrBadConn | ||
| } | ||
|
|
||
| isLastPacket := (pktLen < maxPacketSize) |
There was a problem hiding this comment.
what happens when the packet length of the last packet is exactly maxPacketSize? Is that possible?
I'll look it up now, but if you know, please chime in...
There was a problem hiding this comment.
what happens when the packet length of the last packet is exactly maxPacketSize? Is that possible?
No, a packet length of maxPacketSize marks a split packet
|
LGTM before and still does... |
Replace recursive approach with iterative approach for readPacket()
Possible benefits: