util/must: consider performance implications

[erikgrinaker]

The must package isn't suitable for use in hot paths. The main problem is that it takes printf-style format args as []interface{}:

cockroach/pkg/util/must/must.go

Lines 265 to 275 in 3250477

	func Equal[T comparable](ctx context.Context, a, b T, format string, args ...interface{}) error {
	// TODO(erikgrinaker): Consider erroring out on pointers, if possible. It's
	// usually not what one wants, and can be a footgun.
	if a == b {
	return nil
	}
	// TODO(erikgrinaker): Consider printing a diff, or otherwise improve the
	// formatting, for large values like entire structs.
	format += ": %#v != %#v"
	return failDepth(ctx, 1, format, append(args, a, b)...)
	}

This has a couple of problems: the interface boxing incurs an allocation for certain types, and arguments can escape to the heap. One might hope that the compiler deferred the boxing until after the a == b condition when inlining, but no such luck.

Most likely, the better option is to significantly simplify the API. Something basically similar to:

if a != b {
  return must.Fail(ctx, "a=%v != b=%v", a, b)
}

This only incurs the cost of boxing on the unhappy path, and the heap escape can similarly be deferred by copying the format args value in the branch.

However, let's consider some other options here, and see if we can come up with something suitable.

Jira issue: CRDB-30342

[blathers-crl]

cc @cockroachdb/test-eng

[srosenberg]

Clarification–above concerns the case of not compiling away must.XXX in release builds. E.g., existing assertions conditional on CrdbTestBuild don't suffer since they are effectively compiled away; the same holds for assertions which are turned into no-op, at compilation time, e.g., [1].

[1] https://github.com/arl/assertgo

[erikgrinaker]

I'll point out that we only incur the boxing allocation for objects or large values, so this is still fine as long as we don't pass those. But it's easy to get wrong. We could consider adding a linter or runtime check, and a wrapper function to override the check. But it's not exactly ergonomic.

We could consider a secondary API to decorate the error, but this won't work with fatal assertions since they'll fail before:

if err := must.Equal(ctx, a, b, "format"); err != nil {
  return must.Decorate(err, "%v", arg)
}

Generics won't help us here, since variadic arguments would require the same type for all arguments in that case, and we can't be generic over fmt.Stringer since primitive types don't implement it.

func Equal[T comparable, A fmt.Stringer](ctx context.Context, a, b T, format string, args ...A) error // no bueno

Another option would be to use code generation to generate code which only incurs the boxing cost on failures. Again, not very ergonomic. We could also pass a closure to generate the format args, but that's not ergonomic either, and likely comes with its own performance penalty.

[erikgrinaker]

Clarification–above concerns the case of not compiling away must.XXX in release builds.

Well yes, but we generally want most assertions enabled in release builds too, if we can -- we need to guard against correctness violations, we want to maximize coverage across environments, and we want to be notified about violations.

This allocation would likely be enough by itself to prevent their use in certain core code paths.

[erikgrinaker]

Then again, I suppose one always has the option of manually switching to if a != b { return must.Fail() } in those hot paths. I'm just a bit concerned that people have to watch out for this, it's a bit of a footgun.

[rickystewart]

Generics won't help us here, since variadic arguments would require the same type for all arguments in that case, and we can't be generic over fmt.Stringer since primitive types don't implement it.

[Guy with no knowledge of Go generics voice] Can you define your own interface which is "either fmt.Stringer or bool or string or int8 or uint8, etc..."

[rickystewart]

Maybe the interface boxing still kicks in in that case.

The must functions could also take a single literal string instead of a format string and arguments, meaning you could proactively call fmt.Sprintf() if you need the format string behavior. I suppose that proactively calling fmt.Sprintf() would also be slow in hot paths, but presumably you just wouldn't do this in hot code paths -- and to be fair the fact that the Sprintf() is "outside of the must function" at least means it's obvious when you're doing extra work.

[erikgrinaker]

Can you define your own interface which is "either fmt.Stringer or bool or string or int8 or uint8, etc..."

No, unions currently can't contain interfaces.

Maybe the interface boxing still kicks in in that case.

I don't think it does, because we instantiate a generic function for each concrete type. However, we can't mix different concrete types, they must all have the same type because the variadic parameters have type []T, which I think is a no-go.

The must functions could also take a single literal string instead of a format string and arguments, meaning you could proactively call fmt.Sprintf() if you need the format string behavior. I suppose that proactively calling fmt.Sprintf() would also be slow in hot paths, but presumably you just wouldn't do this in hot code paths -- and to be fair the fact that the Sprintf() is "outside of the must function" at least means it's obvious when you're doing extra work.

Yeah, this would still incur the boxing, but it would also incur the substantial formatting cost even when we don't need it (the common case).

[miretskiy]

Might be best to wait for golang/go#37739

[erikgrinaker]

Closing, we'll need a different API, deferring to #94986.