Skip to content

Commit

Permalink
Allow GC to collect unneeded slice elements (#5804)
Browse files Browse the repository at this point in the history
```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.
  • Loading branch information
ash2k authored Nov 8, 2024
1 parent 1492efa commit 85eb76f
Show file tree
Hide file tree
Showing 7 changed files with 8 additions and 5 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
2 changes: 1 addition & 1 deletion sdk/log/record.go
Original file line number Diff line number Diff line change
Expand Up @@ -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]
}
Expand Down
1 change: 1 addition & 0 deletions sdk/metric/internal/aggregate/drop.go
Original file line number Diff line number Diff line change
Expand Up @@ -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]
}
1 change: 1 addition & 0 deletions sdk/metric/internal/aggregate/exemplar.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}()
Expand Down
2 changes: 2 additions & 0 deletions sdk/metric/pipeline.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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
}
Expand Down
1 change: 1 addition & 0 deletions sdk/trace/batch_span_processor.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
5 changes: 1 addition & 4 deletions sdk/trace/span.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down

0 comments on commit 85eb76f

Please sign in to comment.