Wondering if it would make sense to make the pool global for the whole driver instead of per the logger instance?

Yes, a global pool for the whole driver is the most efficient way to use it.

An appropriate use of a Pool is to manage a group of temporary items silently shared among and potentially reused by concurrent independent clients of a package. Pool provides a way to amortize allocation overhead across many clients.

I changed the len(buf) to cap(buf) to stop considering buffers with smaller len, but enough capacity, as too small for the message.
I think the whole logic tries to reimplement what the bytes.Buffer does... I wonder if this should be rewritten to use bytes.Buffer instead. However it would be a bit hacky, because it would require to write directly to the byte slice returned by buffer.Bytes() and then return the buffer.Bytes()[:writeLen] to the caller.
Also I don't really like *[]byte in sync.Pool, but maybe it's fine?

I changed the len(buf) to cap(buf) to stop considering buffers with smaller len, but enough capacity, as too small for the message.

👍 Good catch!

I think the whole logic tries to reimplement what the bytes.Buffer does... I wonder if this should be rewritten to use bytes.Buffer instead. However it would be a bit hacky, because it would require to write directly to the byte slice returned by buffer.Bytes() and then return the buffer.Bytes()[:writeLen] to the caller.

I came to the same conclusion as well; trying to use a bytes.Buffer would be too hacky to be worth it.

Also I don't really like *[]byte in sync.Pool, but maybe it's fine?

I don't like it either, but it is the most performant way to use a Pool. As mentioned in the example in the godoc:

		// The Pool's New function should generally only return pointer
		// types, since a pointer can be put into the return interface
		// value without an allocation

A slice value is wider than a pointer so putting it directly into the pool would cost the very heap allocation we wanted to avoid. Getting and putting *[]byte values ensures that the heap allocation is only paid once, when the slice is first allocated, no matter how many times it is reused.

Same as in the jsonfilelog. Maybe the pool could be global?

I wanted to move the PutMessage up to be the responsibility of the caller. But a lot of tests rely on this being done inside Log, so left it to keep the changes small. Maybe could be done in a follow up?

🤔 I'm of two minds on this. One one hand, there are more logging drivers than consumers of the logger.Logger interface so pushing responsibility of putting the message back into the pool over to the consumer would be DRYer. On the other hand, the existing architecture has "move" semantics, which gives the log drivers great flexibility in handling incoming log messages as the message's lifetime does not need to be bounded by the Log call. Given that forgetting to put the message back into the pool would only result in a performance regression, not a memory leak (the struct will be GC'ed like normal) I think the flexibility afforded to the drivers by the move semantics is worth the cost of keeping the drivers responsible for returning the messages back to the pool when they're done.

Yes, a global pool for the whole driver is the most efficient way to use it.

An appropriate use of a Pool is to manage a group of temporary items silently shared among and potentially reused by concurrent independent clients of a package. Pool provides a way to amortize allocation overhead across many clients.

🤔 I'm of two minds on this. One one hand, there are more logging drivers than consumers of the logger.Logger interface so pushing responsibility of putting the message back into the pool over to the consumer would be DRYer. On the other hand, the existing architecture has "move" semantics, which gives the log drivers great flexibility in handling incoming log messages as the message's lifetime does not need to be bounded by the Log call. Given that forgetting to put the message back into the pool would only result in a performance regression, not a memory leak (the struct will be GC'ed like normal) I think the flexibility afforded to the drivers by the move semantics is worth the cost of keeping the drivers responsible for returning the messages back to the pool when they're done.

I changed the len(buf) to cap(buf) to stop considering buffers with smaller len, but enough capacity, as too small for the message.

👍 Good catch!

I think the whole logic tries to reimplement what the bytes.Buffer does... I wonder if this should be rewritten to use bytes.Buffer instead. However it would be a bit hacky, because it would require to write directly to the byte slice returned by buffer.Bytes() and then return the buffer.Bytes()[:writeLen] to the caller.

I came to the same conclusion as well; trying to use a bytes.Buffer would be too hacky to be worth it.

Also I don't really like *[]byte in sync.Pool, but maybe it's fine?

I don't like it either, but it is the most performant way to use a Pool. As mentioned in the example in the godoc:

		// The Pool's New function should generally only return pointer
		// types, since a pointer can be put into the return interface
		// value without an allocation

A slice value is wider than a pointer so putting it directly into the pool would cost the very heap allocation we wanted to avoid. Getting and putting *[]byte values ensures that the heap allocation is only paid once, when the slice is first allocated, no matter how many times it is reused.

If we need to pay the cost to reallocate the buffer, we should try to make it big enough to be reusable. Round up to the next power of two?
(Reusing slices with good performance is tricky to get right.)

Optimization opportunity: msg can be returned to the pool as soon as marshal returns, rather than deferring until after WriteLogEntry. Be mindful of that timestamp, though!

60 chars for the log message after overhead seems a touch small to me. It wouldn't fit a full 80-column line of text without resizing. How about sizing it enough for an SMS?