Redesigned the way WithXXX() works

The new design passes an atomic pointer to the named logger's base handler, and a slice of transformer functions which can rebuild the final handler by replaying a sequence of WithGroup and WithAttrs calls.

Each flume.handler now stores a copy of the base delegate's pointer, and a copy of the final delegate, run through the transformers.  Each time it logs a record, it checks whether the base pointer has changed, and if so, re-builds and caches the final delegate.

This design has many advantages, and one cost.  The cost is that it does two atomic.Pointer.Load() calls per log, but in bench testing that is only adding about 1ns overhead (more than 10x faster than a mutex).  The advantages are:

- no references from parents to children, meaning no need to implement weakrefs
- don't need to track groups and attrs the child handlers
- fewer locks
- changing a setting in the Controller is a bit faster...the new sink is propagated lazily to child handlers

v2/TODO.md

@@ -41,5 +41,6 @@
 - [ ] Should there be a bridge from v1 to v2?  Add a way to direct all v1 calls to v2 calls?
   - Working on a slog handler -> zap core bridge, and a zap core -> slog handler bridge
 - [ ] A gofix style tool to migrate a codebase from v1 to v2, and migrating from Log() to LogCtx() calls?
-- [ ] I think the states need to be re-organized back into a parent-child graph, and sinks need to trickle down that tree.  Creating all the handlers and states in conf isn't working the way it was intended.  Rebuilding the leaf handlers is grouping the cached attrs wrong (need tests to verify this), and is also inefficient, since it creates inefficient calls to the sink's WithAttrs()
-- [ ] Add a middleware which supports ReplaceAttr.  Could be used to add ReplaceAttr support to Handlers which don't natively support it
+- [x] I think the states need to be re-organized back into a parent-child graph, and sinks need to trickle down that tree.  Creating all the handlers and states in conf isn't working the way it was intended.  Rebuilding the leaf handlers is grouping the cached attrs wrong (need tests to verify this), and is also inefficient, since it creates inefficient calls to the sink's WithAttrs()
+- [ ] Add a middleware which supports ReplaceAttr.  Could be used to add ReplaceAttr support to Handlers which don't natively support it
+  - [ ] We could then promote ReplaceAttr support to the root of Config.  If the selected handler natively supports ReplaceAttr, great, otherwise we can add the middleware.  To support this, change the way handlers are registered with Config, so that each registration provides a factory method for building the handler, which can take the Config object, and adapt it to the native options that handler supports.

v2/conf.go

