-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Allow GC to collect unneeded slice elements #5804
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5804 +/- ##
=======================================
- Coverage 84.6% 84.6% -0.1%
=======================================
Files 272 272
Lines 22890 22896 +6
=======================================
+ Hits 19382 19387 +5
- Misses 3164 3165 +1
Partials 344 344 |
Do any of the existing benchmark show the performance improvement? If so then it would be good to add |
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.
Missing changelog entry.
The PR description should also have a benchstat
output showcasing the improvement.
No, doesn't look like it. I've added output of benchmarks for the packages I edited to the description. I don't know, maybe there are benchmarks in other packages that would show the difference (unlikely?).
I'm sorry, but I don't have enough time to make significant changes in this PR. Lots of other things to do... I've added a changelog entry. |
ed76b1d
to
f434388
Compare
Rebased to resolve conflict. |
Actually, I do not think any regular benchmarks would show the improvement as basically it is a kind of memory leak (so it is not e.g. an unnecessary heap allocation). However, the existing benchmarks shows that clearing does not introduce any overhead. Therefore my query was kind of irrational. |
Co-authored-by: Robert Pająk <pellared@hotmail.com>
Co-authored-by: Robert Pająk <pellared@hotmail.com>
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.
Thanks 👍
@open-telemetry/go-approvers, PTAL
@ash2k, thank you 🥇 |
This is documented at https://go.dev/wiki/SliceTricks:
... followed by examples of how zeroing out the slice elements solves the problem. This PR does the same.
sdk/log
:sdk/metric/internal/aggregate
:sdk/metric
:sdk/trace
: