Skip to content

Commit

Permalink
Optimize memory usage.
Browse files Browse the repository at this point in the history
If aggregating results from thousands of GetNext() calls in memory, the
runtime seems unable to release some of the memory allocated by send() (or
dispatch() now).

The situation can be improved by copying the resliced response slice in
dispatch(), rather than having the runtime keep a reference to a larger
(and partially usefull) buffer array.

This is the bug I was trying to squash in b84d786.

The logic of adjusting the buffer size dynamically seems to be still
valid though.

With this change, the actual buffer size allocated to handle the
response is no longer a problem so I changed the rxBufSizeMin and
rxBufSizeMax to more aggressive/generous defaults (and min buffer size
is proportional to request size).

This will probably make buffer resizing an exceptional event.
  • Loading branch information
benjamin-thomas authored and soniah committed Jan 5, 2015
1 parent 91b9db9 commit 3e6fc99
Showing 1 changed file with 16 additions and 6 deletions.
22 changes: 16 additions & 6 deletions marshal.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,8 @@ const (
)

const (
rxBufSizeMin = 256
rxBufSizeMax = 65536
rxBufSizeMin = 1024 // Minimal buffer size to handle 1 OID (see dispatch())
rxBufSizeMax = 131072 // Prevent memory allocation from going out of control
)

// Logger is an interface used for debugging. Both Print and
Expand Down Expand Up @@ -141,7 +141,7 @@ func (x *GoSNMP) send(pdus []SnmpPDU, packetOut *SnmpPacket) (result *SnmpPacket
}

var resp []byte
resp, err := dispatch(x.Conn, outBuf)
resp, err := dispatch(x.Conn, outBuf, len(pdus))
if err != nil {
return result, err
}
Expand Down Expand Up @@ -558,9 +558,9 @@ func unmarshalVBL(packet []byte, response *SnmpPacket,
// Previously, resp was allocated rxBufSize (65536) bytes ie a fixed size for
// all responses. To decrease memory usage, resp is dynamically sized, at the
// cost of possible additional network round trips.
func dispatch(c net.Conn, outBuf []byte) ([]byte, error) {
func dispatch(c net.Conn, outBuf []byte, pduCount int) ([]byte, error) {
var resp []byte
for bufSize := rxBufSizeMin; bufSize < rxBufSizeMax; bufSize *= 2 {
for bufSize := rxBufSizeMin * pduCount; bufSize < rxBufSizeMax; bufSize *= 2 {
resp = make([]byte, bufSize)
_, err := c.Write(outBuf)
if err != nil {
Expand All @@ -572,7 +572,17 @@ func dispatch(c net.Conn, outBuf []byte) ([]byte, error) {
}

if n < bufSize {
return resp[:n], nil
// Memory usage optimization. Help the runtime to release as much memory as possible.
//
// See: http://blog.golang.org/go-slices-usage-and-internals, section: A possible "gotcha"
// ...As mentioned earlier, re-slicing a slice doesn't make a copy of the
// underlying array. The full array will be kept in memory until it is no
// longer referenced. Occasionally this can cause the program to hold all
// the data in memory when only a small piece of it is needed.
resp = resp[:n]
resp2 := make([]byte, len(resp))
copy(resp2, resp)
return resp2, nil
}
}
return resp, fmt.Errorf("Response bufSize exceeded rxBufSizeMax (%d)", rxBufSizeMax)
Expand Down

3 comments on commit 3e6fc99

@codedance
Copy link
Contributor

Choose a reason for hiding this comment

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

Fantastic++ Great work @benjamin-thomas . This change looks good. I remember putting the FIXME comment above the buffer make & read a while back. I wanted to fix it at the time but did not have the bandwidth. My thinking was that we may need to drop to a lower packet read level and check MSG_TRUNC and/or work out exactly how go was wrapping the lower-level recv calls. Your n+1 of course will work well. Also auto scaling the initial buffer size based on the oid count is clever.

I'll do a bit of testing. I think we may meed to be a little more aggressive with the initial buffer size. e.g. fine for numeric values, but may commonly overflow with typical OctetStrings causing re-requests. For reference, both netsnmp and snmp4j allocate ~64k buffers upfront by default.

The slice copy is clever.

I'll open a issue/ticket on the thinking around the initial buffer size.

Great work @benjamin-thomas and @soniah

@benjamin-thomas
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the kind words :)

You gotta love pprof! :D

@codedance
Copy link
Contributor

Choose a reason for hiding this comment

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

Yep. If VizGraph is working :-)

Please sign in to comment.