go-cache
go-cache copied to clipboard
patrickmn
Changing RWMutexMap to sync.Map
sync.Map has better performance on multiple cpus than RWMutexMap. Some benchmark results. on 32-core machine:
sync.Map
BenchmarkCacheGetConcurrentExpiring-32 200000000 8.32 ns/op BenchmarkCacheGetConcurrentNotExpiring-32 500000000 3.84 ns/op BenchmarkCacheGetManyConcurrentExpiring-32 2000000000 5.33 ns/op BenchmarkCacheGetManyConcurrentNotExpiring-32 2000000000 0.54 ns/op BenchmarkShardedCacheGetManyConcurrentExpiring-32 2000000000 10.6 ns/op BenchmarkShardedCacheGetManyConcurrentNotExpiring-32 2000000000 2.48 ns/op
RWMap
BenchmarkCacheGetConcurrentExpiring-32 20000000 102 ns/op BenchmarkCacheGetConcurrentNotExpiring-32 20000000 79.5 ns/op BenchmarkCacheGetManyConcurrentExpiring-32 100000000 33.2 ns/op BenchmarkCacheGetManyConcurrentNotExpiring-32 100000000 81.5 ns/op BenchmarkShardedCacheGetManyConcurrentExpiring-32 500000000 105 ns/op BenchmarkShardedCacheGetManyConcurrentNotExpiring-32 100000000 19.7 ns/op
Maybe this would be useful.
This is sufficiently mindblowing so as to be suspicious.
In the docs it states
It is optimized for use in concurrent loops with keys that are stable over time, and either few steady-state stores, or stores localized to one goroutine per key. For use cases that do not share these attributes, it will likely have comparable or worse performance and worse type safety than an ordinary map paired with a read-write mutex.
I'm trying to understand under which circumstances the performance is actually worse. There should probably be a benchmark that simulates that. The note about being best suited for cases where only one goroutine fetches a key is worrying since that's likely not the case much of the time.
In any case, this is also great because it cleans up all the mutex stuff nicely.
Could you please add yourself to CONTRIBUTORS?
This PR is open for more than one month, is it going to be merged at some point or at least tested to validate those Benchmarking data? I also thought of replacing the MutexMap with the new concurrent map that was introduced in Go 1.9.
Unfortunately, I think @zouyx brings up a good point. I'm not sure it's worth breaking installs... :(
The performance of sync.Map really depends on several factors such as number of cores, see for instance this article. If you don't have obvious contention issues, or if you do a lot of writes, you will probably lose some performance if you switch to sync.Map. So the choice isn't that easy.
Taken into consideration backwards compatibility and the uncertain performance impact, I think it's best not to merge this for now.
I'm using (original) go-cache on multiple places in my code to cache and handle network packet informations for hundred-thousands of packets per second. I've tested the vidmed sync.Map fork yesterday and the packet/sec performance went down almost 50%, while the CPU load was the same.
Unfortunately I've no time resource to further debug this, just wanted to let you know.
If https://github.com/patrickmn/go-cache/pull/92 is merged you could make a new Cacher implementation which implements sync.Map.
With this there would be no backward compatibility problem.
