Skip to content

fix: add StatsWithBuffer method for optimized stats retrieval #106

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

Merged
merged 1 commit into from
May 29, 2025

Conversation

ritwikranjan
Copy link
Contributor

@ritwikranjan ritwikranjan commented May 7, 2025

This pull request refactors the ethtool.go file to improve code maintainability by introducing a new method for buffer management, and ensuring backward compatibility. Below are the most significant changes:

Type Renaming for Consistency:

  • Renamed ethtoolGStrings to EthtoolGStrings and ethtoolStats to EthtoolStats to align with Go naming conventions for exported types. This change affects type definitions and all instances where these types are used. [1] [2] [3]

Buffer Management Enhancements:

  • Introduced a new method StatsWithBuffer that allows pre-allocated buffers (EthtoolGStrings and EthtoolStats) to be passed in, reducing heap allocations and improving performance in Go 1.24+.

Backward Compatibility:

  • Updated the existing Stats method to use the new StatsWithBuffer internally while maintaining its original interface, ensuring backward compatibility with existing code. [1] [2] [3]

…ntain backward compatibility

Signed-off-by: Ritwik Ranjan <ritwikranjan@microsoft.com>
@ritwikranjan ritwikranjan force-pushed the fix/ethtool-memory-issue-1 branch from 06f55ef to 1723619 Compare May 22, 2025 16:51
@ritwikranjan
Copy link
Contributor Author

@safchain @halfcrazy I have rebased on top of your changes, this gives the user independence in management of the buffer memory. Let me know if it looks good to you!

@ritwikranjan
Copy link
Contributor Author

A soft reminder to look at the PR @safchain / @halfcrazy

@halfcrazy
Copy link
Contributor

/lgtm

@safchain safchain merged commit f9520b6 into safchain:master May 29, 2025
2 checks passed
@ritwikranjan
Copy link
Contributor Author

@safchain Can we make a release as well?

@safchain
Copy link
Owner

Just released the v0.6.1

@ritwikranjan
Copy link
Contributor Author

Perfect, thanks for the quick turnaround!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants