Reduce DataChannel.readLoop allocations

[bshimc]

Summary

I'm evaluating webrtc for a use case that stresses DataChannels. In profiling, I tracked down some pathological alloc behavior to readLoop. A simple patch that uses sync.Pool to minimize allocs (see patch below) resulted in what appear to be significant GC improvements. That said, the patch in question changes the semantics of the datachannel OnMessage handler; that's to say the contents of the message buffer are only valid inside the lifetime of the handler function. For my use case this was acceptable but that's unlikely to be the case in general (great if it is).

Motivation

Performance.

Describe alternatives you've considered

To guarantee compatibility we'd need to allocate a new buffer sized to n and copy the contents to be passed on before releasing the larger dataChannelBufferSize buffers. I haven't tried this. For our purposes it would be nice to have a configuration that could toggle between no-copy and legacy-safe behaviors.

Additional context

diff --git a/vendor/github.com/pion/webrtc/v3/datachannel.go b/vendor/github.com/pion/webrtc/v3/datachannel.go
index ba1269d..7a32c70 100644
--- a/vendor/github.com/pion/webrtc/v3/datachannel.go
+++ b/vendor/github.com/pion/webrtc/v3/datachannel.go
@@ -311,11 +311,16 @@ func (d *DataChannel) onError(err error) {
        }
 }

+var rlBufPool = sync.Pool{New: func() interface{} {
+       return make([]byte, dataChannelBufferSize)
+}}
+
 func (d *DataChannel) readLoop() {
        for {
-               buffer := make([]byte, dataChannelBufferSize)
+               buffer := rlBufPool.Get().([]byte)
                n, isString, err := d.dataChannel.ReadDataChannel(buffer)
                if err != nil {
+                       rlBufPool.Put(buffer)
                        d.setReadyState(DataChannelStateClosed)
                        if err != io.EOF {
                                d.onError(err)
@@ -325,6 +330,7 @@ func (d *DataChannel) readLoop() {
                }

                d.onMessage(DataChannelMessage{Data: buffer[:n], IsString: isString})
+               rlBufPool.Put(buffer)
        }
 }

Top (before) bottom (after) memory allocation profiles. The second set of profiles actually is generating a lot more QPS through the established datachannels.

[Sean-Der]

This is amazing! Really love to see stuff like this.

To get this merged today I think we can make it a SettingEngine option. This is a pretty scary breaking change, users will upgrade and not understand what hit them :(

We can get this merged quickly (with a test) and then revisit making it a default later.

[Sean-Der]

Fixed by 159ba5a