From 2e9dcb508647dc473a37ecfa244d2bc4a1843ab4 Mon Sep 17 00:00:00 2001 From: Michael Anthony Knyszek Date: Fri, 21 Jan 2022 06:52:43 +0000 Subject: 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. Fixes #50732. Change-Id: Idae89986de1c415ee4e148f778e0e101ca003ade Reviewed-on: https://go-review.googlesource.com/c/go/+/380094 Reviewed-by: Michael Pratt Reviewed-by: Emmanuel Odeke Trust: Michael Knyszek Run-TryBot: Michael Knyszek --- src/runtime/histogram_test.go | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) (limited to 'src/runtime/histogram_test.go') 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) + } + } +} -- cgit v1.3-5-g9baa