Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changing RWMutexMap to sync.Map #72

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

vidmed
Copy link

@vidmed vidmed commented Nov 30, 2017

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.

@patrickmn
Copy link
Owner

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?

@moadqassem
Copy link

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.

@patrickmn
Copy link
Owner

Still waiting for CONTRIBUTORS change. @vidmed ?

@vidmed
Copy link
Author

vidmed commented Mar 27, 2018

@patrickmn sorry for huge delay.
contributor list updated

@zouyx
Copy link

zouyx commented Mar 28, 2018

Must i update to 1.9?

@vidmed
Copy link
Author

vidmed commented Mar 29, 2018

If you mean golang - yes. Sync map was realised in go 1.9

@patrickmn
Copy link
Owner

Unfortunately, I think @zouyx brings up a good point. I'm not sure it's worth breaking installs... :(

@bobintornado
Copy link

merge it to version 2? 😆

@wvh
Copy link

wvh commented Aug 23, 2018

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.

@patrickmn
Copy link
Owner

Taken into consideration backwards compatibility and the uncertain performance impact, I think it's best not to merge this for now.

@btkador
Copy link

btkador commented Feb 11, 2019

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.

@sylr
Copy link

sylr commented Feb 17, 2019

If #92 is merged you could make a new Cacher implementation which implements sync.Map.

With this there would be no backward compatibility problem.

Repository owner deleted a comment from pablodz Jan 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants