Skip to content

fix: improve getMetricName performance#3156

Merged
dnwe merged 2 commits intoIBM:mainfrom
boekkooi-impossiblecloud:rm-fmt
May 20, 2025
Merged

fix: improve getMetricName performance#3156
dnwe merged 2 commits intoIBM:mainfrom
boekkooi-impossiblecloud:rm-fmt

Conversation

@boekkooi-impossiblecloud
Copy link
Copy Markdown
Contributor

@boekkooi-impossiblecloud boekkooi-impossiblecloud commented May 13, 2025

Good day,

This PR improve the performance of getMetricNameForBroker and getMetricNameForTopic by removing the usage of fmt.

The benchstat result are as follows. The benchmarks when run using go test -bench '^\QBenchmark_getMetricNameFor' -run=^$ -count=10.

goos: linux
goarch: amd64
pkg: github.com/IBM/sarama
cpu: AMD Ryzen 7 Pro 7735U with Radeon Graphics     
                           │ faec4c823ff893c7fc7b10021b66542fa77f7cb8.txt │ 07db3539fb81f1bf77d8a8a910b2b1e72b4c52c3.txt │
                           │                    sec/op                    │        sec/op         vs base                │
_getMetricNameForTopic-16                                    157.55n ± 0%            66.85n ± 1%  -57.57% (p=0.000 n=10)
_getMetricNameForBroker-16                                   108.50n ± 1%            43.12n ± 1%  -60.25% (p=0.000 n=10)
geomean                                                       130.7n                 53.69n       -58.93%

                           │ faec4c823ff893c7fc7b10021b66542fa77f7cb8.txt │ 07db3539fb81f1bf77d8a8a910b2b1e72b4c52c3.txt │
                           │                     B/op                     │         B/op          vs base                │
_getMetricNameForTopic-16                                      64.00 ± 0%             16.00 ± 0%  -75.00% (p=0.000 n=10)
_getMetricNameForBroker-16                                    28.000 ± 0%             4.000 ± 0%  -85.71% (p=0.000 n=10)
geomean                                                        42.33                  8.000       -81.10%

                           │ faec4c823ff893c7fc7b10021b66542fa77f7cb8.txt │ 07db3539fb81f1bf77d8a8a910b2b1e72b4c52c3.txt │
                           │                  allocs/op                   │      allocs/op        vs base                │
_getMetricNameForTopic-16                                      3.000 ± 0%             1.000 ± 0%  -66.67% (p=0.000 n=10)
_getMetricNameForBroker-16                                     2.000 ± 0%             1.000 ± 0%  -50.00% (p=0.000 n=10)
geomean                                                        2.449                  1.000       -59.18%

Thank you for reviewing this PR and let me know if anything needs to change.
Have a great day!

@dnwe
Copy link
Copy Markdown
Collaborator

dnwe commented May 13, 2025

Thanks @boekkooi-impossiblecloud – the change and benchmarked improvements look good to me, please can you amend your commits to add a DCO signoff to get that check to pass? See here

@boekkooi-impossiblecloud
Copy link
Copy Markdown
Contributor Author

boekkooi-impossiblecloud commented May 13, 2025

Thanks @boekkooi-impossiblecloud – the change and benchmarked improvements look good to me, please can you amend your commits to add a DCO signoff to get that check to pass? See here

Just did. Thanks for the quick review/response 😄

puellanivis
puellanivis reviewed May 14, 2025
View reviewed changes
puellanivis
puellanivis reviewed May 20, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@puellanivis puellanivis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgot to point out. Looks good to me, and thanks for the benchmark of the alternative. 👍

@dnwe dnwe added the fix label May 20, 2025
@dnwe dnwe changed the title improve getMetricName performance fix: improve getMetricName performance May 20, 2025
dnwe
dnwe approved these changes May 20, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@dnwe dnwe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Great PR

@dnwe dnwe merged commit 876967c into IBM:main May 20, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@puellanivis puellanivis puellanivis left review comments

@dnwe dnwe dnwe approved these changes

Assignees

No one assigned

Labels

fix

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@boekkooi-impossiblecloud @dnwe @puellanivis