-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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: replace Map implementation with HashTrieMap #70683
Comments
I was thinking about this analysis again and I was wrong to say that the old map did not shrink. I intended to say that in the benchmark it wouldn't shrink. The old map implementation indeed does shrink, it just takes much longer. Deleted entries from the read-only map are replaced with a tombstone ("expunged") and only fully cleared when a new dirty map is promoted to a read-only map. This requires enough misses to happen in the read-only map. The new map doesn't have such a generational approach, and disjoint sets of keys can be added and removed at will, with the memory for those being promptly released. The downside is that programs (more likely microbenchmarks) that very frequently add and remove the same key are going to suffer. Again, I'm not terribly worried about this case. I've updated the comment above. |
hi @mknyszek out of curiosity, can you explain what's the core of the "HashTrieMap" and how it differs from the original implementation? |
Hi @PeterBocan , this new implementation was originally part of CL 573956 as an internal HashTrieMap to support the new unique package. You can look at that CL, including the code comments, and the associated #62483 issue. For replacing the public facing sync.Map implementation, you can look at this stack of CLs, including some performance comments in CL 608335, and of course the performance numbers at the start of this issue here are useful as well. There is also some related discussion comparing the old and new sync.Map implementations (starting in ~2 months ago in December) in #21035 and #21032. The old implementation targeted a narrower set of use cases (e.g., see CL 33912 and #18177), especially some of the "read mostly" caches that were used in the stdlib. After you look at some of the details, you could open a thread on the golang-nuts mailing list either with more specific follow-up questions, or even just post there with a summary of what you learned. (There are very knowledgeable people on golang-nuts, and it's a topic people tend to find interesting, so I think you'd get useful replies there). |
HashTrieMap
was added for theunique
package, but it turns out that it's faster thanMap
in many cases, including its own microbenchmarks.This work was already done during the Go 1.24 cycle, culminating in https://go.dev/cl/608335. This issue just captures the work that was done so that it can be referenced.
Microbenchmarks
Higher
GOMAXPROCS
values are omitted because they're not very useful. It's very unlikely that the data structure will be heavily contended on all cores in the same way in real programs; even 4 is pushing it.Analysis
The load-hit case is slightly slower due to Swiss Tables improving the performance of the old
sync.Map
(the new implementation was slightly faster before those changes, when I initially benchmarked). Some benchmarks show a seemingly large slowdown, but that's mainly due to the fact that the new implementation shrinks promptly, whereas the old one shrinks in generations (the dirty map needs to be promoted).The new implementation also avoids issues the old one had, like the ramp-up time of promoting a key to read-only (which isn't captured by the benchmarks) and likely lower contention when mutating disjoint sets of keys.
Feedback
If any of the slowdowns affect you, you can set
GOEXPERIMENT=nosynchashtriemap
at build time to revert to the old implementation, and please file a bug. We'd like the implementations to not cause slowdowns in real-world programs.(We can possibly avoid some of these additional allocations by adding a
sync.Pool
for entry nodes to the map's structure, though this would mainly only help heavy uses ofsync.Map
and may not actually matter in practice.)The text was updated successfully, but these errors were encountered: