perf: use fmt.Fprintf to avoid unnecessary string+args + WriteString #3434

odeke-em · 2025-01-01T23:27:10Z

In a bid to remove unnecessary CPU and memory bloat for the gnovm which takes the order of minutes to run certain code, I noticed the pattern:

io.StringWriter.WriteString(fmt.Sprintf(...))

in which fmt.Sprintf(...) has to create a string by inserting all arguments into the format specifiers then pass that into .WriteString which defeats the entire purpose of io.StringWriter.WriteString that *bytes.Buffer and *strings.Builder implement.

Just from picking a single benchmark that already exists results in improvements in all dimensions:

name               old time/op    new time/op    delta
StringLargeData-8    8.87ms ± 1%    8.28ms ± 3%   -6.68%  (p=0.000 n=17+19)

name               old alloc/op   new alloc/op   delta
StringLargeData-8    8.44MB ± 0%    7.78MB ± 0%   -7.75%  (p=0.000 n=20+19)

name               old allocs/op  new allocs/op  delta
StringLargeData-8     94.1k ± 0%     70.1k ± 0%  -25.51%  (p=0.000 n=15+20)

for heavily used code this is going to reduce on garbage collection cycles too.

Fixes #3433

Gno2D2 · 2025-01-01T23:28:22Z

🛠 PR Checks Summary

All Automated Checks passed. ✅

Manual Checks (for Reviewers):

IGNORE the bot requirements for this PR (force green CI check)
The pull request description provides enough details

✅ Automated Checks (for Contributors):

🟢 Maintainers must be able to edit this pull request (more info)

☑️ Contributor Actions:

Fix any issues flagged by automated checks.
Follow the Contributor Checklist to ensure your PR is ready for review.
- Add new tests, or document why they are unnecessary.
- Provide clear examples/screenshots, if necessary.
- Update documentation, if required.
- Ensure no breaking changes, or include BREAKING CHANGE notes.
- Link related issues/PRs, where applicable.

☑️ Reviewer Actions:

Complete manual checks for the PR, including the guidelines and additional checks if applicable.

📚 Resources:

Debug

Automated Checks
Maintainers must be able to edit this pull request (more info)
If
🟢 Condition met
└── 🟢 The pull request was created from a fork (head branch repo: odeke-em/gno)
Then
🟢 Requirement satisfied
└── 🟢 Maintainer can modify this pull request
Manual Checks
**IGNORE** the bot requirements for this PR (force green CI check)
If
🟢 Condition met
└── 🟢 On every pull request
Can be checked by

Any user with comment edit permission
The pull request description provides enough details
If
🟢 Condition met
└── 🟢 Not (🔴 Pull request author is a member of the team: core-contributors)
Can be checked by

team core-contributors

codecov · 2025-01-01T23:30:02Z

Codecov Report

Attention: Patch coverage is 48.27586% with 15 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
tm2/pkg/sdk/auth/params.go	0.00%	9 Missing ⚠️
gnovm/pkg/gnolang/machine.go	77.77%	4 Missing ⚠️
tm2/pkg/bft/rpc/lib/server/handlers.go	0.00%	2 Missing ⚠️

📢 Thoughts on this report? Let us know!

notJoon

more cleaner than previous version. I left some comments

notJoon · 2025-01-02T01:41:40Z

gnovm/pkg/gnolang/machine.go

@@ -2131,25 +2131,25 @@ func (m *Machine) String() string {
 		totalLength = vsLength + ssLength + xsLength + bsLength + obsLength + fsLength + exceptionsLength
 	)

-	var builder strings.Builder
+	builder := new(strings.Builder)


strings.Builder is a value type, so you don't need to create it using the new keyword. Using new will allocate memory on the heap, which may result in another unnecessary memory allocation.

Not really, builder doesn't escape the heap. Go isn't like traditional languages where invoking "new" automatically allocates on the heap; escape analysis determines that and you can verifiably check that it doesn't escape and also run benchmarks to see that it doesn't escape using -gcflags='-l -m'. I can however bring it to a much better compromise of

var sb strings.Builder builder := &sb

which is a much better compromise than

fmt.Fprintf(&sb, ...)

in every spot and materially even faster.

Oh I didn't know that flag option has exists. Then this seems to have no problem at all. 👍

gnovm/pkg/gnolang/machine.go

odeke-em · 2025-01-02T03:06:30Z

