From 2ae2a94857cb17a98a86a8332d6f76863982bf59 Mon Sep 17 00:00:00 2001 From: Michael Anthony Knyszek Date: Wed, 16 Sep 2020 16:22:28 +0000 Subject: [PATCH] runtime: fix leak and locking in BenchmarkMSpanCountAlloc CL 249917 made the mspan in MSpanCountAlloc no longer stack-allocated (for good reason), but then allocated an mspan on each call and did not free it, resulting in a leak. That allocation was also not protected by the heap lock, which could lead to data corruption of mheap fields and the spanalloc. To fix this, export some functions to allocate/free dummy mspans from spanalloc (with proper locking) and allocate just one up-front for the benchmark, freeing it at the end. Then, update MSpanCountAlloc to accept a dummy mspan. Note that we need to allocate the dummy mspan up-front otherwise we measure things like heap locking and fixalloc performance instead of what we actually want to measure: how fast we can do a popcount on the mark bits. Fixes #41391. Change-Id: If6629a6ec1ece639c7fb78532045837a8c872c04 Reviewed-on: https://go-review.googlesource.com/c/go/+/255297 Run-TryBot: Michael Knyszek Reviewed-by: Keith Randall TryBot-Result: Go Bot Trust: Michael Knyszek --- src/runtime/export_test.go | 28 +++++++++++++++++++++++++--- src/runtime/gc_test.go | 6 +++++- 2 files changed, 30 insertions(+), 4 deletions(-) diff --git a/src/runtime/export_test.go b/src/runtime/export_test.go index 929bb35db6..e3d6441c18 100644 --- a/src/runtime/export_test.go +++ b/src/runtime/export_test.go @@ -983,9 +983,31 @@ func MapHashCheck(m interface{}, k interface{}) (uintptr, uintptr) { return x, y } -func MSpanCountAlloc(bits []byte) int { - s := (*mspan)(mheap_.spanalloc.alloc()) +// mspan wrapper for testing. +//go:notinheap +type MSpan mspan + +// Allocate an mspan for testing. +func AllocMSpan() *MSpan { + var s *mspan + systemstack(func() { + s = (*mspan)(mheap_.spanalloc.alloc()) + }) + return (*MSpan)(s) +} + +// Free an allocated mspan. +func FreeMSpan(s *MSpan) { + systemstack(func() { + mheap_.spanalloc.free(unsafe.Pointer(s)) + }) +} + +func MSpanCountAlloc(ms *MSpan, bits []byte) int { + s := (*mspan)(ms) s.nelems = uintptr(len(bits) * 8) s.gcmarkBits = (*gcBits)(unsafe.Pointer(&bits[0])) - return s.countAlloc() + result := s.countAlloc() + s.gcmarkBits = nil + return result } diff --git a/src/runtime/gc_test.go b/src/runtime/gc_test.go index c5c8a4cecf..9edebdada6 100644 --- a/src/runtime/gc_test.go +++ b/src/runtime/gc_test.go @@ -763,6 +763,10 @@ func BenchmarkScanStackNoLocals(b *testing.B) { } func BenchmarkMSpanCountAlloc(b *testing.B) { + // Allocate one dummy mspan for the whole benchmark. + s := runtime.AllocMSpan() + defer runtime.FreeMSpan(s) + // n is the number of bytes to benchmark against. // n must always be a multiple of 8, since gcBits is // always rounded up 8 bytes. @@ -774,7 +778,7 @@ func BenchmarkMSpanCountAlloc(b *testing.B) { b.ResetTimer() for i := 0; i < b.N; i++ { - runtime.MSpanCountAlloc(bits) + runtime.MSpanCountAlloc(s, bits) } }) } -- 2.48.1