slog: redesign Record

- Return to the implementation consisting of an inline array followed
  by a slice.

- Document the alias problems that can arise.

- Provide Record.Clone to make a deep copy so aliasing bugs can be
  avoided.

- Replace AddAttr with AddAttrs, to permit more efficiently adding
  Attrs, and to provide more implementation flexibility.

- Eliminate random access: support only forward iteration.
  Accomplish it with a method that takes a func(Attr).
  Call this method "Attrs"; the old "Attrs", which returned a []Attr,
  can now be written in terms of the new one.
  We do not provide a way to stop iteration early, because the
  large majority of uses will want to visit all the Attrs.

- Call the constructor NewRecord, not MakeRecord, because Record isn't
  truly a value type.

Change-Id: Iac6843cd8a27e351fc4bde79aa59c6cc2e526e46
Reviewed-on: https://go-review.googlesource.com/c/exp/+/431276
Reviewed-by: Alan Donovan <adonovan@google.com>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>

Author: jba

slog/handler.go

@@ -57,10 +57,10 @@ func (h *defaultHandler) Handle(r Record) error {
 		b.WriteString(r.Level().String())
 		b.WriteByte(' ')
 	}
-	for i := 0; i < r.NumAttrs(); i++ {
-		fmt.Fprint(&b, r.Attr(i)) // Attr.Format will print key=value
+	r.Attrs(func(a Attr) {
+		fmt.Fprint(&b, a) // Attr.Format will print key=value
 		b.WriteByte(' ')
-	}
+	})
 	b.WriteString(r.Message())
 	return log.Output(4, b.String())
 }
@@ -205,13 +205,12 @@ func (h *commonHandler) handle(r Record) error {
 		replace(String(key, val))
 	}
 	*buf = append(*buf, h.preformattedAttrs...)
-	for i := 0; i < r.NumAttrs(); i++ {
-		a := r.Attr(i)
+	r.Attrs(func(a Attr) {
 		if rep != nil {
 			a = rep(a)
 		}
 		appendAttr(app, a)
-	}
+	})
 	app.appendEnd()
 	buf.WriteByte('\n')
 

slog/handler_test.go

