-
Notifications
You must be signed in to change notification settings - Fork 388
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
perf: use fmt.Fprintf to avoid unnecessary string+args + WriteString #3434
perf: use fmt.Fprintf to avoid unnecessary string+args + WriteString #3434
Conversation
🛠 PR Checks SummaryAll Automated Checks passed. ✅ Manual Checks (for Reviewers):
Read More🤖 This bot helps streamline PR reviews by verifying automated checks and providing guidance for contributors and reviewers. ✅ Automated Checks (for Contributors):🟢 Maintainers must be able to edit this pull request (more info) ☑️ Contributor Actions:
☑️ Reviewer Actions:
📚 Resources:Debug
|
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more cleaner than previous version. I left some comments
ff8f2c3
to
932a525
Compare
Thank you for the code review and discourse @notJoon, I've made an update, kindly please help me take another stab. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 💯
902ba09
to
6cd03c2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed over video call with @odeke-em , LGTM.
bfb7245
to
f981e89
Compare
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
f981e89
to
b4f6bbb
Compare
I gave feedback to @odeke-em out-of-band. He's going to sort out the last of the failing tests and we can merge. |
…ncatentation+io.StringWriter.WriteString
…ncatentation+io.StringWriter.WriteString
…3434) 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 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 --------- Co-authored-by: Nathan Toups <[email protected]> Co-authored-by: Morgan <[email protected]>
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:
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:
for heavily used code this is going to reduce on garbage collection cycles too.
Fixes #3433