From e92a9a7377de447451b75da3f1389b419c3dfbe4 Mon Sep 17 00:00:00 2001 From: Yahor Yuzefovich Date: Fri, 26 Aug 2022 01:42:32 +0000 Subject: [PATCH] util/mon: remove nameWithPointer to reduce allocations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This field was added in order to help us track down some of the memory leaks which we have already found, and the field didn't turn out to be that useful. When it was introduced, the implications on the increase in allocations were unknown, and now I don't think the field is worth it. ``` name old time/op new time/op delta FlowSetup/vectorize=true/distribute=true-24 168µs ± 5% 164µs ± 4% -2.33% (p=0.007 n=19+20) FlowSetup/vectorize=true/distribute=false-24 167µs ± 6% 164µs ± 6% ~ (p=0.060 n=20+20) FlowSetup/vectorize=false/distribute=true-24 163µs ± 4% 161µs ± 7% ~ (p=0.057 n=19+20) FlowSetup/vectorize=false/distribute=false-24 161µs ± 6% 159µs ± 5% ~ (p=0.309 n=19+20) name old alloc/op new alloc/op delta FlowSetup/vectorize=true/distribute=true-24 19.6kB ± 8% 19.0kB ± 8% -2.62% (p=0.001 n=19+18) FlowSetup/vectorize=true/distribute=false-24 18.2kB ± 1% 17.7kB ± 1% -2.56% (p=0.000 n=17+16) FlowSetup/vectorize=false/distribute=true-24 25.8kB ± 2% 25.4kB ± 0% -1.44% (p=0.000 n=16+16) FlowSetup/vectorize=false/distribute=false-24 24.7kB ± 0% 24.4kB ± 1% -1.36% (p=0.000 n=16+16) name old allocs/op new allocs/op delta FlowSetup/vectorize=true/distribute=true-24 218 ± 2% 205 ± 3% -5.64% (p=0.000 n=19+19) FlowSetup/vectorize=true/distribute=false-24 208 ± 1% 197 ± 3% -5.63% (p=0.000 n=19+19) FlowSetup/vectorize=false/distribute=true-24 206 ± 0% 197 ± 0% -4.40% (p=0.000 n=16+16) FlowSetup/vectorize=false/distribute=false-24 197 ± 0% 188 ± 0% -4.54% (p=0.000 n=16+16) ``` Release justification: low-risk cleanup. Release note: None --- pkg/util/mon/bytes_usage.go | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/pkg/util/mon/bytes_usage.go b/pkg/util/mon/bytes_usage.go index 2885ac61fc22..a9f45472bef3 100644 --- a/pkg/util/mon/bytes_usage.go +++ b/pkg/util/mon/bytes_usage.go @@ -198,10 +198,6 @@ type BytesMonitor struct { // name identifies this monitor in logging messages. name redact.RedactableString - // nameWithPointer contains name with the address of the monitor attached to - // it. This can be used in logging messages to uniquely identify all - // messages for a single monitor. - nameWithPointer redact.RedactableString // resource specifies what kind of resource the monitor is tracking // allocations for. Specific behavior is delegated to this resource (e.g. @@ -305,7 +301,6 @@ func NewMonitorWithLimit( poolAllocationSize: increment, settings: settings, } - m.nameWithPointer = redact.Sprintf("%s (%p)", name, redact.Safe(m)) m.mu.curBytesCount = curCount m.mu.maxBytesHist = maxHist return m @@ -351,10 +346,10 @@ func (mm *BytesMonitor) Start(ctx context.Context, pool *BytesMonitor, reserved if log.V(2) { poolname := redact.RedactableString("(none)") if pool != nil { - poolname = pool.nameWithPointer + poolname = pool.name } log.InfofDepth(ctx, 1, "%s: starting monitor, reserved %s, pool %s", - mm.nameWithPointer, + mm.name, humanizeutil.IBytes(mm.reserved.used), poolname) } @@ -384,7 +379,6 @@ func NewUnlimitedMonitor( reserved: MakeStandaloneBudget(math.MaxInt64), settings: settings, } - m.nameWithPointer = redact.Sprintf("%s (%p)", name, redact.Safe(m)) m.mu.curBytesCount = curCount m.mu.maxBytesHist = maxHist return m @@ -413,7 +407,7 @@ func (mm *BytesMonitor) doStop(ctx context.Context, check bool) { // monitor is not shared any more. if log.V(1) && mm.mu.maxAllocated >= bytesMaxUsageLoggingThreshold { log.InfofDepth(ctx, 1, "%s, bytes usage max %s", - mm.nameWithPointer, + mm.name, humanizeutil.IBytes(mm.mu.maxAllocated)) } @@ -692,7 +686,7 @@ func (mm *BytesMonitor) reserveBytes(ctx context.Context, x int64) error { // many small allocations. if bits.Len64(uint64(mm.mu.curAllocated)) != bits.Len64(uint64(mm.mu.curAllocated-x)) { log.Infof(ctx, "%s: bytes usage increases to %s (+%d)", - mm.nameWithPointer, + mm.name, humanizeutil.IBytes(mm.mu.curAllocated), x) } } @@ -702,7 +696,7 @@ func (mm *BytesMonitor) reserveBytes(ctx context.Context, x int64) error { // We avoid VEventf here because we want to avoid computing the // trace string if there is nothing to log. log.Infof(ctx, "%s: now at %d bytes (+%d) - %s", - mm.nameWithPointer, mm.mu.curAllocated, x, util.GetSmallTrace(3)) + mm.name, mm.mu.curAllocated, x, util.GetSmallTrace(3)) } return nil } @@ -728,7 +722,7 @@ func (mm *BytesMonitor) releaseBytes(ctx context.Context, sz int64) { // We avoid VEventf here because we want to avoid computing the // trace string if there is nothing to log. log.Infof(ctx, "%s: now at %d bytes (-%d) - %s", - mm.nameWithPointer, mm.mu.curAllocated, sz, util.GetSmallTrace(5)) + mm.name, mm.mu.curAllocated, sz, util.GetSmallTrace(5)) } } @@ -744,7 +738,7 @@ func (mm *BytesMonitor) increaseBudget(ctx context.Context, minExtra int64) erro ) } if log.V(2) { - log.Infof(ctx, "%s: requesting %d bytes from the pool", mm.nameWithPointer, minExtra) + log.Infof(ctx, "%s: requesting %d bytes from the pool", mm.name, minExtra) } return mm.mu.curBudget.Grow(ctx, minExtra) @@ -768,7 +762,7 @@ func (mm *BytesMonitor) roundSize(sz int64) int64 { func (mm *BytesMonitor) releaseBudget(ctx context.Context) { // NB: mm.mu need not be locked here, as this is only called from StopMonitor(). if log.V(2) { - log.Infof(ctx, "%s: releasing %d bytes to the pool", mm.nameWithPointer, mm.mu.curBudget.allocated()) + log.Infof(ctx, "%s: releasing %d bytes to the pool", mm.name, mm.mu.curBudget.allocated()) } mm.mu.curBudget.Clear(ctx) }