-
Notifications
You must be signed in to change notification settings - Fork 17.7k
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
sync: new version of sync.Map #47643
Comments
If another sync.Map is added, I expect users will demand a generic version. Can this be made to work with the generics proposal? If so, maybe it could go into the proposed generics maps package as maps.Sync or some other name. |
I would prefer to improve (replace the implementation of) the existing If the proposed implementation satisfies all of the existing (documented) invariants of the We also need to be careful that |
Can you clarify what it means to say that nil values are not supported? I can think of a couple of possibilities. For example, perhaps you could show a small piece of code that works with the current |
Trying to answer all questions raised so far. Please let me know if something is still unclear. Say, I could go into more details on how atomic snapshots in readers work.
I believe it's possible to swap current So, yes, a replacement for As for generics, I'm not familiar with the design, so can't tell if
Yes, both nested and concurrent Stores in Range should work fine. It would certainly address #21035 since writers won't block any reader (at least in a general sense - with a lock/mutex; atomic snapshot code in readers may need to do some spins until it succeeds) and also writers that want to modify another hash table bucket. As for #21032, I'd say that it should be also resolved since readers don't need to acquire any mutex (and, in general, do any loads into shared memory). Due to this, readers should scale linearly with the number of cores.
I just mean that in my library I raise a panic when a nil value is provided. So But as I mentioned above it's possible to work around that restriction and support nil values. This would mean an allocation of intermediate interface{} struct, but I don't think it can't be considered as something impacting users. Other than that, the number of allocations in |
One more thing to mention. I didn't verify it yet, but there is a way to make scans more efficient than they are now. ATM they do an equality check for each key in scanned buckets, but that could be improved. By storing MSBs of key's hash code in tagged pointers to key/value, it would possible to skip entries known to have a different hash code. See puzpuzpuz/xsync#2 |
If there is a consensus to update In the meanwhile, I'm going to resolve both nil values and grow-only resize limitations in my library. |
Update. Both limitations were resolved. See puzpuzpuz/xsync#6 and puzpuzpuz/xsync#11 |
Closing this due to lack of activity. |
Those who need a generic concurrent map with extra methods such as |
See [this](https://puzpuzpuz.dev/so-long-syncmap) and [this](golang/go#47643). Map growing works a bit differently, and it doesn't support nil, but we do not store nils anyway. Signed-off-by: Dmitriy Matrenichev <[email protected]>
Preface
Many projects written in Golang have modules that use either
sync.Map
, or a plainmap
protected with a mutex. Those of them that usesync.Map
not necessarily fit into the use cases suggested by the docs:Such projects and even those that conform to the above use cases would benefit from better performance of
sync.Map
. This issue is aimed to describe a way to improve existingsync.Map
or add a new concurrent map implementation.Proposal
Proposed data structure may be found here: https://github.com/puzpuzpuz/xsync#map
This map uses a modified version of Cache-Line Hash Table (CLHT) data structure. CLHT is built around idea to organize the hash table in cache-line-sized buckets, so that on all modern CPUs update operations complete with at most one cache-line transfer. Also, Get operations involve no write to memory, as well as no mutexes or any other sort of locks.
xsync.Map
has some modifications of the CLHT algorithm.I won't go into all details of the original algorithm, but if you're familiar with Java's ConcurrentHashMap, it's somewhat similar, yet organizes the table into a CPU-friendly way. On 64-bit builds the bucket layout looks like the following:
More details and considerations may be found here.
In general, I'm under impression that the algorithm fits nicely into Golang due to flexible control over memory layout of structs, as well as the presence of garbage collection.
Compatibility aspects
The most significant behavioral difference with
sync.Map
is that nil values are not supported. That could be preserved, or a interface{} values pointing to a special "nil" struct could be used internally in the Map to mark nil values. Or the restriction could be put of the user code which means a breaking change in case if it's done insync.Map
.Resize is also different since
xsync.Map
is grow-only, but that could be changed if necessary.Benchmarks
The following image shows results of a benchmark run on a cloud VM (GCP, e2-highcpu-32, Intel Xeon, Haswell gen, Golang 1.16.5, Ubuntu 20.04 64-bit). The scenario assumes pre-warmed 1M entries, 99% Gets, 0.5% Stores, 0.5% Deletes and can be considered as read-heavy which is beneficial for
sync.Map
.It may be seen that
sync.Map
(BenchmarkMapStandard_WarmUp) doesn't scale in this scenario, whilexsync.Map
(BenchmarkMap_WarmUp) does. More measurements can be found here.If you have an idea of a better benchmark, I'm happy to run it and share the measurements. So far, I didn't find a scenario where
xsync.Map
would lead to a regression when compared withsync.Map
.Summary
Not necessary
xsync.Map
or its algorithm should be used for the next version ofsync.Map
and any other viable option may be considered. But having an efficient concurrent map as a part of the standard Golang library would be certainly beneficial for all users.The text was updated successfully, but these errors were encountered: