puzpuzpuz / xsync

Concurrent data structures for Go
Apache License 2.0
1.13k stars 40 forks source link

Is there any reason to not use xsync.Map? #102

Open pelicans45 opened 1 year ago

pelicans45 commented 1 year ago

I have an application that makes use of a lot of different sync.Maps. It mostly fits the recommended use case for sync.Map ("append-only maps" with a lot more reads than writes), but the benchmarks for xsync.Map do look better across the board, so I was thinking of maybe just swapping them all out. I see your blog post mentions "When it comes to reads which are the strongest point of sync.Map, both data structures are on par.", though in the repo README you say "Due to this design, in all considered scenarios Map outperforms sync.Map."

Just wondering if you can foresee any reason to not do this. This blog post mentions potential memory concerns, but I think that probably won't be an issue for me.

puzpuzpuz commented 1 year ago

Hi,

xsync.Map doesn't have drawbacks when compared with sync.Map or at least I'm not aware of them. But in certain niche use cases, a plain map protected with a mutex may be a better choice. See https://github.com/puzpuzpuz/xsync/issues/94#issuecomment-1500807151 for more details.

mgnsk commented 1 year ago

I have a usecase where I found no reason to switch from the standard library's sync.Map: https://github.com/mgnsk/evcache/pull/35

pelicans45 commented 1 year ago

@puzpuzpuz I guess I'll need to do some testing

@mgnsk Interesting. Not sure how much of a difference it might make, but In my case I was thinking of using xsync.Map rather than xsync.MapOf.

puzpuzpuz commented 1 year ago

@mgnsk the only parallel benchmark you have has a contention point that kills the scalability of operations in both sync.Map and xsync.Map: https://github.com/mgnsk/evcache/blob/5e3f071a8d4be4eabddb0fb227d24121163e8741/cache_test.go#L302

Also, you always evict the same key which looks very artificial. Finally, you're using a mutex in Evict and PushBack methods (I didn't check others), so probably that's why a different concurrent map doesn't make any difference - the overhead of synchronization and other data structures is much more noticeable.

puzpuzpuz commented 1 year ago

@65947689564 first of all, I advise you to write benchmarks that are close enough to your use case rather than trust benchmarks in xsync or any other repository.

In general, sync.Map has very similar scalability characteristics to xsync.Map when it comes to read operations. Both don't use mutexes when reading data. But write operation scalability is very different: sync.Map uses a single mutex in many cases when it comes to the map mutation leading to blocked reads.

See https://github.com/golang/go/blob/13529cc5f443ef4e242da3716daa0032aa8d34f2/src/sync/map.go for more details. As for xsync.Map, this post explains how it works (the current design is slightly different).

As a rule of thumb, if your map never or almost never mutates, both sync.Map and xsync.Map should be on par. On the other hand, if it never mutates, you should go for a plain map which doesn't involve atomic operations and has plain memory layout. As for xsync.Map vs. xsync.MapOf, MapOf is a better choice since it doesn't use intermediate interface{} structs.

rabbbit commented 8 months ago

See #102 example of sync.Map performing better xsync.MapOf (in a fairly specific use-case).