base: optimize {NodeIDContainer,StoreIDContainer}.String()

These String() methods were implemented in terms of their respective
SafeFormat, which was pretty expensive: upwards of 750ns and between 4-7
allocations depending on the node id. This cost caused at least two
workarounds, that the patch annotates.

The patch makes stringifying cheap by precomputing the value and moving
from SafeFormatter to SafeValue. Moving away from SafeFormatter to a
more down-to-earth implementation brings the cost down to between 0 and
1 allocations. But I went further and precomputed the value because
these containers are used as logging tags and so can easily end up
being stringified very frequently.

Release note: None

docs/generated/redact_safe.md

@@ -2,6 +2,8 @@ The following types are considered always safe for reporting:
 
 File | Type
 --|--
+pkg/base/node_id.go | `*NodeIDContainer`
+pkg/base/node_id.go | `*StoreIDContainer`
 pkg/cli/exit/exit.go | `Code`
 pkg/jobs/jobspb/wrap.go | `Type`
 pkg/kv/kvserver/closedts/ctpb/service.go | `LAI`

pkg/base/node_id.go

@@ -29,26 +29,29 @@ import (
 type NodeIDContainer struct {
 	_ util.NoCopy
 
-	// nodeID is atomically updated under the mutex; it can be read atomically
-	// without the mutex.
+	// nodeID is accessed atomically.
 	nodeID int32
+
+	// If nodeID has been set, str represents nodeID converted to string. We
+	// precompute this value to speed up String() and keep it from allocating
+	// memory dynamically.
+	str atomic.Value
 }
 
 // String returns the node ID, or "?" if it is unset.
 func (n *NodeIDContainer) String() string {
-	return redact.StringWithoutMarkers(n)
-}
-
-// SafeFormat implements the redact.SafeFormatter interface.
-func (n *NodeIDContainer) SafeFormat(w redact.SafePrinter, _ rune) {
-	val := n.Get()
-	if val == 0 {
-		w.SafeRune('?')
-	} else {
-		w.Print(val)
+	s := n.str.Load()
+	if s == nil {
+		return "?"
 	}
+	return s.(string)
 }
 
+var _ redact.SafeValue = &NodeIDContainer{}
+
+// SafeValue implements the redact.SafeValue interface.
+func (n *NodeIDContainer) SafeValue() {}
+
 // Get returns the current node ID; 0 if it is unset.
 func (n *NodeIDContainer) Get() roachpb.NodeID {
 	return roachpb.NodeID(atomic.LoadInt32(&n.nodeID))
@@ -67,13 +70,15 @@ func (n *NodeIDContainer) Set(ctx context.Context, val roachpb.NodeID) {
 	} else if oldVal != int32(val) {
 		log.Fatalf(ctx, "different NodeIDs set: %d, then %d", oldVal, val)
 	}
+	n.str.Store(strconv.Itoa(int(val)))
 }
 
 // Reset changes the NodeID regardless of the old value.
 //
 // Should only be used in testing code.
 func (n *NodeIDContainer) Reset(val roachpb.NodeID) {
 	atomic.StoreInt32(&n.nodeID, int32(val))
+	n.str.Store(strconv.Itoa(int(val)))
 }
 
 // StoreIDContainer is added as a logtag in the pebbleLogger's context.
@@ -83,9 +88,13 @@ func (n *NodeIDContainer) Reset(val roachpb.NodeID) {
 type StoreIDContainer struct {
 	_ util.NoCopy
 
-	// After the struct is initially created, storeID is atomically
-	// updated under the mutex; it can be read atomically without the mutex.
+	// storeID is accessed atomically.
 	storeID int32
+
+	// If storeID has been set, str represents storeID converted to string. We
+	// precompute this value to speed up String() and keep it from allocating
+	// memory dynamically.
+	str atomic.Value
 }
 
 // TempStoreID is used as the store id for a temp pebble engine's log
@@ -95,21 +104,18 @@ const TempStoreID = -1
 // stores if they haven't been initialized. If a main store hasn't
 // been initialized, then "?" is returned.
 func (s *StoreIDContainer) String() string {
-	return redact.StringWithoutMarkers(s)
-}
-
-// SafeFormat implements the redact.SafeFormatter interface.
-func (s *StoreIDContainer) SafeFormat(w redact.SafePrinter, _ rune) {
-	val := s.Get()
-	if val == 0 {
-		w.SafeRune('?')
-	} else if val == TempStoreID {
-		w.Print("temp")
-	} else {
-		w.Print(val)
+	str := s.str.Load()
+	if str == nil {
+		return "?"
 	}
+	return str.(string)
 }
 
+var _ redact.SafeValue = &StoreIDContainer{}
+
+// SafeValue implements the redact.SafeValue interface.
+func (s *StoreIDContainer) SafeValue() {}
+
 // Get returns the current storeID; 0 if it is unset.
 func (s *StoreIDContainer) Get() int32 {
 	return atomic.LoadInt32(&s.storeID)
@@ -133,6 +139,11 @@ func (s *StoreIDContainer) Set(ctx context.Context, val int32) {
 				oldVal, val)
 		}
 	}
+	if val == TempStoreID {
+		s.str.Store("temp")
+	} else {
+		s.str.Store(strconv.Itoa(int(val)))
+	}
 }
 
 // A SQLInstanceID is an ephemeral ID assigned to a running instance of the SQL

pkg/rpc/context.go

@@ -777,7 +777,7 @@ func (ctx *Context) grpcDialOptions(
 		// is in setupSpanForIncomingRPC().
 		//
 		tagger := func(span *tracing.Span) {
-			span.SetTag("node", attribute.StringValue(ctx.NodeID.Get().String()))
+			span.SetTag("node", attribute.IntValue(int(ctx.NodeID.Get())))
 		}
 		unaryInterceptors = append(unaryInterceptors,
 			tracing.ClientInterceptor(tracer, tagger))

pkg/util/tracing/span_inner.go

@@ -81,6 +81,11 @@ func (s *spanInner) GetRecording() Recording {
 	}
 	// If the span is not verbose, optimize by avoiding the tags.
 	// This span is likely only used to carry payloads around.
+	//
+	// TODO(andrei): The optimization for avoiding the tags was done back when
+	// stringifying a {NodeID,StoreID}Container (a very common tag) was expensive.
+	// That has become cheap since, so this optimization might not be worth it any
+	// more.
 	wantTags := s.crdb.recordingType() == RecordingVerbose
 	return s.crdb.getRecording(wantTags)
 }