moving UDP payload size to config option, improve throughput performance

[sebito91]

Current UDP service alloc's a byte slice at max theoretical UDP packet size for each iteration of reading from the socket. This is inefficient in terms of memory allocation (ReadFromUDP pulls one packet at a time, so it's largely overkill for 95% of use cases) and execution time (inordinate about of time spent in alloc, causes backpressure on UDP socket at scale + extra overhead for GC).

Proposed changes add UDP payload size as a config option (default is UDP max) and help speed up throughput when closely aligned with payload size sent from metrics source (e.g. udp_payload in telegraf).

To test/prove this, you can use this tester script (thanks @daviesalex) with different slice sizes. Testing with max UDP at 65536, alloc times hover around 10-20us but drop to ~120-130ns when size dropped to match typical udp_payload at 1500.

package main

import (
        "fmt"
        "time"

func main() {
        for { 
                start := time.Now()

                foo := make([]byte, 1500)

                now := time.Now()
                diff := now.Sub(start)
                fmt.Printf("ndiff: %v (%d)\n", diff, len(foo))
        } 
}

[otoolep]

Thanks guys.

@sebito91 -- have you signed the CLA?

[otoolep]

Nit: golang -> Go. We never call it golang. :-)

[sebito91]

Heh, I'll remove 'golang'...and yes already signed the CLA.

[otoolep]

This makes a lot of sense to me -- thanks @sebito91 and @daviesalex

+1 on green.

@jwilder @sparrc -- can I get a second review?

[otoolep]

Also, our convention is that we set the value of commented out flags to the default. However, a note here explaining that a lower value would be better for most would make sense.

Sound good?

[sebito91]

Sounds good, will update now.

[jwilder]

👍

[jsternberg]

Can you rename this constant to MaxUDPBufferSize and modify the service's Open function to return an error when the payload size is set too high in the config?

It would be nice to either error or give some kind of warning to the user if they set the buffer size unnecessarily high.

[sebito91]

This diff actually removes that const in lieu of a config var. See original commit:

sebito91@3bf5d9c

[jsternberg]

Yes. I know. I thought it might be valuable to keep this constant and use it to ensure that the payload size doesn't get set unnecessarily high by a user.

It's a usability concern, not one of correctness.

[sebito91]

There's nothing wrong with allocating a buffer size > 65536, the only reason that value was selected was to provide guidance on an upper bound. Nothing stopping you from setting that higher if you really want (in terms of usability).

[jsternberg]

Never mind then.

[sparrc]

Thanks much @sebito91