Merge #108272

108272: util/must: revert API r=erikgrinaker a=erikgrinaker This patch reverts #106508, since `@RaduBerinde` [pointed out](#107790 (review)) a performance flaw where it will often incur an allocation on the happy path due to interface boxing of the format args. Other options are considered in #108169. We'll revisit runtime assertions with a different API that avoids this cost on the happy path. Co-authored-by: Erik Grinaker <[email protected]>
cockroachdb · Aug 11, 2023 · 62e59ec · 62e59ec
2 parents 6fa90e9 + c568373
commit 62e59ec
Show file tree

Hide file tree

Showing 42 changed files with 84 additions and 1,299 deletions.
diff --git a/pkg/BUILD.bazel b/pkg/BUILD.bazel
@@ -664,7 +664,6 @@ ALL_TESTS = [
     "//pkg/util/metric/aggmetric:aggmetric_test",
     "//pkg/util/metric:metric_test",
     "//pkg/util/mon:mon_test",
-    "//pkg/util/must:must_test",
     "//pkg/util/netutil/addr:addr_test",
     "//pkg/util/netutil:netutil_test",
     "//pkg/util/optional:optional_test",
@@ -2349,8 +2348,6 @@ GO_TARGETS = [
     "//pkg/util/metric:metric_test",
     "//pkg/util/mon:mon",
     "//pkg/util/mon:mon_test",
-    "//pkg/util/must:must",
-    "//pkg/util/must:must_test",
     "//pkg/util/netutil/addr:addr",
     "//pkg/util/netutil/addr:addr_test",
     "//pkg/util/netutil:netutil",

diff --git a/pkg/kv/kvserver/BUILD.bazel b/pkg/kv/kvserver/BUILD.bazel
@@ -206,7 +206,6 @@ go_library(
         "//pkg/util/metric",
         "//pkg/util/metric/aggmetric",
         "//pkg/util/mon",
-        "//pkg/util/must",
         "//pkg/util/pprofutil",
         "//pkg/util/protoutil",
         "//pkg/util/quotapool",

diff --git a/pkg/kv/kvserver/batcheval/BUILD.bazel b/pkg/kv/kvserver/batcheval/BUILD.bazel
@@ -55,6 +55,7 @@ go_library(
     importpath = "github.com/cockroachdb/cockroach/pkg/kv/kvserver/batcheval",
     visibility = ["//visibility:public"],
     deps = [
+        "//pkg/build",
         "//pkg/clusterversion",
         "//pkg/keys",
         "//pkg/kv/kvpb",
@@ -84,7 +85,6 @@ go_library(
         "//pkg/util/limit",
         "//pkg/util/log",
         "//pkg/util/mon",
-        "//pkg/util/must",
         "//pkg/util/protoutil",
         "//pkg/util/tracing",
         "//pkg/util/uuid",

diff --git a/pkg/kv/kvserver/batcheval/cmd_clear_range.go b/pkg/kv/kvserver/batcheval/cmd_clear_range.go
@@ -23,9 +23,10 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/storage"
 	"github.com/cockroachdb/cockroach/pkg/storage/enginepb"
+	"github.com/cockroachdb/cockroach/pkg/util"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
-	"github.com/cockroachdb/cockroach/pkg/util/must"
 	"github.com/cockroachdb/errors"
+	"github.com/kr/pretty"
 )
 
 // ClearRangeBytesThreshold is the threshold over which the ClearRange
@@ -173,60 +174,63 @@ func computeStatsDelta(
 
 	// We can avoid manually computing the stats delta if we're clearing
 	// the entire range.
-	if desc.StartKey.Equal(from) && desc.EndKey.Equal(to) {
+	entireRange := desc.StartKey.Equal(from) && desc.EndKey.Equal(to)
+	if entireRange {
 		// Note this it is safe to use the full range MVCC stats, as
-		// opposed to the usual method of computing only a localized
+		// opposed to the usual method of computing only a localizied
 		// stats delta, because a full-range clear prevents any concurrent
 		// access to the stats. Concurrent changes to range-local keys are
 		// explicitly ignored (i.e. SysCount, SysBytes).
 		delta = cArgs.EvalCtx.GetMVCCStats()
 		delta.SysCount, delta.SysBytes, delta.AbortSpanBytes = 0, 0, 0 // no change to system stats
+	}
 
-		// Assert correct stats.
-		if err := must.Expensive(func() error {
-			if delta.ContainsEstimates != 0 {
-				return nil
-			}
-			computed, err := storage.ComputeStats(readWriter, from, to, delta.LastUpdateNanos)
-			if err != nil {
-				return err
-			}
-			return must.Equal(ctx, delta, computed, "range MVCC stats differ from computed")
-		}); err != nil {
-			return enginepb.MVCCStats{}, err
-		}
-
-	} else {
-		// If we can't use the fast path, compute stats across the cleared span.
-		var err error
-		delta, err = storage.ComputeStats(readWriter, from, to, delta.LastUpdateNanos)
+	// If we can't use the fast stats path, or race test is enabled, compute stats
+	// across the key span to be cleared.
+	if !entireRange || util.RaceEnabled {
+		computed, err := storage.ComputeStats(readWriter, from, to, delta.LastUpdateNanos)
 		if err != nil {
 			return enginepb.MVCCStats{}, err
 		}
-
-		// We need to adjust for the fragmentation of any MVCC range tombstones that
-		// straddle the span bounds. The clearing of the inner fragments has already
-		// been accounted for above. We take care not to peek outside the Raft range
-		// bounds.
-		leftPeekBound, rightPeekBound := rangeTombstonePeekBounds(
-			from, to, desc.StartKey.AsRawKey(), desc.EndKey.AsRawKey())
-		rkIter := readWriter.NewMVCCIterator(storage.MVCCKeyIterKind, storage.IterOptions{
-			KeyTypes:   storage.IterKeyTypeRangesOnly,
-			LowerBound: leftPeekBound,
-			UpperBound: rightPeekBound,
-		})
-		defer rkIter.Close()
-
-		if cmp, lhs, err := storage.PeekRangeKeysLeft(rkIter, from); err != nil {
-			return enginepb.MVCCStats{}, err
-		} else if cmp > 0 {
-			delta.Subtract(storage.UpdateStatsOnRangeKeySplit(from, lhs.Versions))
+		// If we took the fast path but race is enabled, assert stats were correctly
+		// computed.
+		if entireRange {
+			// Retain the value of ContainsEstimates for tests under race.
+			computed.ContainsEstimates = delta.ContainsEstimates
+			// We only want to assert the correctness of stats that do not contain
+			// estimates.
+			if delta.ContainsEstimates == 0 && !delta.Equal(computed) {
+				log.Fatalf(ctx, "fast-path MVCCStats computation gave wrong result: diff(fast, computed) = %s",
+					pretty.Diff(delta, computed))
+			}
 		}
+		delta = computed
+
+		// If we're not clearing the entire range, we need to adjust for the
+		// fragmentation of any MVCC range tombstones that straddle the span bounds.
+		// The clearing of the inner fragments has already been accounted for above.
+		// We take care not to peek outside the Raft range bounds.
+		if !entireRange {
+			leftPeekBound, rightPeekBound := rangeTombstonePeekBounds(
+				from, to, desc.StartKey.AsRawKey(), desc.EndKey.AsRawKey())
+			rkIter := readWriter.NewMVCCIterator(storage.MVCCKeyIterKind, storage.IterOptions{
+				KeyTypes:   storage.IterKeyTypeRangesOnly,
+				LowerBound: leftPeekBound,
+				UpperBound: rightPeekBound,
+			})
+			defer rkIter.Close()
+
+			if cmp, lhs, err := storage.PeekRangeKeysLeft(rkIter, from); err != nil {
+				return enginepb.MVCCStats{}, err
+			} else if cmp > 0 {
+				delta.Subtract(storage.UpdateStatsOnRangeKeySplit(from, lhs.Versions))
+			}
 
-		if cmp, rhs, err := storage.PeekRangeKeysRight(rkIter, to); err != nil {
-			return enginepb.MVCCStats{}, err
-		} else if cmp < 0 {
-			delta.Subtract(storage.UpdateStatsOnRangeKeySplit(to, rhs.Versions))
+			if cmp, rhs, err := storage.PeekRangeKeysRight(rkIter, to); err != nil {
+				return enginepb.MVCCStats{}, err
+			} else if cmp < 0 {
+				delta.Subtract(storage.UpdateStatsOnRangeKeySplit(to, rhs.Versions))
+			}
 		}
 	}
 

diff --git a/pkg/kv/kvserver/batcheval/cmd_export.go b/pkg/kv/kvserver/batcheval/cmd_export.go
@@ -16,6 +16,7 @@ import (
 	"fmt"
 	"time"
 
+	"github.com/cockroachdb/cockroach/pkg/build"
 	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvpb"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/batcheval/result"
@@ -26,7 +27,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/storage"
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
-	"github.com/cockroachdb/cockroach/pkg/util/must"
 	"github.com/cockroachdb/cockroach/pkg/util/tracing"
 	"github.com/cockroachdb/errors"
 )
@@ -274,11 +274,19 @@ func evalExport(
 					reply.ResumeReason = kvpb.RESUME_ELASTIC_CPU_LIMIT
 					break
 				} else {
-					// There should be no condition aside from resource constraints that
-					// results in an early exit without exporting any data. Regardless, if
-					// we have a resumeKey we immediately retry the ExportRequest from
-					// that key and timestamp onwards.
-					_ = must.True(ctx, resumeInfo.CPUOverlimit, "Export returned no data: %+v", resumeInfo)
+					if !resumeInfo.CPUOverlimit {
+						// We should never come here. There should be no condition aside from
+						// resource constraints that results in an early exit without
+						// exporting any data. Regardless, if we have a resumeKey we
+						// immediately retry the ExportRequest from that key and timestamp
+						// onwards.
+						if !build.IsRelease() {
+							return result.Result{}, errors.AssertionFailedf("ExportRequest exited without " +
+								"exporting any data for an unknown reason; programming error")
+						} else {
+							log.Warningf(ctx, "unexpected resume span from ExportRequest without exporting any data for an unknown reason: %v", resumeInfo)
+						}
+					}
 					start = resumeInfo.ResumeKey.Key
 					resumeKeyTS = resumeInfo.ResumeKey.Timestamp
 					continue

diff --git a/pkg/kv/kvserver/batcheval/cmd_migrate.go b/pkg/kv/kvserver/batcheval/cmd_migrate.go
@@ -22,7 +22,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/spanset"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/storage"
-	"github.com/cockroachdb/cockroach/pkg/util/must"
+	"github.com/cockroachdb/errors"
 )
 
 func init() {
@@ -63,8 +63,8 @@ func Migrate(
 	migrationVersion := args.Version
 
 	fn, ok := migrationRegistry[migrationVersion]
-	if err := must.True(ctx, ok, "migration for %s not found", migrationVersion); err != nil {
-		return result.Result{}, err
+	if !ok {
+		return result.Result{}, errors.AssertionFailedf("migration for %s not found", migrationVersion)
 	}
 	pd, err := fn(ctx, readWriter, cArgs)
 	if err != nil {

diff --git a/pkg/kv/kvserver/batcheval/cmd_push_txn.go b/pkg/kv/kvserver/batcheval/cmd_push_txn.go
@@ -15,6 +15,7 @@ import (
 	"context"
 	"time"
 
+	"github.com/cockroachdb/cockroach/pkg/build"
 	"github.com/cockroachdb/cockroach/pkg/clusterversion"
 	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvpb"
@@ -26,7 +27,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/storage"
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
-	"github.com/cockroachdb/cockroach/pkg/util/must"
 	"github.com/cockroachdb/errors"
 	"github.com/cockroachdb/redact"
 )
@@ -286,7 +286,9 @@ func PushTxn(
 	// transaction recovery procedure always finalizes target transactions, even
 	// if initiated by a PUSH_TIMESTAMP.
 	if pusheeStaging && pusherWins && pushType == kvpb.PUSH_TIMESTAMP {
-		_ = must.True(ctx, pusheeStagingFailed, "parallel commit must be known to have failed for push to succeed")
+		if !pusheeStagingFailed && !build.IsRelease() {
+			log.Fatalf(ctx, "parallel commit must be known to have failed for push to succeed")
+		}
 		pushType = kvpb.PUSH_ABORT
 	}
 

diff --git a/pkg/kv/kvserver/scheduler.go b/pkg/kv/kvserver/scheduler.go
@@ -18,8 +18,8 @@ import (
 	"unsafe"
 
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
+	"github.com/cockroachdb/cockroach/pkg/util"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
-	"github.com/cockroachdb/cockroach/pkg/util/must"
 	"github.com/cockroachdb/cockroach/pkg/util/stop"
 	"github.com/cockroachdb/cockroach/pkg/util/syncutil"
 	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
@@ -400,9 +400,11 @@ func (ss *raftSchedulerShard) worker(
 				state.flags |= stateRaftReady
 			}
 		}
-
-		_ = must.Equal(ctx, state.flags&stateRaftTick != 0, state.ticks != 0,
-			"flags %d with %d ticks", state.flags, state.ticks) // safe to continue
+		if util.RaceEnabled { // assert the ticks invariant
+			if tick := state.flags&stateRaftTick != 0; tick != (state.ticks != 0) {
+				log.Fatalf(ctx, "stateRaftTick is %v with ticks %v", tick, state.ticks)
+			}
+		}
 		if state.flags&stateRaftTick != 0 {
 			for t := state.ticks; t > 0; t-- {
 				// processRaftTick returns true if the range should perform ready

diff --git a/pkg/roachpb/BUILD.bazel b/pkg/roachpb/BUILD.bazel
@@ -39,7 +39,6 @@ go_library(
         "//pkg/util/humanizeutil",
         "//pkg/util/interval",
         "//pkg/util/log",
-        "//pkg/util/must",
         "//pkg/util/protoutil",
         "//pkg/util/syncutil",
         "//pkg/util/timetz",

diff --git a/pkg/roachpb/data.go b/pkg/roachpb/data.go
@@ -32,13 +32,13 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/concurrency/isolation"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/concurrency/lock"
 	"github.com/cockroachdb/cockroach/pkg/storage/enginepb"
+	"github.com/cockroachdb/cockroach/pkg/util"
 	"github.com/cockroachdb/cockroach/pkg/util/bitarray"
 	"github.com/cockroachdb/cockroach/pkg/util/duration"
 	"github.com/cockroachdb/cockroach/pkg/util/encoding"
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
 	"github.com/cockroachdb/cockroach/pkg/util/interval"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
-	"github.com/cockroachdb/cockroach/pkg/util/must"
 	"github.com/cockroachdb/cockroach/pkg/util/protoutil"
 	"github.com/cockroachdb/cockroach/pkg/util/timetz"
 	"github.com/cockroachdb/cockroach/pkg/util/uuid"
@@ -2472,9 +2472,11 @@ var _ sort.Interface = SequencedWriteBySeq{}
 // Find searches for the index of the SequencedWrite with the provided
 // sequence number. Returns -1 if no corresponding write is found.
 func (s SequencedWriteBySeq) Find(seq enginepb.TxnSeq) int {
-	_ = must.Expensive(func() error {
-		return must.True(context.TODO(), sort.IsSorted(s), "SequencedWriteBySeq not sorted")
-	})
+	if util.RaceEnabled {
+		if !sort.IsSorted(s) {
+			panic("SequencedWriteBySeq must be sorted")
+		}
+	}
 	if i := sort.Search(len(s), func(i int) bool {
 		return s[i].Sequence >= seq
 	}); i < len(s) && s[i].Sequence == seq {

diff --git a/pkg/testutils/lint/gcassert_paths.txt b/pkg/testutils/lint/gcassert_paths.txt
@@ -29,4 +29,3 @@ util
 util/admission
 util/hlc
 util/intsets
-util/must
diff --git a/pkg/testutils/lint/passes/fmtsafe/functions.go b/pkg/testutils/lint/passes/fmtsafe/functions.go
@@ -162,35 +162,6 @@ var requireConstFmt = map[string]bool{
 
 	"(github.com/cockroachdb/cockroach/pkg/kv/kvpb.TestPrinter).Printf": true,
 
-	// must assertions.
-	"github.com/cockroachdb/cockroach/pkg/util/must.Fail":           true,
-	"github.com/cockroachdb/cockroach/pkg/util/must.failDepth":      true,
-	"github.com/cockroachdb/cockroach/pkg/util/must.True":           true,
-	"github.com/cockroachdb/cockroach/pkg/util/must.False":          true,
-	"github.com/cockroachdb/cockroach/pkg/util/must.Equal":          true,
-	"github.com/cockroachdb/cockroach/pkg/util/must.NotEqual":       true,
-	"github.com/cockroachdb/cockroach/pkg/util/must.Greater":        true,
-	"github.com/cockroachdb/cockroach/pkg/util/must.GreaterOrEqual": true,
-	"github.com/cockroachdb/cockroach/pkg/util/must.Less":           true,
-	"github.com/cockroachdb/cockroach/pkg/util/must.LessOrEqual":    true,
-	"github.com/cockroachdb/cockroach/pkg/util/must.EqualBytes":     true,
-	"github.com/cockroachdb/cockroach/pkg/util/must.NotEqualBytes":  true,
-	"github.com/cockroachdb/cockroach/pkg/util/must.PrefixBytes":    true,
-	"github.com/cockroachdb/cockroach/pkg/util/must.NotPrefixBytes": true,
-	"github.com/cockroachdb/cockroach/pkg/util/must.Len":            true,
-	"github.com/cockroachdb/cockroach/pkg/util/must.Contains":       true,
-	"github.com/cockroachdb/cockroach/pkg/util/must.NotContains":    true,
-	"github.com/cockroachdb/cockroach/pkg/util/must.Empty":          true,
-	"github.com/cockroachdb/cockroach/pkg/util/must.NotEmpty":       true,
-	"github.com/cockroachdb/cockroach/pkg/util/must.Nil":            true,
-	"github.com/cockroachdb/cockroach/pkg/util/must.NotNil":         true,
-	"github.com/cockroachdb/cockroach/pkg/util/must.Same":           true,
-	"github.com/cockroachdb/cockroach/pkg/util/must.NotSame":        true,
-	"github.com/cockroachdb/cockroach/pkg/util/must.Zero":           true,
-	"github.com/cockroachdb/cockroach/pkg/util/must.NotZero":        true,
-	"github.com/cockroachdb/cockroach/pkg/util/must.Error":          true,
-	"github.com/cockroachdb/cockroach/pkg/util/must.NoError":        true,
-
 	// Error things are populated in the init() message.
 }
 

diff --git a/pkg/util/log/logcrash/BUILD.bazel b/pkg/util/log/logcrash/BUILD.bazel
@@ -14,7 +14,6 @@ go_library(
         "//pkg/util/envutil",
         "//pkg/util/log",
         "//pkg/util/log/severity",
-        "//pkg/util/must",
         "//pkg/util/timeutil",
         "@com_github_cockroachdb_errors//:errors",
         "@com_github_getsentry_sentry_go//:sentry-go",