runtime/debug: make TestFreeOSMemory more robust

FreeOSMemory relies on the function FreeOSMemory increasing HeapReleased
as opposed to the background scavenger, because it reads memory stats
*after* the free of a large allocation. However, before that even
happens, the background scavenger can swoop in and release all that
memory, making it appear as if FreeOSMemory didn't do anything.

This change modifies the test to just make sure that the large
allocation's memory is returned to the OS *somehow*, by the end of the
test. It doesn't really care which happens. It also increases the size
of that large allocation to increase the likelihood that the test isn't
relying 100% on the background scavenger, and that FreeOSMemory is doing
some of the work.

Fixes #49478.

Change-Id: Ief1d839753720ebb88cbb616c46302293ee2d19c
Reviewed-on: https://go-review.googlesource.com/c/go/+/363414
Reviewed-by: David Chase <drchase@google.com>
Trust: Michael Knyszek <mknyszek@google.com>

diff --git a/src/runtime/debug/garbage_test.go b/src/runtime/debug/garbage_test.go
index 69e769ecf29d382f42a8fe417746afa952ab46af..c3501408dd4264b44afabd03dbd0ea158cc860a8 100644 (file)
--- a/src/runtime/debug/garbage_test.go
+++ b/src/runtime/debug/garbage_test.go
@@ -6,6 +6,7 @@ package debug_test
 
 import (
        "internal/testenv"
+       "os"
        "runtime"
        . "runtime/debug"
        "testing"
@@ -87,21 +88,65 @@ func TestReadGCStats(t *testing.T) {
        }
 }
 
-var big = make([]byte, 1<<20)
+var big []byte
 
 func TestFreeOSMemory(t *testing.T) {
-       var ms1, ms2 runtime.MemStats
+       // Tests FreeOSMemory by making big susceptible to collection
+       // and checking that at least that much memory is returned to
+       // the OS after.
 
-       if big == nil {
-               t.Skip("test is not reliable when run multiple times")
-       }
-       big = nil
+       const bigBytes = 32 << 20
+       big = make([]byte, bigBytes)
+
+       // Make sure any in-progress GCs are complete.
        runtime.GC()
-       runtime.ReadMemStats(&ms1)
+
+       var before runtime.MemStats
+       runtime.ReadMemStats(&before)
+
+       // Clear the last reference to the big allocation, making it
+       // susceptible to collection.
+       big = nil
+
+       // FreeOSMemory runs a GC cycle before releasing memory,
+       // so it's fine to skip a GC here.
+       //
+       // It's possible the background scavenger runs concurrently
+       // with this function and does most of the work for it.
+       // If that happens, it's OK. What we want is a test that fails
+       // often if FreeOSMemory does not work correctly, and a test
+       // that passes every time if it does.
        FreeOSMemory()
-       runtime.ReadMemStats(&ms2)
-       if ms1.HeapReleased >= ms2.HeapReleased {
-               t.Errorf("released before=%d; released after=%d; did not go up", ms1.HeapReleased, ms2.HeapReleased)
+
+       var after runtime.MemStats
+       runtime.ReadMemStats(&after)
+
+       // Check to make sure that the big allocation (now freed)
+       // had its memory shift into HeapReleased as a result of that
+       // FreeOSMemory.
+       if after.HeapReleased <= before.HeapReleased {
+               t.Fatalf("no memory released: %d -> %d", before.HeapReleased, after.HeapReleased)
+       }
+
+       // Check to make sure bigBytes was released, plus some slack. Pages may get
+       // allocated in between the two measurements above for a variety for reasons,
+       // most commonly for GC work bufs. Since this can get fairly high, depending
+       // on scheduling and what GOMAXPROCS is, give a lot of slack up-front.
+       //
+       // Add a little more slack too if the page size is bigger than the runtime page size.
+       // "big" could end up unaligned on its ends, forcing the scavenger to skip at worst
+       // 2x pages.
+       slack := uint64(bigBytes / 2)
+       pageSize := uint64(os.Getpagesize())
+       if pageSize > 8<<10 {
+               slack += pageSize * 2
+       }
+       if slack > bigBytes {
+               // We basically already checked this.
+               return
+       }
+       if after.HeapReleased-before.HeapReleased < bigBytes-slack {
+               t.Fatalf("less than %d released: %d -> %d", bigBytes, before.HeapReleased, after.HeapReleased)
        }
 }