go: sync: Pool example suggests incorrect usage

The solution is of course to put only buffers with small byte slices back into the pool.

if b.Cap() <= 1<<12 {
         pool.put(b)
}

Alternatively, you could use an array of sync.Pools to bucketize the items by size: https://github.com/golang/go/blob/7e394a2/src/net/http/h2_bundle.go#L998-L1043

I guess a minimal example of such a usage would be:

package main

import (
	"sync"
	"net"
)

const MaxPacketSize = 4096

var bufPool = sync.Pool {
	New: func() interface{} {
		return make([]byte, MaxPacketSize)
	},
}

func process(outChan chan []byte) {
	for data := range outChan {
		// process data

		// Re-slice to maximum capacity and return it
		// for re-use. This is important to guarantee that
		// all calls to Get() will return a buffer of
		// length MaxPacketSize.
		bufPool.Put(data[:MaxPacketSize])
	}
}

func reader(conn net.PacketConn, outChan chan []byte) {
	for {
		data := bufPool.Get().([]byte)
		
		n, _, err := conn.ReadFrom(data)
		
		if err != nil {
			break
		}
		
		outChan <- data[:n]
	}
	
	close(outChan)
}

func main() {
	N := 3
	var wg sync.WaitGroup
	
	outChan := make(chan []byte, N)
	
	wg.Add(N)
	
	for i := 0; i < N; i++ {
		go func() {
			process(outChan)
			wg.Done()
		}()
	}
	
	wg.Add(1)

	conn, err := net.ListenPacket("udp", "localhost:10001")

	if err != nil {
		panic(err.Error())
	}
	
	go func() {
		reader(conn, outChan)
		wg.Done()
	}()
	
	wg.Wait()
}

but of course… whether this is going to be faster than the GC depends on how many packets per second you have to deal with and what exactly you do with the data etc. In real world you’d benchmark with GC/sync.Pool and compare the two. At the time we wrote our code there was a significant time spent allocating new stuff and using a scheme as above we’ve managed to increase the throughput. Of course, one should re-benchmark this with every update to the GC.

Maybe the solution is not to recommend a sync.Pool at all anymore? This is my understanding from a comment I read about how GC makes this more or less useless

We would certainly like to get to this point, and the GC has improved a lot, but for high-churn allocations with obvious lifetimes and no need for zeroing, sync.Pool can still be a significant optimization. As @RLH likes to say, every use of sync.Pool is a bug report on the GC. But we’re still taking those bug reports. 😃

I should also note that if #22950 is done, then usages like this will cause large buffers to be pinned forever since this example has a steady state of Pool usage, so the GC would never clear the pool.

That’s clearly true, but even right now it’s partly by chance that these examples are eventually dropping the large buffers. And in the more realistic stochastic mix example, it’s not clear to me that #22950 would make it any better or worse.

I agree with @dsnet’s original point that we should document that sync.Pool treats all objects interchangeably, so they should all have roughly the same “weight”. And it would be good to provide some suggestions for what to do in situations where this isn’t the case, and perhaps some concrete examples of poor sync.Pool usage.

@storozhukBM

But if you actually have more or less frequent necessities in big buffers, the pool will still work but will be a bit less efficient.

This is unfortunately the problem with a probabilistic model. I found it to be much less efficient for applications that did continually use a large buffer size. Ideally, we want perfect reuse if an application consistently uses a large buffer size.

I believe that returning a variable-length buffer to the pool needs to consider history of how well the buffer was utilized. With regard to history, you can either go with global history or local history. Global history is more accurate, but then you have to deal with coordination, or you can go with local history, and hope that its good enough. Did you see my code snippet in https://github.com/golang/go/issues/27735#issuecomment-739169121? I only relies on local history and addresses the problem of discarding large buffers when they are underutilized, while ensuring perfect reuse when consistently use large buffers.

We’ve used sync.Pool for dealing with network packets and others do too (such as lucas-clemente/quic-go) because for those use cases you gain performance when using them. However, in those cases []byte is used instead of bytes.Buffer and they all have the same capacity and are re-sliced as needed. Before putting them back they are re-sliced to MaxPacketSize and before reading you Get a new []byte and re-slice it to the size of the network packet.

The same we did even for structs such as when parsing packets to a struct func ParsePacket([]byte data) *Packet (which overwrites all fields in a struct anyway) which means instead of allocating/freeing thousands of new structs each second they can be re-used using sync.Pool which makes things a little bit faster.

Would changing the example to use an array (fixed size) rather than a slice solve this problem? In Chihaya, this is how we’ve used sync.Pool and our implementation before it was in the standard library.

Maybe the solution is not to recommend a sync.Pool at all anymore?

I legitimately don’t think there ever was a time to generally recommend sync.Pool. I find it a pretty contentious add to the standard library because of how careful and knowledgable of the runtime you need to be in order to use it effectively. If you need optimization at this level, you probably know how to implement this best for your own use case.

Sorry to interject randomly, but I saw this thread on Twitter and have strong opinions on this feature.

So I believe, capping the length or capacity of a slice does not actually reduce the size of the underlying backing array. So, this would not actually save or restore any more memory than stuffing the whole buffer anyways. The only way to truly shrink the memory usage of a slice would be to reallocate it at a smaller size, and then copy the data into… but at that point, in this particular use case, that means we would be better off just dropping the whole slice on the ground, and letting the GC clean it up.

@peterbourgon, wouldn’t using bytes.Buffer still have the same issue unless the user explicitly enforces some maximum bounds on the buffer length? If so, presumably the same technique that someone could do with a raw []byte?

I’ve been trying to find a way to use variable-length buffers with sync.Pool, and the approach that I’ve been taking is something like: https://github.com/golang/go/issues/27735#issuecomment-739169121

Do you have any suggestions for better use cases we could include in the example, that are reasonably compact?

Maybe the solution is not to recommend a sync.Pool at all anymore? This is my understanding from a comment I read about how GC makes this more or less useless