fmt: reduce Errorf("x") allocations to match errors.New("x")

For unformatted strings, it comes up periodically that there are
more allocations using fmt.Errorf("x") compared to errors.New("x").
People cite it as a reason to switch code using fmt.Errorf to
use errors.New instead.

Three examples from the last few weeks essentially made
this suggestion: #75235, CL 708496, and CL 708618. Prior to that,
it is periodically suggested as a vet check (e.g., proposals #17173
and #52696) or in various CLs to change the standard library
(e.g., CL 403938 and CL 588776).

On the other hand, I believe the position of the core Go team
is that it is usually not worthwhile to make such a change. For example,
in #52696, Russ wrote:

  Thanks for raising the issue, but please don't do this. Using
  fmt.Errorf("foo") is completely fine, especially in a program where
  all the errors are constructed with fmt.Errorf. Having to
  mentally switch between two functions based on the argument
  is unnecessary noise.

This CL attempts to mostly take performance out of the discussion.

We drop from 2 allocations to 0 allocations for a non-escaping error,
and drop from 2 allocations to 1 allocation for an escaping error:

  _ = fmt.Errorf("foo")    // non-escaping error
  sink = fmt.Errorf("foo") // escaping error

This now matches the allocations for errors.New("foo") in both cases.

The CPU cost difference is greatly reduced, though there is still
a small ~4ns difference measurable in these microbenchmarks. Previously,
it was ~64ns vs. ~21ns for fmt.Errorf("x") vs. errors.New("x")
for escaping errors, whereas with this CL it is now ~25ns vs. ~21ns.

When fmt.Errorf("foo") executes with this CL, there are essentially
three optimizations now, in rough order of usefulness:
 (1) we always avoid an allocation inside the doPrintf machinery;
 (2) if the error does not otherwise escape, we can stack allocate
     the errors.errorString struct by virtue of mid-stack inlining
     of fmt.Errorf and the resulting inlining of errors.New, which
     also can be more effective via PGO;
 (3) stringslite.IndexByte is a tiny bit faster than going through the
     for loops looking for '%' inside doPrintf.

See https://blog.filippo.io/efficient-go-apis-with-the-inliner/ for
background on avoiding heap allocations via mid-stack inlining.

The common case here is likely that the string format argument is a
constant when there are no other arguments.

However, one concern could be that by not allocating a copy, we could
now keep a string argument alive longer with this change, which could
be a pessimization if for example that string argument is a
slice of a much bigger string:

  s := bigString[m:n]
  longLivedErr := fmt.Errorf(s)

Aside from that being perhaps unusual code, vet will complain about
s there as a "non-constant format string in call to fmt.Errorf", so that
particular example seems unlikely to occur frequently in practice.

The main benchmark results are below. "old" is prior to this CL, "new"
is with this CL. The non-escaping case is "local", the escaping case is
"sink". In practice, I suspect errors escape the majority of the time.
Benchmark code at https://go.dev/play/p/rlRSO1ehx8O

goos: linux
goarch: amd64
pkg: fmt
cpu: AMD EPYC 7B13
                         │  old-7bd6fac4.txt      │           new-dcd2a72f0.txt    │
                         │      sec/op            │   sec/op     vs base           │
Errorf/no-args/local-16              63.76n ± 1%   4.874n ± 0%  -92.36% (n=120)
Errorf/no-args/sink-16               64.25n ± 1%   25.81n ± 0%  -59.83% (n=120)
Errorf/int-arg/local-16              90.86n ± 1%   90.97n ± 1%        ~ (p=0.713 n=120)
Errorf/int-arg/sink-16               91.81n ± 1%   91.10n ± 1%   -0.76% (p=0.036 n=120)
geomean                              76.46n        31.95n       -58.20%

                         │  old-7bd6fac4.txt      │             new-dcd2a72f0.txt  │
                         │       B/op             │    B/op     vs base            │
Errorf/no-args/local-16               19.00 ± 0%    0.00 ± 0%  -100.00% (n=120)
Errorf/no-args/sink-16                19.00 ± 0%   16.00 ± 0%   -15.79% (n=120)
Errorf/int-arg/local-16               24.00 ± 0%   24.00 ± 0%         ~ (p=1.000 n=120) ¹
Errorf/int-arg/sink-16                24.00 ± 0%   24.00 ± 0%         ~ (p=1.000 n=120) ¹
geomean                               21.35                    ?                        ² ³
¹ all samples are equal

                         │  old-7bd6fac4.txt      │       new-dcd2a72f0.txt        │
                         │     allocs/op          │ allocs/op   vs base            │
Errorf/no-args/local-16               2.000 ± 0%   0.000 ± 0%  -100.00% (n=120)
Errorf/no-args/sink-16                2.000 ± 0%   1.000 ± 0%   -50.00% (n=120)
Errorf/int-arg/local-16               2.000 ± 0%   2.000 ± 0%         ~ (p=1.000 n=120) ¹
Errorf/int-arg/sink-16                2.000 ± 0%   2.000 ± 0%         ~ (p=1.000 n=120) ¹
geomean                               2.000                    ?                        ² ³
¹ all samples are equal

Change-Id: Ib27c52933bec5c2236624c577fbb1741052e792f
Reviewed-on: https://go-review.googlesource.com/c/go/+/708836
Reviewed-by: Damien Neil <dneil@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Commit-Queue: t hepudds <thepudds1460@gmail.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Reviewed-by: Emmanuel Odeke <emmanuel@orijtech.com>

diff --git a/src/fmt/errors.go b/src/fmt/errors.go
index 1ac83404bc7c55785fe34ca51c0b415ed77521fd..a0ce7ada346dbc0dbb122e1c5006c1ab89bb1e69 100644 (file)
--- a/src/fmt/errors.go
+++ b/src/fmt/errors.go
@@ -6,6 +6,7 @@ package fmt
 
 import (
        "errors"
+       "internal/stringslite"
        "slices"
 )
 
@@ -19,7 +20,22 @@ import (
 // order they appear in the arguments.
 // It is invalid to supply the %w verb with an operand that does not implement
 // the error interface. The %w verb is otherwise a synonym for %v.
-func Errorf(format string, a ...any) error {
+func Errorf(format string, a ...any) (err error) {
+       // This function has been split in a somewhat unnatural way
+       // so that both it and the errors.New call can be inlined.
+       if err = errorf(format, a...); err != nil {
+               return err
+       }
+       // No formatting was needed. We can avoid some allocations and other work.
+       // See https://go.dev/cl/708836 for details.
+       return errors.New(format)
+}
+
+// errorf formats and returns an error value, or nil if no formatting is required.
+func errorf(format string, a ...any) error {
+       if len(a) == 0 && stringslite.IndexByte(format, '%') == -1 {
+               return nil
+       }
        p := newPrinter()
        p.wrapErrs = true
        p.doPrintf(format, a)

diff --git a/src/fmt/errors_test.go b/src/fmt/errors_test.go
index 4eb55faffe7a181777ae1961d35f3a9f66a5042b..52bf42d0a623a682dd03fd5016f2af90a48001ba 100644 (file)
--- a/src/fmt/errors_test.go
+++ b/src/fmt/errors_test.go
@@ -54,6 +54,15 @@ func TestErrorf(t *testing.T) {
        }, {
                err:      noVetErrorf("%w is not an error", "not-an-error"),
                wantText: "%!w(string=not-an-error) is not an error",
+       }, {
+               err:      fmt.Errorf("no verbs"),
+               wantText: "no verbs",
+       }, {
+               err:      noVetErrorf("no verbs with extra arg", "extra"),
+               wantText: "no verbs with extra arg%!(EXTRA string=extra)",
+       }, {
+               err:      noVetErrorf("too many verbs: %w %v"),
+               wantText: "too many verbs: %!w(MISSING) %!v(MISSING)",
        }, {
                err:       noVetErrorf("wrapped two errors: %w %w", errString("1"), errString("2")),
                wantText:  "wrapped two errors: 1 2",

diff --git a/src/fmt/fmt_test.go b/src/fmt/fmt_test.go
index 86e458ae6481fbf68176f875e54b48595e35b3ca..c07da5683c24635d02f26ec31d61aca02b438efd 100644 (file)
--- a/src/fmt/fmt_test.go
+++ b/src/fmt/fmt_test.go
@@ -1480,6 +1480,7 @@ func BenchmarkFprintIntNoAlloc(b *testing.B) {
 
 var mallocBuf bytes.Buffer
 var mallocPointer *int // A pointer so we know the interface value won't allocate.
+var sink any
 
 var mallocTest = []struct {
        count int
@@ -1510,6 +1511,10 @@ var mallocTest = []struct {
                mallocBuf.Reset()
                Fprintf(&mallocBuf, "%x %x %x", mallocPointer, mallocPointer, mallocPointer)
        }},
+       {0, `Errorf("hello")`, func() { _ = Errorf("hello") }},
+       {2, `Errorf("hello: %x")`, func() { _ = Errorf("hello: %x", mallocPointer) }},
+       {1, `sink = Errorf("hello")`, func() { sink = Errorf("hello") }},
+       {2, `sink = Errorf("hello: %x")`, func() { sink = Errorf("hello: %x", mallocPointer) }},
 }
 
 var _ bytes.Buffer