@@ -36,10 +36,8 @@ func TestWith(t *testing.T) {
 
 func TestCommonHandle(t *testing.T) {
 	tm := time.Now()
-	r := MakeRecord(tm, InfoLevel, "message", 1)
-	r.AddAttr(String("a", "one"))
-	r.AddAttr(Int("b", 2))
-	r.AddAttr(Any("", "ignore me"))
+	r := NewRecord(tm, InfoLevel, "message", 1)
+	r.AddAttrs(String("a", "one"), Int("b", 2), Any("", "ignore me"))
 
 	for _, test := range []struct {
 		name string

slog/json_handler_test.go

@@ -21,8 +21,8 @@ import (
 func TestJSONHandler(t *testing.T) {
 	var buf bytes.Buffer
 	h := NewJSONHandler(&buf)
-	r := MakeRecord(testTime, InfoLevel, "m", 0)
-	r.AddAttr(Int("a", 1))
+	r := NewRecord(testTime, InfoLevel, "m", 0)
+	r.AddAttrs(Int("a", 1))
 	if err := h.Handle(r); err != nil {
 		t.Fatal(err)
 	}

slog/logger.go

@@ -47,7 +47,7 @@ func (w *handlerWriter) Write(buf []byte) (int, error) {
 	if len(buf) > 0 && buf[len(buf)-1] == '\n' {
 		buf = buf[:len(buf)-1]
 	}
-	r := MakeRecord(time.Now(), InfoLevel, string(buf), depth)
+	r := NewRecord(time.Now(), InfoLevel, string(buf), depth)
 	return origLen, w.h.Handle(r)
 }
 
@@ -113,7 +113,7 @@ func (l *Logger) LogDepth(calldepth int, level Level, msg string, args ...any) {
 		return
 	}
 	r := l.makeRecord(msg, level, calldepth)
-	setAttrs(&r, args)
+	r.setAttrsFromArgs(args)
 	_ = l.Handler().Handle(r)
 }
 
@@ -127,39 +127,7 @@ func (l *Logger) makeRecord(msg string, level Level, depth int) Record {
 	if useSourceLine {
 		depth += 5
 	}
-	return MakeRecord(time.Now(), level, msg, depth)
-}
-
-const badKey = "!BADKEY"
-
-func setAttrs(r *Record, args []any) {
-	var attr Attr
-	for len(args) > 0 {
-		attr, args = argsToAttr(args)
-		r.AddAttr(attr)
-	}
-}
-
-// argsToAttr turns a prefix of the args slice into an Attr and returns
-// the unused portion of the slice.
-// If args[0] is an Attr, it returns it.
-// If args[0] is a string, it treats the first two elements as
-// a key-value pair.
-// Otherwise, it treats args[0] as a value with a missing key.
-func argsToAttr(args []any) (Attr, []any) {
-	switch x := args[0].(type) {
-	case string:
-		if len(args) == 1 {
-			return String(badKey, x), nil
-		}
-		return Any(x, args[1]), args[2:]
-
-	case Attr:
-		return x, args[1:]
-
-	default:
-		return Any(badKey, x), args[1:]
-	}
+	return NewRecord(time.Now(), level, msg, depth)
 }
 
 // LogAttrs is a more efficient version of [Logger.Log] that accepts only Attrs.
@@ -174,7 +142,7 @@ func (l *Logger) LogAttrsDepth(calldepth int, level Level, msg string, attrs ...
 		return
 	}
 	r := l.makeRecord(msg, level, calldepth)
-	r.addAttrs(attrs)
+	r.AddAttrs(attrs...)
 	_ = l.Handler().Handle(r)
 }
 

slog/logger_test.go

@@ -91,7 +91,7 @@ func TestAttrs(t *testing.T) {
 	l2.Info("m", "c", 3)
 	h := l2.Handler().(*captureHandler)
 	check(h.attrs, Int("a", 1), Int("b", 2))
-	check(h.r.Attrs(), Int("c", 3))
+	check(attrsSlice(h.r), Int("c", 3))
 }
 
 func TestCallDepth(t *testing.T) {
@@ -199,7 +199,7 @@ func TestAlloc(t *testing.T) {
 		s := "abc"
 		i := 2000
 		d := time.Second
-		wantAllocs(t, 10, func() {
+		wantAllocs(t, 11, func() {
 			dl.Info("hello",
 				"n", i, "s", s, "d", d,
 				"n", i, "s", s, "d", d,
@@ -253,9 +253,9 @@ func TestSetAttrs(t *testing.T) {
 		{[]any{"a", 1, "b"}, []Attr{Int("a", 1), String(badKey, "b")}},
 		{[]any{"a", 1, 2, 3}, []Attr{Int("a", 1), Int(badKey, 2), Int(badKey, 3)}},
 	} {
-		r := MakeRecord(time.Time{}, 0, "", 0)
-		setAttrs(&r, test.args)
-		got := r.Attrs()
+		r := NewRecord(time.Time{}, 0, "", 0)
+		r.setAttrsFromArgs(test.args)
+		got := attrsSlice(r)
 		if !attrsEqual(got, test.want) {
 			t.Errorf("%v:\ngot  %v\nwant %v", test.args, got, test.want)
 		}

slog/record.go

@@ -12,6 +12,9 @@ import (
 const nAttrsInline = 5
 
 // A Record holds information about a log event.
+// Copies of a Record share state.
+// Do not modify a Record after handing out a copy to it.
+// Use [Record.Clone] to create a copy with no shared state.
 type Record struct {
 	// The time at which the output method (Log, Info, etc.) was called.
 	time time.Time
@@ -28,26 +31,28 @@ type Record struct {
 
 	// Allocation optimization: an inline array sized to hold
 	// the majority of log calls (based on examination of open-source
-	// code). The array holds the end of the sequence of Attrs.
-	tail [nAttrsInline]Attr
+	// code). It holds the start of the list of Attrs.
+	front [nAttrsInline]Attr
 
-	// The number of Attrs in tail.
-	nTail int
+	// The number of Attrs in front.
+	nFront int
 
-	// The sequence of Attrs except for the tail, represented as a functional
-	// list of arrays.
-	attrs list[[nAttrsInline]Attr]
+	// The list of Attrs except for those in front.
+	// Invariants:
+	//   - len(back) > 0 iff nFront == len(front)
+	//   - Unused array elements are zero. Used to detect mistakes.
+	back []Attr
 }
 
-// MakeRecord creates a new Record from the given arguments.
-// Use [Record.AddAttr] to add attributes to the Record.
+// NewRecord creates a Record from the given arguments.
+// Use [Record.AddAttrs] to add attributes to the Record.
 // If calldepth is greater than zero, [Record.SourceLine] will
 // return the file and line number at that depth,
-// where 1 means the caller of MakeRecord.
+// where 1 means the caller of NewRecord.
 //
-// MakeRecord is intended for logging APIs that want to support a [Handler] as
+// NewRecord is intended for logging APIs that want to support a [Handler] as
 // a backend.
-func MakeRecord(t time.Time, level Level, msg string, calldepth int) Record {
+func NewRecord(t time.Time, level Level, msg string, calldepth int) Record {
 	var p uintptr
 	if calldepth > 0 {
 		p = pc(calldepth + 1)
@@ -85,50 +90,97 @@ func (r *Record) SourceLine() (file string, line int) {
 	return f.File, f.Line
 }
 
-// Attrs returns a copy of the sequence of Attrs in r.
-func (r *Record) Attrs() []Attr {
-	res := make([]Attr, 0, r.attrs.len()*nAttrsInline+r.nTail)
-	r.attrs = r.attrs.normalize()
-	for _, f := range r.attrs.front {
-		res = append(res, f[:]...)
+// Clone returns a copy of the record with no shared state.
+// The original record and the clone can both be modified
+// without interfering with each other.
+func (r *Record) Clone() Record {
+	c := *r
+	if len(c.back) > 0 {
+		c.back = make([]Attr, len(c.back))
+		copy(c.back, r.back)
 	}
-	for _, a := range r.tail[:r.nTail] {
-		res = append(res, a)
-	}
-	return res
+	return c
 }
 
-// NumAttrs returns the number of Attrs in r.
+// NumAttrs returns the number of attributes in the Record.
 func (r *Record) NumAttrs() int {
-	return r.attrs.len()*nAttrsInline + r.nTail
+	return r.nFront + len(r.back)
+}
+
+// Attrs calls f on each Attr in the Record.
+func (r *Record) Attrs(f func(Attr)) {
+	for i := 0; i < r.nFront; i++ {
+		f(r.front[i])
+	}
+	for _, a := range r.back {
+		f(a)
+	}
 }
 
-// Attr returns the i'th Attr in r.
-func (r *Record) Attr(i int) Attr {
-	if r.attrs.back != nil {
-		r.attrs = r.attrs.normalize()
+// AddAttrs appends the given attrs to the Record's list of Attrs.
+func (r *Record) AddAttrs(attrs ...Attr) {
+	n := copy(r.front[r.nFront:], attrs)
+	r.nFront += n
+	// Check if a copy was modified by slicing past the end
+	// and seeing if the Attr there is non-zero.
+	if cap(r.back) > len(r.back) {
+		end := r.back[:len(r.back)+1][len(r.back)]
+		if end != (Attr{}) {
+			panic("copies of a slog.Record were both modified")
+		}
 	}
-	alen := r.attrs.len() * nAttrsInline
-	if i < alen {
-		return r.attrs.at(i / nAttrsInline)[i%nAttrsInline]
+	r.back = append(r.back, attrs[n:]...)
+}
+
+func (r *Record) setAttrsFromArgs(args []any) {
+	var a Attr
+	for len(args) > 0 {
+		a, args = argsToAttr(args)
+		if r.nFront < len(r.front) {
+			r.front[r.nFront] = a
+			r.nFront++
+		} else {
+			if r.back == nil {
+				r.back = make([]Attr, 0, countAttrs(args))
+			}
+			r.back = append(r.back, a)
+		}
 	}
-	return r.tail[i-alen]
+
 }
 
-// AddAttr appends an attributes to the record's list of attributes.
-// It does not check for duplicate keys.
-func (r *Record) AddAttr(a Attr) {
-	if r.nTail == len(r.tail) {
-		r.attrs = r.attrs.append(r.tail)
-		r.nTail = 0
+// countAttrs returns the number of Attrs that would be created from args.
+func countAttrs(args []any) int {
+	n := 0
+	for i := 0; i < len(args); i++ {
+		n++
+		if _, ok := args[i].(string); ok {
+			i++
+		}
 	}
-	r.tail[r.nTail] = a
-	r.nTail++
+	return n
 }
 
-func (r *Record) addAttrs(attrs []Attr) {
-	// TODO: be cleverer.
-	for _, a := range attrs {
-		r.AddAttr(a)
+const badKey = "!BADKEY"
+
+// argsToAttr turns a prefix of the nonempty args slice into an Attr
+// and returns the unconsumed portion of the slice.
+// If args[0] is an Attr, it returns it.
+// If args[0] is a string, it treats the first two elements as
+// a key-value pair.
+// Otherwise, it treats args[0] as a value with a missing key.
+func argsToAttr(args []any) (Attr, []any) {
+	switch x := args[0].(type) {
+	case string:
+		if len(args) == 1 {
+			return String(badKey, x), nil
+		}
+		return Any(x, args[1]), args[2:]
+
+	case Attr:
+		return x, args[1:]
+
+	default:
+		return Any(badKey, x), args[1:]
 	}
 }