From abe3e0c6da0233788506a5789fa7b4c475a18d7b Mon Sep 17 00:00:00 2001 From: Emmanuel T Odeke Date: Fri, 9 Sep 2022 14:57:46 -0700 Subject: [PATCH] perf: all: remove unnecessary allocations from strings.Builder.WriteString(fmt.Sprintf(...)) (#13230) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This change removes a code pattern that I noticed while on a late night audit of cosmovisor in which strings.Builder.WriteString(fmt.Sprintf(...)) calls were being made, yet that's counterproductive to using fmt.Fprintf which will check whether the writer implements .WriteString and then avoids the need to firstly build a string using fmt.Sprintf. The performance wins from this change transcend all dimensions as exhibited below: ```shell $ benchstat before.txt after.txt name old time/op new time/op delta DetailString-8 5.48µs ±23% 4.40µs ±11% -19.79% (p=0.000 n=20+17) name old alloc/op new alloc/op delta DetailString-8 2.63kB ± 0% 2.11kB ± 0% -19.76% (p=0.000 n=20+20) name old allocs/op new allocs/op delta DetailString-8 63.0 ± 0% 50.0 ± 0% -20.63% (p=0.000 n=20+20) ``` Fixes #13229 --- client/rpc/validators.go | 10 ++++------ cosmovisor/args.go | 4 ++-- cosmovisor/args_test.go | 26 ++++++++++++++++++++++++++ cosmovisor/errors/multi.go | 4 ++-- server/util_test.go | 6 +++--- types/events.go | 4 ++-- x/distribution/types/proposal.go | 4 ++-- x/params/types/proposal/proposal.go | 8 ++++---- 8 files changed, 45 insertions(+), 21 deletions(-) diff --git a/client/rpc/validators.go b/client/rpc/validators.go index 1fb7e796d2..76a545d80b 100644 --- a/client/rpc/validators.go +++ b/client/rpc/validators.go @@ -84,19 +84,17 @@ type ResultValidatorsOutput struct { func (rvo ResultValidatorsOutput) String() string { var b strings.Builder - b.WriteString(fmt.Sprintf("block height: %d\n", rvo.BlockHeight)) - b.WriteString(fmt.Sprintf("total count: %d\n", rvo.Total)) + fmt.Fprintf(&b, "block height: %d\n", rvo.BlockHeight) + fmt.Fprintf(&b, "total count: %d\n", rvo.Total) for _, val := range rvo.Validators { - b.WriteString( - fmt.Sprintf(` + fmt.Fprintf(&b, ` Address: %s Pubkey: %s ProposerPriority: %d VotingPower: %d `, - val.Address, val.PubKey, val.ProposerPriority, val.VotingPower, - ), + val.Address, val.PubKey, val.ProposerPriority, val.VotingPower, ) } diff --git a/cosmovisor/args.go b/cosmovisor/args.go index 229d5336b2..59345705df 100644 --- a/cosmovisor/args.go +++ b/cosmovisor/args.go @@ -379,7 +379,7 @@ func (cfg Config) DetailString() string { var sb strings.Builder sb.WriteString("Configurable Values:\n") for _, kv := range configEntries { - sb.WriteString(fmt.Sprintf(" %s: %s\n", kv.name, kv.value)) + fmt.Fprintf(&sb, " %s: %s\n", kv.name, kv.value) } sb.WriteString("Derived Values:\n") dnl := 0 @@ -390,7 +390,7 @@ func (cfg Config) DetailString() string { } dFmt := fmt.Sprintf(" %%%ds: %%s\n", dnl) for _, kv := range derivedEntries { - sb.WriteString(fmt.Sprintf(dFmt, kv.name, kv.value)) + fmt.Fprintf(&sb, dFmt, kv.name, kv.value) } return sb.String() } diff --git a/cosmovisor/args_test.go b/cosmovisor/args_test.go index a8bec5bb2e..9b17fe449b 100644 --- a/cosmovisor/args_test.go +++ b/cosmovisor/args_test.go @@ -689,3 +689,29 @@ func (s *argsTestSuite) TestLogConfigOrError() { }) } } + +var sink interface{} + +func BenchmarkDetailString(b *testing.B) { + cfg := &Config{ + Home: "/foo", Name: "myd", + AllowDownloadBinaries: true, + UnsafeSkipBackup: true, + PollInterval: 450 * time.Second, + PreupgradeMaxRetries: 1e7, + } + + b.ReportAllocs() + b.ResetTimer() + + for i := 0; i < b.N; i++ { + sink = cfg.DetailString() + } + + if sink == nil { + b.Fatal("Benchmark did not run") + } + + // Otherwise reset the sink. + sink = (interface{})(nil) +} diff --git a/cosmovisor/errors/multi.go b/cosmovisor/errors/multi.go index a78a61a4e8..297cfdf1f4 100644 --- a/cosmovisor/errors/multi.go +++ b/cosmovisor/errors/multi.go @@ -55,12 +55,12 @@ func (e MultiError) Len() int { // Error implements the error interface for a MultiError. func (e *MultiError) Error() string { var sb strings.Builder - sb.WriteString(fmt.Sprintf("%d errors: ", len(e.errs))) + fmt.Fprintf(&sb, "%d errors: ", len(e.errs)) for i, err := range e.errs { if i != 0 { sb.WriteString(", ") } - sb.WriteString(fmt.Sprintf("%d: %v", i+1, err)) + fmt.Fprintf(&sb, "%d: %v", i+1, err) } return sb.String() } diff --git a/server/util_test.go b/server/util_test.go index 6798d556eb..eb4ec6fb2d 100644 --- a/server/util_test.go +++ b/server/util_test.go @@ -107,7 +107,7 @@ func TestInterceptConfigsPreRunHandlerReadsConfigToml(t *testing.T) { t.Fatalf("creating config.toml file failed: %v", err) } - _, err = writer.WriteString(fmt.Sprintf("db_backend = '%s'\n", testDbBackend)) + _, err = fmt.Fprintf(writer, "db_backend = '%s'\n", testDbBackend) if err != nil { t.Fatalf("Failed writing string to config.toml: %v", err) } @@ -148,7 +148,7 @@ func TestInterceptConfigsPreRunHandlerReadsAppToml(t *testing.T) { t.Fatalf("creating app.toml file failed: %v", err) } - _, err = writer.WriteString(fmt.Sprintf("halt-time = %d\n", testHaltTime)) + _, err = fmt.Fprintf(writer, "halt-time = %d\n", testHaltTime) if err != nil { t.Fatalf("Failed writing string to app.toml: %v", err) } @@ -312,7 +312,7 @@ func (v precedenceCommon) setAll(t *testing.T, setFlag *string, setEnvVar *strin t.Fatalf("creating config.toml file failed: %v", err) } - _, err = writer.WriteString(fmt.Sprintf("[rpc]\nladdr = \"%s\"\n", *setConfigFile)) + _, err = fmt.Fprintf(writer, "[rpc]\nladdr = \"%s\"\n", *setConfigFile) if err != nil { t.Fatalf("Failed writing string to config.toml: %v", err) } diff --git a/types/events.go b/types/events.go index 37f8f99608..3415927516 100644 --- a/types/events.go +++ b/types/events.go @@ -244,10 +244,10 @@ func (se StringEvents) String() string { var sb strings.Builder for _, e := range se { - sb.WriteString(fmt.Sprintf("\t\t- %s\n", e.Type)) + fmt.Fprintf(&sb, "\t\t- %s\n", e.Type) for _, attr := range e.Attributes { - sb.WriteString(fmt.Sprintf("\t\t\t- %s\n", attr.String())) + fmt.Fprintf(&sb, "\t\t\t- %s\n", attr) } } diff --git a/x/distribution/types/proposal.go b/x/distribution/types/proposal.go index a78ab62127..925738d308 100644 --- a/x/distribution/types/proposal.go +++ b/x/distribution/types/proposal.go @@ -8,11 +8,11 @@ import ( // String implements the Stringer interface. func (csp CommunityPoolSpendProposal) String() string { var b strings.Builder - b.WriteString(fmt.Sprintf(`Community Pool Spend Proposal: + fmt.Fprintf(&b, `Community Pool Spend Proposal: Title: %s Description: %s Recipient: %s Amount: %s -`, csp.Title, csp.Description, csp.Recipient, csp.Amount)) +`, csp.Title, csp.Description, csp.Recipient, csp.Amount) return b.String() } diff --git a/x/params/types/proposal/proposal.go b/x/params/types/proposal/proposal.go index 55c65afa4e..c1cbcdd992 100644 --- a/x/params/types/proposal/proposal.go +++ b/x/params/types/proposal/proposal.go @@ -51,18 +51,18 @@ func (pcp *ParameterChangeProposal) ValidateBasic() error { func (pcp ParameterChangeProposal) String() string { var b strings.Builder - b.WriteString(fmt.Sprintf(`Parameter Change Proposal: + fmt.Fprintf(&b, `Parameter Change Proposal: Title: %s Description: %s Changes: -`, pcp.Title, pcp.Description)) +`, pcp.Title, pcp.Description) for _, pc := range pcp.Changes { - b.WriteString(fmt.Sprintf(` Param Change: + fmt.Fprintf(&b, ` Param Change: Subspace: %s Key: %s Value: %X -`, pc.Subspace, pc.Key, pc.Value)) +`, pc.Subspace, pc.Key, pc.Value) } return b.String()