Avoid unnecessary synchronization in QueryCache#queryContains - #2298
Avoid unnecessary synchronization in QueryCache#queryContains#22981wairesd wants to merge 2 commits into
Conversation
Use a lock-free ConcurrentHashMap.get() before falling back to compute() to avoid bucket-level locking when the result is already cached. On the hot path (repeated queries to the same block position within a tick), this eliminates contention on the CHM segment lock entirely, reducing overhead to a single volatile read.
|
It looks like the lock ensures that access to the inner map is synchronized, which seems to be required. |
Replace EnumMap with ConcurrentHashMap in QueryOption.createCache() so that concurrent reads and computeIfAbsent calls on the inner map are thread-safe. This allows QueryCache.queryContains() to use a lock-free get() fast-path before falling back to compute(), avoiding unnecessary bucket-level locking on repeated queries to the same location. Use QueryOption.values().length as initial capacity since the map will never hold more entries than there are QueryOption enum constants.
|
Fixed the thread-safety concern by replacing EnumMap with ConcurrentHashMap for the inner option map in all three QueryOption.createCache() implementations. The computeIfAbsent calls on the inner map are now safe, and the lock-free get() fast-path in QueryCache.queryContains() is correct. |
| // Fast path: avoid locking if the result is already cached | ||
| Map<QueryOption, ApplicableRegionSet> existing = cache.get(key); | ||
| if (existing != null) { | ||
| ApplicableRegionSet result = existing.get(option); | ||
| if (result != null) { | ||
| return result; | ||
| } | ||
| } |
There was a problem hiding this comment.
Instead of this, can we split the createCache method into two parts, one that returns the appropriate map, and one that fills in the information with computeIfAbsent or whatever is typically used in the != null branch? That way, instead of still having lock contention for every initial query, we get out of the computeIfAbsent quickly and do a follow-up one only on the smaller map if necessary.
It might not be perfectly that clean, but I think we should avoid having expensive logic inside a compute or this fix won't help that much.
3 participants
Use a lock-free ConcurrentHashMap.get() before falling back to compute() to avoid bucket-level locking when the result is already cached. On the hot path (repeated queries to the same block position within a tick), this eliminates contention on the CHM segment lock entirely, reducing overhead to a single volatile read.