Use binary search to pick bucket.

[beorn7]

With the usual number of buckets, this doesn't really make a
difference, but it should scale...

@juliusv @brian-brazil

[brian-brazil]

Do you have a benchmark for this?

Branch misprediction may cause this to be sub-optimal.

[juliusv]

Yeah, would be interesting to see at what bucket counts this actually amortizes (very large, I suspect) and what the time overhead is for more normal bucket counts.

[beorn7]

Benchmarks are in the code. The difference is within the statistical noise for the very low bucket count. So it is not measurable slower, but one day, somebody wants to have 10,000 buckets, and that will be my moment of triumph...

[juliusv]

Hmm, in a totally sequential use case like this:

func BenchmarkHistogramSerial(b *testing.B) {
  b.StopTimer()
  s := NewHistogram(HistogramOpts{})

  b.StartTimer()
  for i := 0; i < b.N; i++ {
    s.Observe(<insert value here>)
  }
}

For the old code, I get 55ns/op for observing 0 values and 65ns/ops for observing 10.

For the new code, I get 88ns/op pretty much no matter which value I observe in the bucket range.

[beorn7]

Benchmarks are already provided in the code. I ran them and could not see any difference except noise. The reason probably is that the benchmarks in the code do a bit more (many goroutines in parallel, observe values from an array of samples, not just a constant value) so that more things add to the time that do not depend on the search strategy.

Extrapolating from your result above, the break even would be reached at about 40 buckets, which is still quite realistic. We can try that out. I also want to accommodate the case with 100 or 1000 buckets, even if I have to pay 20ns penalty per observation for cases with few buckets. (We should try that out. Once I'm in...) We were joking about the death spiral (latencies increase, and now even the time to Observe() those latencies increase...). But should those 20ns really matter, than we really don't want to have the observe time depend on the observed value.

[beorn7]

So there is actually a pretty "pure" microbenchmark in the code: BenchmarkHistogramObserve1

My results: 27.1 ns/op with linear search, 33.6 ns/op with binary search.
That's default bucket count, and the observed values are increasing, i.e. most of them end up in the highest bucket.

If it were only for me, I'd consider these changes irrelevant in practice and go for binary search just to not get linearly increasing observe times with higher bucket counts.

If you insist, I will run benchmarks with higher bucket counts (and 'fairer' conditions where most of the observations happen in the middle buckets). I'll find the break-even point and will implement a switch to the most efficient search method depending on bucket count. (I just thing I have more pressing things to do...)

[juliusv]

Agreed, that's something we can do later. Let's just merge this for now, but keep in mind it's something that can be optimized later, if needed.

👍

[beorn7]

I'll add a TODO for that.

[beorn7]

I couldn't resist and ran a benchmark (BenchmarkHistogramNoLabels, which is almost exactly like Julius's benchmark above).
11 buckets: 38.3 ns/op linear - binary 48.7 ns/op
100 buckets: 78.1 ns/op linear - binary 54.9 ns/op
300 buckets: 154 ns/op linear - binary 61.6 ns/op