slog: Support inline group fields per spec

buffer/pool.go

@@ -20,25 +20,29 @@
 
 package buffer
 
-import "sync"
+import (
+	"go.uber.org/zap/internal/pool"
+)
 
 // A Pool is a type-safe wrapper around a sync.Pool.
 type Pool struct {
-	p *sync.Pool
+	p *pool.Pool[*Buffer]
 }
 
 // NewPool constructs a new Pool.
 func NewPool() Pool {
-	return Pool{p: &sync.Pool{
-		New: func() interface{} {
-			return &Buffer{bs: make([]byte, 0, _size)}
-		},
-	}}
+	return Pool{
+		p: pool.New(func() *Buffer {
+			return &Buffer{
+				bs: make([]byte, 0, _size),
+			}
+		}),
+	}
 }
 
 // Get retrieves a Buffer from the pool, creating one if necessary.
 func (p Pool) Get() *Buffer {
-	buf := p.p.Get().(*Buffer)
+	buf := p.p.Get()
 	buf.Reset()
 	buf.pool = p
 	return buf

error.go

@@ -21,14 +21,13 @@
 package zap
 
 import (
-	"sync"
-
+	"go.uber.org/zap/internal/pool"
 	"go.uber.org/zap/zapcore"
 )
 
-var _errArrayElemPool = sync.Pool{New: func() interface{} {
+var _errArrayElemPool = pool.New(func() *errArrayElem {
 	return &errArrayElem{}
-}}
+})
 
 // Error is shorthand for the common idiom NamedError("error", err).
 func Error(err error) Field {
@@ -60,7 +59,7 @@ func (errs errArray) MarshalLogArray(arr zapcore.ArrayEncoder) error {
 		// potentially an "errorVerbose" attribute, we need to wrap it in a
 		// type that implements LogObjectMarshaler. To prevent this from
 		// allocating, pool the wrapper type.
-		elem := _errArrayElemPool.Get().(*errArrayElem)
+		elem := _errArrayElemPool.Get()
 		elem.error = errs[i]
 		arr.AppendObject(elem)
 		elem.error = nil

exp/go.mod

@@ -4,7 +4,7 @@ go 1.19
 
 require (
 	github.com/stretchr/testify v1.8.1
-	go.uber.org/zap v1.24.0
+	go.uber.org/zap v1.24.1-0.20230320162319-cb7832fc5727
 	golang.org/x/exp v0.0.0-20230315142452-642cacee5cc0
 )
 

exp/go.sum

@@ -19,6 +19,8 @@ go.uber.org/multierr v1.10.0 h1:S0h4aNzvfcFsC3dRF1jLoaov7oRaKqRGC/pUEJ2yvPQ=
 go.uber.org/multierr v1.10.0/go.mod h1:20+QtiLqy0Nd6FdQB9TLXag12DsQkrbs3htMFfDN80Y=
 go.uber.org/zap v1.24.0 h1:FiJd5l1UOLj0wCgbSE0rwwXHzEdAZS6hiiSnxJN/D60=
 go.uber.org/zap v1.24.0/go.mod h1:2kMP+WWQ8aoFoedH3T2sq6iJ2yDWpHbP0f6MQbS9Gkg=
+go.uber.org/zap v1.24.1-0.20230320162319-cb7832fc5727 h1:dK3Xm+OZ6BjJzzEiNWzaLfx4bjOEfOXR6gd0MdiCwtg=
+go.uber.org/zap v1.24.1-0.20230320162319-cb7832fc5727/go.mod h1:JIAUzQIH94IC4fOJQm7gMmBJP5k7wQfdcnYdPoEXJYk=
 golang.org/x/exp v0.0.0-20230315142452-642cacee5cc0 h1:pVgRXcIictcr+lBQIFeiwuwtDIs4eL21OuM9nyAADmo=
 golang.org/x/exp v0.0.0-20230315142452-642cacee5cc0/go.mod h1:CxIveKay+FTh1D0yPZemJVgC/95VzuuOLq5Qi4xnoYc=
 gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM=

exp/zapslog/slog.go

@@ -25,10 +25,46 @@ import (
 	"runtime"
 
 	"go.uber.org/zap"
+	"go.uber.org/zap/internal/pool"
 	"go.uber.org/zap/zapcore"
 	"golang.org/x/exp/slog"
 )
 
+type fieldSlice []zapcore.Field
+
+func (s *fieldSlice) Append(f zapcore.Field) {
+	*s = append(*s, f)
+}
+
+func (s *fieldSlice) AddTo(enc zapcore.ObjectEncoder) {
+	for i := 0; i < s.Len(); i++ {
+		(*s)[i].AddTo(enc)
+	}
+}
+
+func (s *fieldSlice) Extend(f *fieldSlice) {
+	*s = append(*s, *f...)
+}
+
+func (s *fieldSlice) Len() int {
+	return len(*s)
+}
+
+func (s *fieldSlice) Reset() {
+	*s = (*s)[:0]
+}
+
+func (s *fieldSlice) Return() {
+	s.Reset()
+	_fieldSlices.Put(s)
+}
+
+var _fieldSlices = pool.New(func() *fieldSlice {
+	// n.b. The majority of cases will only have one field.
+	x := make(fieldSlice, 0, 1)
+	return &x
+})
+
 // Handler implements the slog.Handler by writing to a zap Core.
 type Handler struct {
 	core      zapcore.Core
@@ -74,43 +110,62 @@ type groupObject []slog.Attr
 
 func (gs groupObject) MarshalLogObject(enc zapcore.ObjectEncoder) error {
 	for _, attr := range gs {
-		convertAttrToField(attr).AddTo(enc)
+		converted := convertAttrToFields(attr)
+		converted.AddTo(enc)
+		converted.Return()
 	}
 	return nil
 }
 
-func convertAttrToField(attr slog.Attr) zapcore.Field {
+func convertAttrToFields(attr slog.Attr) *fieldSlice {
+	fields := _fieldSlices.Get()
+
 	switch attr.Value.Kind() {
 	case slog.KindBool:
-		return zap.Bool(attr.Key, attr.Value.Bool())
+		fields.Append(zap.Bool(attr.Key, attr.Value.Bool()))
 	case slog.KindDuration:
-		return zap.Duration(attr.Key, attr.Value.Duration())
+		fields.Append(zap.Duration(attr.Key, attr.Value.Duration()))
 	case slog.KindFloat64:
-		return zap.Float64(attr.Key, attr.Value.Float64())
+		fields.Append(zap.Float64(attr.Key, attr.Value.Float64()))
 	case slog.KindInt64:
-		return zap.Int64(attr.Key, attr.Value.Int64())
+		fields.Append(zap.Int64(attr.Key, attr.Value.Int64()))
 	case slog.KindString:
-		return zap.String(attr.Key, attr.Value.String())
+		fields.Append(zap.String(attr.Key, attr.Value.String()))
 	case slog.KindTime:
-		return zap.Time(attr.Key, attr.Value.Time())
+		fields.Append(zap.Time(attr.Key, attr.Value.Time()))
 	case slog.KindUint64:
-		return zap.Uint64(attr.Key, attr.Value.Uint64())
+		fields.Append(zap.Uint64(attr.Key, attr.Value.Uint64()))
 	case slog.KindGroup:
-		return zap.Object(attr.Key, groupObject(attr.Value.Group()))
+		if len(attr.Key) > 0 {
+			fields.Append(zap.Object(attr.Key, groupObject(attr.Value.Group())))
+		} else {
+			for _, gattr := range attr.Value.Group() {
+				converted := convertAttrToFields(gattr)
+				fields.Extend(converted)
+				converted.Return()
+			}
+		}
 	case slog.KindLogValuer:
-		return convertAttrToField(slog.Attr{
+		// We're not using the fieldSlice depooled in this scope, so return it
+		// to the pool.
+		fields.Return()
+
+		return convertAttrToFields(slog.Attr{
 			Key: attr.Key,
 			// TODO: resolve the value in a lazy way
 			Value: attr.Value.Resolve(),
 		})
 	default:
-		return zap.Any(attr.Key, attr.Value.Any())
+		fields.Append(zap.Any(attr.Key, attr.Value.Any()))
 	}
+
+	return fields
 }
 
 // convertSlogLevel maps slog Levels to zap Levels.
-// Note that there is some room between slog levels while zap levels are continuous, so we can't 1:1 map them.
-// See also https://go.googlesource.com/proposal/+/master/design/56345-structured-logging.md?pli=1#levels
+// Note that there is some room between slog levels while zap levels are
+// continuous, so we can't 1:1 map them. See also:
+// https://go.googlesource.com/proposal/+/master/design/56345-structured-logging.md#levels
 func convertSlogLevel(l slog.Level) zapcore.Level {
 	switch {
 	case l >= slog.LevelError:
@@ -139,6 +194,7 @@ func (h *Handler) Handle(ctx context.Context, record slog.Record) error {
 		// TODO: do we need to set the following fields?
 		// Stack:
 	}
+
 	ce := h.core.Check(ent, nil)
 	if ce == nil {
 		return nil
@@ -157,33 +213,52 @@ func (h *Handler) Handle(ctx context.Context, record slog.Record) error {
 		}
 	}
 
-	fields := make([]zapcore.Field, 0, record.NumAttrs())
+	fields := _fieldSlices.Get()
+	defer fields.Return()
+
 	record.Attrs(func(attr slog.Attr) {
-		fields = append(fields, convertAttrToField(attr))
+		converted := convertAttrToFields(attr)
+		fields.Extend(converted)
+		converted.Return()
 	})
-	ce.Write(fields...)
+	ce.Write(*fields...)
+
 	return nil
 }
 
 // WithAttrs returns a new Handler whose attributes consist of
 // both the receiver's attributes and the arguments.
 func (h *Handler) WithAttrs(attrs []slog.Attr) slog.Handler {
-	fields := make([]zapcore.Field, len(attrs))
-	for i, attr := range attrs {
-		fields[i] = convertAttrToField(attr)
+	fields := _fieldSlices.Get()
+	defer fields.Return()
+
+	for _, attr := range attrs {
+		converted := convertAttrToFields(attr)
+		fields.Extend(converted)
+		converted.Return()
 	}
-	return h.withFields(fields...)
+
+	return h.withFields(fields)
 }
 
 // WithGroup returns a new Handler with the given group appended to
 // the receiver's existing groups.
 func (h *Handler) WithGroup(group string) slog.Handler {
-	return h.withFields(zap.Namespace(group))
+	// Stack-allocate here: the number of options is known at compile time.
+	tmp := [...]zapcore.Field{
+		zap.Namespace(group),
+	}
+
+	cloned := *h
+	cloned.core = h.core.With(tmp[:])
+	return &cloned
 }
 
 // withFields returns a cloned Handler with the given fields.
-func (h *Handler) withFields(fields ...zapcore.Field) *Handler {
+func (h *Handler) withFields(fields *fieldSlice) *Handler {
+	defer fields.Return()
+
 	cloned := *h
-	cloned.core = h.core.With(fields)
+	cloned.core = h.core.With(*fields)
 	return &cloned
 }

exp/zapslog/slog_benchmark_test.go

@@ -0,0 +1,41 @@
+package zapslog_test
+
+import (
+	"io"
+	"testing"
+
+	"go.uber.org/zap/exp/zapslog"
+	"go.uber.org/zap/zapcore"
+	"golang.org/x/exp/slog"
+)
+
+func BenchmarkSlog(b *testing.B) {
+	var (
+		enc = zapcore.NewJSONEncoder(zapcore.EncoderConfig{
+			MessageKey: "msg",
+			LevelKey:   "level",
+		})
+		core   = zapcore.NewCore(enc, zapcore.AddSync(io.Discard), zapcore.DebugLevel)
+		logger = slog.New(zapslog.NewHandler(core))
+		attrs  = []any{
+			slog.String("hello", "world"),
+			slog.Group(
+				"nested group",
+				slog.String("foo", "bar"),
+				slog.String("baz", "bat"),
+			),
+			slog.Group(
+				"", // inline group
+				slog.String("foo", "bar"),
+				slog.String("baz", "bat"),
+			),
+		}
+	)
+
+	b.ReportAllocs()
+	b.ResetTimer()
+
+	for i := 0; i < b.N; i++ {
+		logger.Info("hello", attrs...)
+	}
+}

internal/pool/pool.go

@@ -0,0 +1,58 @@
+// Copyright (c) 2023 Uber Technologies, Inc.
+//
+// Permission is hereby granted, free of charge, to any person obtaining a copy
+// of this software and associated documentation files (the "Software"), to deal
+// in the Software without restriction, including without limitation the rights
+// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+// copies of the Software, and to permit persons to whom the Software is
+// furnished to do so, subject to the following conditions:
+//
+// The above copyright notice and this permission notice shall be included in
+// all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+// THE SOFTWARE.
+
+// Package pool provides internal pool utilities.
+package pool
+
+import (
+	"sync"
+)
+
+// A Pool is a generic wrapper around [sync.Pool] to provide strongly-typed
+// object pooling.
+//
+// Note that SA6002 (ref: https://staticcheck.io/docs/checks/#SA6002) will
+// not be detected, so all internal pool use must take care to only store
+// pointer types.
+type Pool[T any] struct {
+	pool sync.Pool
+}
+
+// New returns a new [Pool] for T, and will use fn to construct new Ts when
+// the pool is empty.
+func New[T any](fn func() T) *Pool[T] {
+	return &Pool[T]{
+		pool: sync.Pool{
+			New: func() any {
+				return fn()
+			},
+		},
+	}
+}
+
+// Get gets a T from the pool, or creates a new one if the pool is empty.
+func (p *Pool[T]) Get() T {
+	return p.pool.Get().(T)
+}
+
+// Put returns x into the pool.
+func (p *Pool[T]) Put(x T) {
+	p.pool.Put(x)
+}