@@ -2,28 +2,22 @@ package flume
 
 import (
 	"log/slog"
-	"runtime"
-	"sync"
+	"sync/atomic"
 )
 
 type conf struct {
-	name      string
-	lvl       *slog.LevelVar
-	customLvl bool
+	name                  string
+	lvl                   *slog.LevelVar
+	customLvl, customSink bool
 	// sink is the ultimate, final handler
 	// delegate is the sink wrapped with middleware
-	sink, delegate slog.Handler
-	customSink     bool
-	sync.Mutex
-	states           map[*state]struct{}
+	sink             slog.Handler
+	delegatePtr      atomic.Pointer[slog.Handler]
 	middleware       []Middleware
 	globalMiddleware []Middleware
 }
 
 func (c *conf) setSink(sink slog.Handler, isDefault bool) {
-	c.Lock()
-	defer c.Unlock()
-
 	if c.customSink && isDefault {
 		return
 	}
@@ -51,11 +45,9 @@ func (c *conf) rebuildDelegate() {
 		h = c.globalMiddleware[i].Apply(h)
 	}
 
-	c.delegate = h
+	h = h.WithAttrs([]slog.Attr{slog.String(LoggerKey, c.name)})
 
-	for s := range c.states {
-		s.setDelegate(c.delegate)
-	}
+	c.delegatePtr.Store(&h)
 }
 
 func (c *conf) setLevel(l slog.Level, isDefault bool) {
@@ -72,50 +64,20 @@ func (c *conf) setLevel(l slog.Level, isDefault bool) {
 }
 
 func (c *conf) use(middleware ...Middleware) {
-	c.Lock()
-	defer c.Unlock()
-
 	c.middleware = append(c.middleware, middleware...)
 
 	c.rebuildDelegate()
 }
 
 func (c *conf) setGlobalMiddleware(middleware []Middleware) {
-	c.Lock()
-	defer c.Unlock()
-
 	c.globalMiddleware = middleware
 
 	c.rebuildDelegate()
 }
 
-func (c *conf) newHandler(attrs []slog.Attr, groups []string) *handler {
-	c.Lock()
-	defer c.Unlock()
-
-	s := &state{
-		attrs:  attrs,
-		groups: groups,
-		level:  c.lvl,
-		conf:   c,
+func (c *conf) handler() slog.Handler {
+	return &handler{
+		basePtr: &c.delegatePtr,
+		level:   c.lvl,
 	}
-	s.setDelegate(c.delegate)
-
-	c.states[s] = struct{}{}
-
-	h := &handler{
-		state: s,
-	}
-
-	// when the handler goes out of scope, run a finalizer which
-	// removes the state reference from its parent state, allowing
-	// it to be gc'd
-	runtime.SetFinalizer(h, func(_ *handler) {
-		c.Lock()
-		defer c.Unlock()
-
-		delete(c.states, s)
-	})
-
-	return h
 }

v2/config.go

@@ -87,6 +87,7 @@ type Config struct {
 const (
 	EncodingJSON      = "json"
 	EncodingText      = "text"
+	EncodingTerm      = "term"
 	EncodingTermColor = "term-color"
 )
 
@@ -260,16 +261,17 @@ func (c Config) Handler() slog.Handler {
 	switch c.Encoding {
 	case "text", "console":
 		handler = slog.NewTextHandler(out, &opts)
-	case "term":
+	case EncodingTerm:
 		handler = console.NewHandler(out, &console.HandlerOptions{
+			NoColor:     true,
 			AddSource:   c.AddSource,
 			Theme:       console.NewDefaultTheme(),
 			ReplaceAttr: ChainReplaceAttrs(c.ReplaceAttrs...),
 			TimeFormat:  "15:04:05.000",
 			Headers:     []string{LoggerKey},
 			HeaderWidth: 13,
 		})
-	case "term-color":
+	case EncodingTermColor:
 		handler = console.NewHandler(out, &console.HandlerOptions{
 			AddSource:   c.AddSource,
 			Theme:       console.NewDefaultTheme(),

v2/controller.go

@@ -59,7 +59,7 @@ func (c *Controller) Handler(name string) slog.Handler {
 	c.mutex.Lock()
 	defer c.mutex.Unlock()
 
-	return c.conf(name).newHandler([]slog.Attr{slog.String(LoggerKey, name)}, nil)
+	return c.conf(name).handler()
 }
 
 // conf locates or creates a new conf for the given name.  The Controller
@@ -75,7 +75,6 @@ func (c *Controller) conf(name string) *conf {
 		cfg = &conf{
 			name:             name,
 			lvl:              levelVar,
-			states:           map[*state]struct{}{},
 			globalMiddleware: c.defaultMiddleware,
 		}
 		cfg.setSink(c.defaultSink, true)

v2/handler.go

@@ -8,86 +8,76 @@ import (
 )
 
 type handler struct {
-	*state
-}
+	// atomic pointer to the base delegate
+	basePtr *atomic.Pointer[slog.Handler]
 
-type state struct {
-	// attrs associated with this handler.  immutable
-	attrs []slog.Attr
-	// group associated with this handler.  immutable
-	groups []string
+	// atomic pointer to a memoized copy of the base
+	// delegate, plus any transformations (i.e. WithGroup or WithAttrs calls)
+	memoPrt atomic.Pointer[[2]*slog.Handler]
 
-	// atomic pointer to handler delegate
-	delegatePtr atomic.Pointer[slog.Handler]
+	// list of WithGroup/WithAttrs transformations.  Can be re-applied
+	// to the base delegate any time it changes
+	transformers []func(slog.Handler) slog.Handler
 
 	// should be reference to the levelVar in the parent conf
 	level *slog.LevelVar
-
-	conf *conf
 }
 
-func (s *state) setDelegate(delegate slog.Handler) {
-	// re-apply groups and attrs settings
-	// don't need to check if s.attrs is empty: it will never be empty because
-	// all flume handlers have at least a "logger_name" attribute
-	// TODO: I think this logic is wrong.  this assumes
-	// that all these attrs are either nested in *no* groups, or
-	// nested in *all* the groups.  Really, each attr will only
-	// be nested in whatever set of groups was active when that
-	// attr was first added.
-	// TODO: Need to go back to each state holding pointers to
-	// children, and trickling delegate or conf changes down
-	// to to children.  And children need to point to parents...
-	// need to ensure that *only leaf states* are eligible for
-	// garbage collection, and not states which still have
-	// referenced children.
-	// Also, I think each state only needs to hold the set of
-	// attrs which were used to create that state using WithAttrs.
-	// It doesn't need the list of all attrs cached by parent
-	// states.  So long as those parents already embedded those attrs
-	// in their respective delegate handlers, and those delegate
-	// handlers have already been trickled down to this state,
-	// this state doesn't care about those parent attrs.  The state
-	// only needs to hold on to its own attrs in case a new delegate
-	// trickles down, and the state needs to rebuild its own delegate
-	delegate = delegate.WithAttrs(slices.Clone(s.attrs))
-	for _, g := range s.groups {
-		delegate = delegate.WithGroup(g)
+func (s *handler) WithAttrs(attrs []slog.Attr) slog.Handler {
+	transformer := func(h slog.Handler) slog.Handler {
+		return h.WithAttrs(attrs)
+	}
+	return &handler{
+		basePtr:      s.basePtr,
+		level:        s.level,
+		transformers: slices.Clip(append(s.transformers, transformer)),
 	}
-
-	s.delegatePtr.Store(&delegate)
-}
-
-func (s *state) delegate() slog.Handler {
-	handlerRef := s.delegatePtr.Load()
-	return *handlerRef
 }
 
-func (s *state) WithAttrs(attrs []slog.Attr) slog.Handler {
-	// TODO: I think I need to clone or clip this slice
-	// TODO: this is actually pretty inefficient.  Each time this is
-	// called, we end up re-calling WithAttrs and WithGroup on the delegate
-	// several times.
-	// TODO: consider adding native support for ReplaceAttr, and calling it
-	// here...that way I can implement ReplaceAttrs in flume, and it
-	// doesn't matter if the sinks implement it.  I'd need to add calls
-	// to it in Handle() as well.
-	return s.conf.newHandler(append(s.attrs, attrs...), s.groups)
-}
+func (s *handler) WithGroup(name string) slog.Handler {
+	transformer := func(h slog.Handler) slog.Handler {
+		return h.WithGroup(name)
+	}
 
-func (s *state) WithGroup(name string) slog.Handler {
-	// TODO: I think I need to clone or clip this slice
-	return s.conf.newHandler(s.attrs, append(s.groups, name))
+	return &handler{
+		basePtr:      s.basePtr,
+		level:        s.level,
+		transformers: slices.Clip(append(s.transformers, transformer)),
+	}
 }
 
-func (s *state) Enabled(_ context.Context, level slog.Level) bool {
+func (s *handler) Enabled(_ context.Context, level slog.Level) bool {
 	return level >= s.level.Level()
 }
 
-func (s *state) Handle(ctx context.Context, record slog.Record) error {
+func (s *handler) Handle(ctx context.Context, record slog.Record) error {
 	return s.delegate().Handle(ctx, record)
 }
 
+func (s *handler) delegate() slog.Handler {
+	base := s.basePtr.Load()
+	if base == nil {
+		return noop
+	}
+	memo := s.memoPrt.Load()
+	if memo != nil && memo[0] == base {
+		hPtr := memo[1]
+		if hPtr != nil {
+			return *hPtr
+		}
+	}
+	// build and memoize
+	delegate := *base
+	for _, transformer := range s.transformers {
+		delegate = transformer(delegate)
+	}
+	if delegate == nil {
+		delegate = noop
+	}
+	s.memoPrt.Store(&[2]*slog.Handler{base, &delegate})
+	return delegate
+}
+
 var noop = noopHandler{}
 
 type noopHandler struct{}