refactor: use strings.Builder to improve performance#458
refactor: use strings.Builder to improve performance#458zjumathcode wants to merge 1 commit intoinitia-labs:mainfrom
Conversation
Signed-off-by: zjumathcode <pai314159@2980.com>
📝 WalkthroughWalkthroughRefactored string assembly across five files from direct concatenation to Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
I wrote a benchmark that shows the performance advantage of using strings.Builder, as follows: package bench_test
import (
"fmt"
"strings"
"testing"
)
type Route struct {
Group string
MatcherSetsRaw string
HandlersRaw []string
Terminal bool
}
func (r Route) StringOld() string {
handlersRaw := "["
for _, hr := range r.HandlersRaw {
handlersRaw += " " + string(hr)
}
handlersRaw += "]"
return fmt.Sprintf(`{Group:"%s" MatcherSetsRaw:%s HandlersRaw:%s Terminal:%t}`,
r.Group, r.MatcherSetsRaw, handlersRaw, r.Terminal)
}
func (r Route) StringNew() string {
var handlersRaw strings.Builder
handlersRaw.WriteString("[")
for _, hr := range r.HandlersRaw {
handlersRaw.WriteString(" " + string(hr))
}
handlersRaw.WriteString("]")
return fmt.Sprintf(`{Group:"%s" MatcherSetsRaw:%s HandlersRaw:%s Terminal:%t}`,
r.Group, r.MatcherSetsRaw, handlersRaw.String(), r.Terminal)
}
func BenchmarkRouteString_Old(b *testing.B) {
r := Route{
Group: "api",
MatcherSetsRaw: "[matcher]",
HandlersRaw: []string{"foo", "bar", "baz", "qux", "quux"},
Terminal: true,
}
b.ReportAllocs()
for i := 0; i < b.N; i++ {
_ = r.StringOld()
}
}
func BenchmarkRouteString_New(b *testing.B) {
r := Route{
Group: "api",
MatcherSetsRaw: "[matcher]",
HandlersRaw: []string{"foo", "bar", "baz", "qux", "quux"},
Terminal: true,
}
b.ReportAllocs()
for i := 0; i < b.N; i++ {
_ = r.StringNew()
}
}➜ benchmark go test -bench=. -benchmem
goos: darwin
goarch: arm64
pkg: demo/stringsbuilder/benchmark
cpu: Apple M4
BenchmarkRouteString_Old-10 5239779 211.3 ns/op 248 B/op 10 allocs/op
BenchmarkRouteString_New-10 6258066 193.5 ns/op 200 B/op 7 allocs/op
PASS
ok demo/stringsbuilder/benchmark 3.482s |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
x/distribution/types/validator.go (1)
39-49: ValidatorSlashEvents.String builder usage looks good; optional micro‑optimizations possibleThe refactor to
strings.Builderpreserves the existing output shape (header + per‑slash blocks, trailing whitespace trimmed) and avoids repeated concatenation. If you want to push this a bit further, you could skip the intermediate allocations fromfmt.Sprintfby writing directly into the builder, e.g.:fmt.Fprintf(&out, " Slash %d:\n Period: %d\n Fractions: %s\n", i, sl.ValidatorPeriod, sl.Fractions)This is purely optional given how infrequently this Stringer is likely called.
x/gov/types/proposal.go (1)
86-95: Proposals.String refactor is fine; consider minor cleanupsThe switch to
strings.Builderhere is straightforward and should be behavior‑preserving (same lines, trailing newline trimmed). Two optional tweaks if you touch this again:
- Avoid the intermediate allocation with
fmt.Sprintfby usingfmt.Fprintf(&out, "%d - %s\n", prop.Id, prop.Status).- Either extend the per‑proposal line to include
[Type] Titleor adjust the header, since the current header columns and body don’t match.Both are cosmetic; the refactor itself looks correct.
x/mstaking/keeper/invariants.go (2)
145-174: PositiveDelegationInvariant message building is correct; builder use is fineSwitching
msgfromstringtostrings.Builderand then usingmsg.String()in the finalfmt.Sprintfkeeps the invariant output the same while avoiding repeated string concatenation in the loop. Given how rarely invariants run, this is already more than sufficient; if you ever want to shave a bit more, you could do:fmt.Fprintf(&msg, "\tdelegation with negative shares: %+v\n", delegation)to avoid the intermediate
fmt.Sprintfallocation. Optional only.
181-223: DelegatorSharesInvariant builder refactor looks good; optional direct writesThe
strings.Builderuse here is also semantically equivalent to the oldmsg += fmt.Sprintf(...)pattern, and the finalFormatInvariantcall just consumesmsg.String()as expected.As above, you could switch to:
fmt.Fprintf(&msg, "broken delegator shares invariance:\n\tvalidator.DelegatorShares: %v\n\tsum of Delegator.Shares: %v\n", expValTotalDelShares, calculatedValTotalDelShares)to skip an extra allocation, but that’s purely a readability/perf nicety.
x/mstaking/types/delegation.go (1)
290-315: Redelegation.String builder usage and trimming look correctThe new
strings.Builderimplementation forRedelegation.Stringkeeps the same structure as before:
- Header with delegator/src/dst and an
Entries:section plus blank line.- One multi-line block per entry, each ending in
\n.- Final
strings.TrimRight(out.String(), "\n")to drop the trailing newline while preserving other whitespace.That’s a solid pattern. As with other sites, you can optionally switch to
fmt.Fprintf(&out, ...)to avoid intermediatefmt.Sprintfstrings, but it’s not necessary for clarity or correctness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (5)
x/distribution/types/validator.go(1 hunks)x/gov/keeper/proposal.go(3 hunks)x/gov/types/proposal.go(1 hunks)x/mstaking/keeper/invariants.go(5 hunks)x/mstaking/types/delegation.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
x/mstaking/keeper/invariants.go (1)
x/mstaking/types/keys.go (1)
ModuleName(5-5)
🔇 Additional comments (2)
x/mstaking/types/delegation.go (1)
185-203: Review comment is based on incorrect code snapshotThe review references a
strings.Builderimplementation and provides a diff for it, but the actual code inx/mstaking/types/delegation.go(lines 186-202) uses simple string concatenation with+=, notstrings.Builder.While the underlying formatting concern is valid—
UnbondingDelegation.Stringdoes lack newlines between entries and will produce output like"Entries: Unbonding Delegation 0:"on a single line—the code snippet and diff in the review do not match the current implementation and cannot be applied.The comparison with
Redelegation.String(lines 290-313) is accurate:Redelegation.Stringproperly ends raw strings with newlines and usesstrings.TrimRight(out, "\n")at the end, whileUnbondingDelegation.Stringdoes not.Clarify whether this review is for a pending PR or whether the strings.Builder refactor was already applied, as the current code differs from what the review describes.
Likely an incorrect or invalid review comment.
x/gov/keeper/proposal.go (1)
42-49: Code review is based on outdated code snapshotThe review comment references code using
strings.BuilderandmsgsStr.WriteString(), but the current version ofx/gov/keeper/proposal.go(lines 42–47) uses simple string concatenation with the+=operator:msgsStr := "" // ... msgsStr += fmt.Sprintf(",%s", sdk.MsgTypeURL(msg))The strings.Builder refactoring mentioned in the review does not appear to have been applied. The underlying concern about the leading comma remains valid for the current code, but cannot be addressed using the exact diff provided since it targets non-existent code.
Likely an incorrect or invalid review comment.
|
Thanks for PR, but we are not receiving this kind of maintain PRs. |
2 participants
Description
strings.Builder has fewer memory allocations and better performance.
More info: golang/go#75190
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...