-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Use the Connection Buffer for Writing #134
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
ddf24e6
use the connection buffer for writing
julienschmidt 656614d
writeExecutePacket: fix capacity check
julienschmidt 3d95bd0
writeExecutePacket: fix packing
julienschmidt 33d6df2
various refactoring
julienschmidt 5975ca9
more refactoring
julienschmidt 8751b72
buffer: rename take buffer funcs
julienschmidt 605647e
changelog: Add write buffer
julienschmidt d8e6c38
writeCommandPacketUint32: fix packet length comment
julienschmidt 228ba34
packets: YAR (yet another refactoring)
julienschmidt File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,7 +12,10 @@ import "io" | |
|
||
const defaultBufSize = 4096 | ||
|
||
// A read buffer similar to bufio.Reader but zero-copy-ish | ||
// A buffer which is used for both reading and writing. | ||
// This is possible since communication on each connection is synchronous. | ||
// In other words, we can't write and read simultaneously on the same connection. | ||
// The buffer is similar to bufio.Reader / Writer but zero-copy-ish | ||
// Also highly optimized for this particular use case. | ||
type buffer struct { | ||
buf []byte | ||
|
@@ -37,8 +40,11 @@ func (b *buffer) fill(need int) (err error) { | |
} | ||
|
||
// grow buffer if necessary | ||
// TODO: let the buffer shrink again at some point | ||
// Maybe keep the org buf slice and swap back? | ||
if need > len(b.buf) { | ||
newBuf := make([]byte, need) | ||
// Round up to the next multiple of the default size | ||
newBuf := make([]byte, ((need/defaultBufSize)+1)*defaultBufSize) | ||
copy(newBuf, b.buf) | ||
b.buf = newBuf | ||
} | ||
|
@@ -74,3 +80,44 @@ func (b *buffer) readNext(need int) (p []byte, err error) { | |
b.length -= need | ||
return | ||
} | ||
|
||
// returns a buffer with the requested size. | ||
// If possible, a slice from the existing buffer is returned. | ||
// Otherwise a bigger buffer is made. | ||
// Only one buffer (total) can be used at a time. | ||
func (b *buffer) takeBuffer(length int) []byte { | ||
if b.length > 0 { | ||
return nil | ||
} | ||
|
||
// test (cheap) general case first | ||
if length <= defaultBufSize || length <= cap(b.buf) { | ||
return b.buf[:length] | ||
} | ||
|
||
if length < maxPacketSize { | ||
b.buf = make([]byte, length) | ||
return b.buf | ||
} | ||
return make([]byte, length) | ||
} | ||
|
||
// shortcut which can be used if the requested buffer is guaranteed to be | ||
// smaller than defaultBufSize | ||
// Only one buffer (total) can be used at a time. | ||
func (b *buffer) takeSmallBuffer(length int) []byte { | ||
if b.length == 0 { | ||
return b.buf[:length] | ||
} | ||
return nil | ||
} | ||
|
||
// takeCompleteBuffer returns the complete existing buffer. | ||
// This can be used if the necessary buffer size is unknown. | ||
// Only one buffer (total) can be used at a time. | ||
func (b *buffer) takeCompleteBuffer() []byte { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as for takeSmallBuffer, but this one only has one caller (in |
||
if b.length == 0 { | ||
return b.buf | ||
} | ||
return nil | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everywhere this is used in packets.go, it's immediately followed by
if data == nil
based error handling, so even if it's inlined by the compiler, it always branches twice.b.length != 0
is the only way this can return nil, so I say manually inline all occurrences (and add a comment), use the buffer directly and drop the superflouusif
for all 4 occurences.That's bad from a code style perspective, but the comment should suffice. It will probably be a little faster and not more complex than it is now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Branches are not generally evil. In this case 1 branch miss is to be expected. I think keeping the buffer logic separate is worth it.