Thank you for the code review and discourse @notJoon, I've made an update, kindly please help me take another stab.

notJoon

LGTM 💯

In a bid to remove unnecessary CPU and memory bloat for the gnovm which takes the order of minutes to run certain code, I noticed the pattern: io.StringWriter.WriteString(fmt.Sprintf(...)) in which fmt.Sprintf(...) has to create a string by inserting all arguments into the format specifiers then pass that into .WriteString which defeats the entire purpose of io.StringWriter.WriteString that *bytes.Buffer and *strings.Builder implement. Just from picking a single benchmark that already exists results in improvements in all dimensions: ```shell $ benchstat before.txt after.txt name old time/op new time/op delta StringLargeData-8 8.87ms ± 1% 8.28ms ± 3% -6.68% (p=0.000 n=17+19) name old alloc/op new alloc/op delta StringLargeData-8 8.44MB ± 0% 7.78MB ± 0% -7.75% (p=0.000 n=20+19) name old allocs/op new allocs/op delta StringLargeData-8 94.1k ± 0% 70.1k ± 0% -25.51% (p=0.000 n=15+20) ``` for heavily used code this is going to reduce on garbage collection cycles too. Fixes gnolang#3433

github-actions bot assigned odeke-em Jan 1, 2025

github-actions bot added 📦 🤖 gnovm Issues or PRs gnovm related 📦 🌐 tendermint v2 Issues or PRs tm2 related labels Jan 1, 2025

notJoon suggested changes Jan 2, 2025

View reviewed changes

odeke-em force-pushed the all-use-fmt.Fprintf-to-avoid-raw-string-concatentation+io.StringWriter.WriteString branch from ff8f2c3 to 932a525 Compare January 2, 2025 03:04

odeke-em requested a review from notJoon January 2, 2025 03:06

notJoon approved these changes Jan 2, 2025

View reviewed changes

odeke-em force-pushed the all-use-fmt.Fprintf-to-avoid-raw-string-concatentation+io.StringWriter.WriteString branch from 932a525 to 902ba09 Compare January 2, 2025 03:12

odeke-em force-pushed the all-use-fmt.Fprintf-to-avoid-raw-string-concatentation+io.StringWriter.WriteString branch from 902ba09 to 6cd03c2 Compare January 2, 2025 03:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf: use fmt.Fprintf to avoid unnecessary string+args + WriteString #3434

perf: use fmt.Fprintf to avoid unnecessary string+args + WriteString #3434

odeke-em commented Jan 1, 2025 •

edited

Loading

Gno2D2 commented Jan 1, 2025 •

edited

Loading

✅ Automated Checks (for Contributors):

☑️ Contributor Actions:

☑️ Reviewer Actions:

📚 Resources:

If

Then

If

Can be checked by

If

Can be checked by

codecov bot commented Jan 1, 2025 •

edited

Loading

notJoon left a comment

notJoon Jan 2, 2025 •

edited

Loading

odeke-em Jan 2, 2025

notJoon Jan 2, 2025

odeke-em commented Jan 2, 2025

notJoon left a comment

perf: use fmt.Fprintf to avoid unnecessary string+args + WriteString #3434

Are you sure you want to change the base?

perf: use fmt.Fprintf to avoid unnecessary string+args + WriteString #3434

Conversation

odeke-em commented Jan 1, 2025 • edited Loading

Gno2D2 commented Jan 1, 2025 • edited Loading

🛠 PR Checks Summary

Manual Checks (for Reviewers):

✅ Automated Checks (for Contributors):

☑️ Contributor Actions:

☑️ Reviewer Actions:

📚 Resources:

If

Then

If

Can be checked by

If

Can be checked by

codecov bot commented Jan 1, 2025 • edited Loading

Codecov Report

notJoon left a comment

Choose a reason for hiding this comment

notJoon Jan 2, 2025 • edited Loading

Choose a reason for hiding this comment

odeke-em Jan 2, 2025

Choose a reason for hiding this comment

notJoon Jan 2, 2025

Choose a reason for hiding this comment

odeke-em commented Jan 2, 2025

notJoon left a comment

Choose a reason for hiding this comment

odeke-em commented Jan 1, 2025 •

edited

Loading

Gno2D2 commented Jan 1, 2025 •

edited

Loading

codecov bot commented Jan 1, 2025 •

edited

Loading

notJoon Jan 2, 2025 •

edited

Loading