allegro / bigcache

Efficient cache for gigabytes of data written in Go.
http://allegro.tech/2016/03/writing-fast-cache-service-in-go.html
Apache License 2.0
7.45k stars 593 forks source link

Benchmark is not fair, So the speed can't be trusted #356

Closed LawyZheng closed 11 months ago

LawyZheng commented 1 year ago

Thanks for the cache package, it helps me a lot.

But I find some problems with the result when I read the Benchmarks result carefully.

BenchmarkFreeCacheSet-8                 11068976           703 ns/op         328 B/op          2 allocs/op
BenchmarkBigCacheSet-8                  10183717           478 ns/op         304 B/op          2 allocs/op

BenchmarkBigCacheSetParallel-8          34233472           148 ns/op         317 B/op          3 allocs/op
BenchmarkFreeCacheSetParallel-8         34222654           268 ns/op         350 B/op          3 allocs/op

The main problem is why the more times the test function run, the longer avg time it costs.

As we all know, the first number means the times the test function runs, and the second means the average of each time cost. And the bench time is the same according to -benchtime=4s.

So I got confused about why this happened. It's impossible.

I read the source code of the benchmark test, and finally, I found where the problem lies. https://github.com/allegro/bigcache-bench/blob/98e08029e2a1964c70a4e55c5324c004b8826c9c/caches_bench_test.go#L32-L34

for i := 0; i < b.N; i++ {
    cache.Set([]byte(key(i)), value(), 0)
}

This is not a good code for the benchmark test, because it uses b.N as parameters. b.N stands for how many times the function will be executed.

Under the same time duration, the function which is faster will be executed more times. So the b.N will go bigger than the other one. As b.N goes bigger, it will set more elements to the cache, leading the whole function to take more time definitely. So I think it is an unfair benchmark for the faster one.

Or we can have the tests in which we set the benchtime to 1s, 2s, 3s, 4s, 5s, 6s, and 7s. And we will see the FreeCache goes faster than BigCache in the beginning, but it will spend more avg time with the benchtime going up, although it runs more times than BigCache.

In my option, since the benchmark remains controversial, who is the faster between BigCache and FreeCache should be reconsidered.

LawyZheng commented 1 year ago

In addition, I think the benchmark should add the case where the struct is used as the value rather than the []byte, especially when compared to the Map or Sync.Map.

Because we usually use the cache to store our instances of the struct rather than the []byte, in this case, the cost of serialization should be considered in the BigCache and FreeCache.

janisz commented 1 year ago

@LawyZheng good point. Waiting for PRs with new benchmarks

LawyZheng commented 1 year ago

@LawyZheng good point. Waiting for PRs with new benchmarks

already created a PR https://github.com/allegro/bigcache-bench/pull/11

LawyZheng commented 1 year ago

PR has been merged successfully. After you guys update the README in both repositories, I will close this issue.