From 85eb76f2fd8ef474b7cbb4e5f8dc307b872cf3c6 Mon Sep 17 00:00:00 2001 From: Mikhail Mazurskiy <126021+ash2k@users.noreply.github.com> Date: Fri, 8 Nov 2024 17:36:35 +1100 Subject: [PATCH] Allow GC to collect unneeded slice elements (#5804) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ```go type interInst struct { x int } type inter interface { } var sink []inter func BenchmarkX(b *testing.B) { sink = make([]inter, b.N) for i := 0; i < b.N; i++ { sink[i] = &interInst{} } clear(sink) sink = sink[:0] runtime.GC() var ms runtime.MemStats runtime.ReadMemStats(&ms) b.Log(b.N, ms.Frees) // Frees is the cumulative count of heap objects freed. } ``` ``` clear: ioz_test.go:35: 1 589 ioz_test.go:35: 100 711 ioz_test.go:35: 10000 10729 ioz_test.go:35: 1000000 1010750 <-- 1m+ freed ioz_test.go:35: 16076874 17087643 ioz_test.go:35: 19514749 36602412 ``` ``` no clear: ioz_test.go:35: 1 585 ioz_test.go:35: 100 606 ioz_test.go:35: 10000 725 ioz_test.go:35: 1000000 10745 <-- some "overhead" objects freed, not the slice. ioz_test.go:35: 16391445 1010765 ioz_test.go:35: 21765238 17402230 ``` This is documented at https://go.dev/wiki/SliceTricks: > NOTE If the type of the element is a pointer or a struct with pointer fields, which need to be garbage collected, the above implementations of Cut and Delete have a potential memory leak problem: some elements with values are still referenced by slice a’s underlying array, just not “visible” in the slice. Because the “deleted” value is referenced in the underlying array, the deleted value is still “reachable” during GC, even though the value cannot be referenced by your code. If the underlying array is long-lived, this represents a leak. Followed by examples of how zeroing out the slice elements solves the problem. This PR does the same. --- CHANGELOG.md | 1 + sdk/log/record.go | 2 +- sdk/metric/internal/aggregate/drop.go | 1 + sdk/metric/internal/aggregate/exemplar.go | 1 + sdk/metric/pipeline.go | 2 ++ sdk/trace/batch_span_processor.go | 1 + sdk/trace/span.go | 5 +---- 7 files changed, 8 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 862048c31f5..b4b14b58253 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -46,6 +46,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Support scope attributes and make them as identifying for `Meter` in `go.opentelemetry.io/otel` and `go.opentelemetry.io/otel/sdk/metric`. (#5926) - Support scope attributes and make them as identifying for `Logger` in `go.opentelemetry.io/otel` and `go.opentelemetry.io/otel/sdk/log`. (#5925) - Make schema URL and scope attributes as identifying for `Tracer` in `go.opentelemetry.io/otel/bridge/opentracing`. (#5931) +- Clear unneeded slice elements to allow GC to collect the objects in `go.opentelemetry.io/otel/sdk/metric` and `go.opentelemetry.io/otel/sdk/trace`. (#5804) ### Removed diff --git a/sdk/log/record.go b/sdk/log/record.go index ea842660b0b..155e4cad2b6 100644 --- a/sdk/log/record.go +++ b/sdk/log/record.go @@ -234,7 +234,7 @@ func (r *Record) AddAttributes(attrs ...log.KeyValue) { // // Do not use head(attrs, r.attributeCountLimit - n) here. If // (r.attributeCountLimit - n) <= 0 attrs needs to be emptied. - last := max(0, (r.attributeCountLimit - n)) + last := max(0, r.attributeCountLimit-n) r.addDropped(len(attrs) - last) attrs = attrs[:last] } diff --git a/sdk/metric/internal/aggregate/drop.go b/sdk/metric/internal/aggregate/drop.go index 76d52839b60..8396faaa4ae 100644 --- a/sdk/metric/internal/aggregate/drop.go +++ b/sdk/metric/internal/aggregate/drop.go @@ -22,5 +22,6 @@ func (r *dropRes[N]) Offer(context.Context, N, []attribute.KeyValue) {} // Collect resets dest. No exemplars will ever be returned. func (r *dropRes[N]) Collect(dest *[]exemplar.Exemplar) { + clear(*dest) // Erase elements to let GC collect objects *dest = (*dest)[:0] } diff --git a/sdk/metric/internal/aggregate/exemplar.go b/sdk/metric/internal/aggregate/exemplar.go index dcb899d6267..25d709948e9 100644 --- a/sdk/metric/internal/aggregate/exemplar.go +++ b/sdk/metric/internal/aggregate/exemplar.go @@ -17,6 +17,7 @@ var exemplarPool = sync.Pool{ func collectExemplars[N int64 | float64](out *[]metricdata.Exemplar[N], f func(*[]exemplar.Exemplar)) { dest := exemplarPool.Get().(*[]exemplar.Exemplar) defer func() { + clear(*dest) // Erase elements to let GC collect objects. *dest = (*dest)[:0] exemplarPool.Put(dest) }() diff --git a/sdk/metric/pipeline.go b/sdk/metric/pipeline.go index c504984df80..775e2452619 100644 --- a/sdk/metric/pipeline.go +++ b/sdk/metric/pipeline.go @@ -132,6 +132,7 @@ func (p *pipeline) produce(ctx context.Context, rm *metricdata.ResourceMetrics) } if err := ctx.Err(); err != nil { rm.Resource = nil + clear(rm.ScopeMetrics) // Erase elements to let GC collect objects. rm.ScopeMetrics = rm.ScopeMetrics[:0] return err } @@ -145,6 +146,7 @@ func (p *pipeline) produce(ctx context.Context, rm *metricdata.ResourceMetrics) if err := ctx.Err(); err != nil { // This means the context expired before we finished running callbacks. rm.Resource = nil + clear(rm.ScopeMetrics) // Erase elements to let GC collect objects. rm.ScopeMetrics = rm.ScopeMetrics[:0] return err } diff --git a/sdk/trace/batch_span_processor.go b/sdk/trace/batch_span_processor.go index 4ce757dfd6b..ccc97e1b662 100644 --- a/sdk/trace/batch_span_processor.go +++ b/sdk/trace/batch_span_processor.go @@ -280,6 +280,7 @@ func (bsp *batchSpanProcessor) exportSpans(ctx context.Context) error { // // It is up to the exporter to implement any type of retry logic if a batch is failing // to be exported, since it is specific to the protocol and backend being sent to. + clear(bsp.batch) // Erase elements to let GC collect objects bsp.batch = bsp.batch[:0] if err != nil { diff --git a/sdk/trace/span.go b/sdk/trace/span.go index 730fb85c3ef..17f883c2c86 100644 --- a/sdk/trace/span.go +++ b/sdk/trace/span.go @@ -639,10 +639,7 @@ func (s *recordingSpan) dedupeAttrsFromRecord(record map[attribute.Key]int) { record[a.Key] = len(unique) - 1 } } - // s.attributes have element types of attribute.KeyValue. These types are - // not pointers and they themselves do not contain pointer fields, - // therefore the duplicate values do not need to be zeroed for them to be - // garbage collected. + clear(s.attributes[len(unique):]) // Erase unneeded elements to let GC collect objects. s.attributes = unique }