Skip to content

zero-copy buffer #55

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.

Sign up for GitHub

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 6 commits into from
Apr 24, 2013
Merged

zero-copy buffer #55

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
96a4f13
return byte slice directly from buffer
julienschmidt Apr 21, 2013
c2dde2d
optimized buffer refill
julienschmidt Apr 21, 2013
de021f9
getSystemVar result validity doc
julienschmidt Apr 21, 2013
a602069
Grow buffer instead of using a tmp slice
julienschmidt Apr 24, 2013
7ac8bc9
grow buffer doc + fmt
julienschmidt Apr 24, 2013
f3fa1f4
grow buffer doc clarification
julienschmidt Apr 24, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 40 additions & 24 deletions buffer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,14 @@ func newBuffer(rd io.Reader) *buffer {
}
}

// fill reads at least _need_ bytes in the buffer
// existing data in the buffer gets lost
// fill reads into the buffer until at least _need_ bytes are in it
func (b *buffer) fill(need int) (err error) {
// move existing data to the beginning
if b.length > 0 && b.idx > 0 {
copy(b.buf[0:b.length], b.buf[b.idx:])
}

b.idx = 0
b.length = 0

var n int
for b.length < need {
Expand All @@ -51,35 +54,48 @@ func (b *buffer) fill(need int) (err error) {
return
}

// read len(p) bytes
func (b *buffer) read(p []byte) (err error) {
need := len(p)
// returns next N bytes from buffer.
// The returned slice is only guaranteed to be valid until the next read
func (b *buffer) readNext(need int) (p []byte, err error) {
// return slice from buffer if possible
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replace function body with:

    if len(b.buf) < need {
        // expand buffer if neccessary
        b.buf = append(b.buf, make([]byte, need - len(b.buf))...)
    }
    if need < b.length {
        // fill buffer with data
        if err = b.fill(need); err != nil {
            return
        }
    }
    p = b.buf[b.idx:b.idx + need]
    b.idx += need
    b.length -= need
    return

I don't know if the append-code is idiomatic, but it's the best I can offer. Go's compiler could optimize it and get rid of the non-required allocation. I'm not sure about the performance as is, though. Maybe a make and copy is faster, maybe it'll even stay faster with later go compilers.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I asked about growing a slice on go-nuts. Hopefully someone knows the best way to do it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taking the results from go-nuts into consideration (esp. Maxim Khitrov's performance comparisons) I'd opt for moving append into fill and use its capacity doubling behavior.
Get rid of the first if in my proposal and handle resizing by append in fill.
It should be safe as long as nobody low on RAM stores DVD-ISOs in MySQL.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even in this case the buffer grows just to maxPacketSize bytes (16 MiB)

if b.length >= need {
p = b.buf[b.idx : b.idx+need]
b.idx += need
b.length -= need
return

if b.length < need {
if b.length > 0 {
copy(p[0:b.length], b.buf[b.idx:])
need -= b.length
p = p[b.length:]
} else {

b.idx = 0
b.length = 0
}
// does the data fit into the buffer?
if need < len(b.buf) {
// refill
err = b.fill(need) // err deferred
p = b.buf[:need]
b.idx += need
b.length -= need
return

if need >= len(b.buf) {
var n int
} else {
p = make([]byte, need)
has := 0
for err == nil && need > has {

// copy data that is already in the buffer
if b.length > 0 {
copy(p[0:b.length], b.buf[b.idx:])
has = b.length
need -= has
b.idx = 0
b.length = 0
}

// read rest directly into the new slice
var n int
for err == nil && need > 0 {
n, err = b.rd.Read(p[has:])
has += n
need -= n
}
return
}

err = b.fill(need) // err deferred
}

copy(p, b.buf[b.idx:])
b.idx += need
b.length -= need
return
}
1 change: 1 addition & 0 deletions connection.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,7 @@ func (mc *mysqlConn) Query(query string, args []driver.Value) (driver.Rows, erro
}

// Gets the value of the given MySQL System Variable
// The returned byte slice is only valid until the next read
func (mc *mysqlConn) getSystemVar(name string) (val []byte, err error) {
// Send command
err = mc.writeCommandPacketStr(comQuery, "SELECT @@"+name)
Expand Down
8 changes: 3 additions & 5 deletions packets.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,14 @@ import (
// Read packet to buffer 'data'
func (mc *mysqlConn) readPacket() (data []byte, err error) {
// Read packet header
data = make([]byte, 4)
err = mc.buf.read(data)
data, err = mc.buf.readNext(4)
if err != nil {
errLog.Print(err.Error())
return nil, driver.ErrBadConn
}

// Packet Length [24 bit]
pktLen := uint32(data[0]) | uint32(data[1])<<8 | uint32(data[2])<<16
pktLen := int(uint32(data[0]) | uint32(data[1])<<8 | uint32(data[2])<<16)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

leave as is, cast to int in L54 readNext?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would be the benefit of that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

honestly - none. Just one change less (and I dislike parens around long expressions)

if pktLen < 1 {
errLog.Print(errMalformPkt.Error())
Expand All @@ -52,8 +51,7 @@ func (mc *mysqlConn) readPacket() (data []byte, err error) {
mc.sequence++

// Read packet body [pktLen bytes]
data = make([]byte, pktLen)
err = mc.buf.read(data)
data, err = mc.buf.readNext(pktLen)
if err == nil {
if pktLen < maxPacketSize {
return data, nil
Expand Down