[release-branch.go1.16] runtime: simplify histogram buckets considerably

There was an off-by-one error in the time histogram buckets calculation
that caused the linear sub-buckets distances to be off by 2x.

The fix was trivial, but in writing tests I realized there was a much
simpler way to express the calculation for the histogram buckets, and
took the opportunity to do that here. The new bucket calculation also
fixes the bug.

For #50732.
Fixes #50733.

Change-Id: Idae89986de1c415ee4e148f778e0e101ca003ade
Reviewed-on: https://go-review.googlesource.com/c/go/+/380094
Reviewed-by: Michael Pratt <mpratt@google.com>
Reviewed-by: Emmanuel Odeke <emmanuel@orijtech.com>
Trust: Michael Knyszek <mknyszek@google.com>
Run-TryBot: Michael Knyszek <mknyszek@google.com>
(cherry picked from commit 2e9dcb508647dc473a37ecfa244d2bc4a1843ab4)
Reviewed-on: https://go-review.googlesource.com/c/go/+/384620

1 files changed, 40 insertions, 0 deletions

diff --git a/src/runtime/histogram_test.go b/src/runtime/histogram_test.go
index dbc64fa559..b12b65a41e 100644
--- a/src/runtime/histogram_test.go
+++ b/src/runtime/histogram_test.go
@@ -68,3 +68,43 @@ func TestTimeHistogram(t *testing.T) {
 
 	dummyTimeHistogram = TimeHistogram{}
 }
+
+func TestTimeHistogramMetricsBuckets(t *testing.T) {
+	buckets := TimeHistogramMetricsBuckets()
+
+	nonInfBucketsLen := TimeHistNumSubBuckets * TimeHistNumSuperBuckets
+	expBucketsLen := nonInfBucketsLen + 2 // Count -Inf and +Inf.
+	if len(buckets) != expBucketsLen {
+		t.Fatalf("unexpected length of buckets: got %d, want %d", len(buckets), expBucketsLen)
+	}
+	// Check the first non-Inf 2*TimeHistNumSubBuckets buckets in order, skipping the
+	// first bucket which should be -Inf (checked later).
+	//
+	// Because of the way this scheme works, the bottom TimeHistNumSubBuckets
+	// buckets are fully populated, and then the next TimeHistNumSubBuckets
+	// have the TimeHistSubBucketBits'th bit set, while the bottom are once
+	// again fully populated.
+	for i := 1; i <= 2*TimeHistNumSubBuckets+1; i++ {
+		if got, want := buckets[i], float64(i-1)/1e9; got != want {
+			t.Errorf("expected bucket %d to have value %e, got %e", i, want, got)
+		}
+	}
+	// Check some values.
+	idxToBucket := map[int]float64{
+		0:                 math.Inf(-1),
+		33:                float64(0x10<<1) / 1e9,
+		34:                float64(0x11<<1) / 1e9,
+		49:                float64(0x10<<2) / 1e9,
+		58:                float64(0x19<<2) / 1e9,
+		65:                float64(0x10<<3) / 1e9,
+		513:               float64(0x10<<31) / 1e9,
+		519:               float64(0x16<<31) / 1e9,
+		expBucketsLen - 2: float64(0x1f<<43) / 1e9,
+		expBucketsLen - 1: math.Inf(1),
+	}
+	for idx, bucket := range idxToBucket {
+		if got, want := buckets[idx], bucket; got != want {
+			t.Errorf("expected bucket %d to have value %e, got %e", idx, want, got)
+		}
+	}
+}