Better MapCache concurrency#1849
Conversation
|
Can this just be a new cache type instead of completely replacing the prior one? |
|
@drcrallen if there is a good reason to keep the old one yes, but for now I'm just looking for feedback |
There was a problem hiding this comment.
Can this be a static ImmutableMap?
There was a problem hiding this comment.
ImmutableMap.of() does return a static
|
The reason to add new cache is to follow the "add and deprecate" method when doing features. If this was a simple modification of the prior map cache then it would make sense to just improve the prior impl. But this is essentially a re-write. |
|
Ok, I did some benchmarking and found that the level of concurrency in a java ConcurrentHashMap DOES have a very notable influence on its performance.... in java7 But in java8 I did not find a statistically significant difference. Good job java team on that one. ConcurrentHashMap in java 8 is leaps and bounds faster than in java7 |
|
@drcrallen @xvrl can you take a look at https://github.com/ben-manes/concurrentlinkedhashmap ？ |
There was a problem hiding this comment.
if my understanding is correct, there is no eviction on this cache, if so, i don't see why not using a simple concurrent hash map ?
|
i don't see any UTs for eviction especially for the custom made weight function. |
|
@binlijin Please consider Caffeine if this project is Java 8 based. I'd much rather see projects adopt that then CLHM, if possible. Guava's performance is sadly subpar because I punted on some important optimizations during the port and was then unsuccessful in advocating for them afterwards. Caffeine supports all of Guava's features, has significantly higher throughput, and much better hit rate. |
|
@ben-manes Thanks for the info! I'm filing an issue on the matter. Officially druid only requires java7, but there's no reason an extension would be forbidden from requiring java8. |
|
Closing in favor of #3028 |
6 participants
This is a first stab at a better MapCache implementation, which uses Guava cache instead of a synchronized Map which caused bad locking behavior when closing namespaces.
Some initial benchmarking seems to indicate it may exhibit somewhat higher latencies overall, but we need more realistic workloads to tell for sure. At least the locking issue should be reduced.
fixes #1836