CAMEL-19295: Basic thread-safe LRU cache #10157
Conversation
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Maintainers, please note that first-time contributors require manual approval for the GitHub Actions to run. If necessary Apache Camel Committers may access logs and test results in the job summaries! |
|
🚫 There are (likely) no components to be tested in this PR |
|
@davsclaus I've just got the results of testing this code. It has quite a big impact in concurrent scenarios. I run some tests comparing with Camel 4.0.0-M3 (baseline) with this patch. For resolving the endpoints: Baseline: This patch: Note: the _X is the number of threads (i.e.; EndpointResolveTest.testActionStatus_4 is the test with 4 threads). For the endpoint registry operations: Baseline: This patch: And the impact is quite extreme for the registry under concurrent access: Baseline: This patch: |
|
Yeah can you test with camel-caffeine-lrucache as well. Also I think we can have a non LRU cache, we often just want to keep 1000 elements in a cache, and they may not need to be exact LRU based. So they can be FIFO or anything like that, just that the size of big enough for normal use-cases. So basically we can make a cache that is just a ConcurrentMap from the JDK to be used in the various places, where a LRU is really not needed. |
|
Sure thing. Let me run the tests with
Indeed. Maybe, it's the case we could go with a simpler alternative by default and ... if needed, we can leave it flexible so users can plug a more extensible one if they need to. I was thinking maybe, we could have a ring buffer (circular queue). IMHO, we may not necessarily need to expire the least-recently used records, but we could overwrite them if needed be. |
|
@davsclaus if it helps I have an idea of how to implement a basic thread-safe LRU cache. Let me know if you are interested. |
|
@essobedo yeah sure, we may only need LRU in some situations
|
|
@essobedo if you come up with the LRU Cache relatively soon - no pressure - I also offer to run the same tests on the perf lab I have access. I should have access to the machines for a few more weeks, so it should be fine. |
|
Ok I try to provide something today |
|
@davsclaus May I use the same branch? Or should I create another one? |
|
yes sure you are welcome to use this branch or whatever you think is best |
|
Hi, I have created the CAMEL-19311 to discuss the LRU. I'm adding the issue here just to link this issue to the JIRA issue opened by me. |
| @@ -53,7 +54,7 @@ public <K, V> Map<K, V> createLRUCache(int maximumCacheSize) { | |||
| @Override | |||
| public <K, V> Map<K, V> createLRUCache(int maximumCacheSize, Consumer<V> onEvict) { | |||
| LOG.trace("Creating LRUCache with maximumCacheSize: {}", maximumCacheSize); | |||
| return new SimpleLRUCache<>(16, maximumCacheSize, onEvict); | |||
| return Collections.synchronizedMap(new SimpleLRUCache<>(16, maximumCacheSize, onEvict)); | |||
There was a problem hiding this comment.
I think it can impact code casting to SimpleLRUCache to use some SimpleLRUCache's methods.
|
@orpiske could you please try this? |
|
🚫 There are (likely) no components to be tested in this PR |
|
Second Chance (Clock) is very easy to implement to give LRU like hit rates, high read concurrency, and is a good fit for small custom caches. It is basically a FIFO with a mark bit that is set on read. On a write a global lock is acquired, the FIFO is scanned resetting the mark bit, and the first unset entry is chosen for eviction. This means the worst case is O(n), which is fine for a cache of a few thousand entries. As reads are the common case, writes that block on this lock to perform a small amount of work is acceptable. When brainstorming approaches for ConcurrentLinkedHashMap (Guava/Caffeine predecessor), that was my original approach that triggered my interest when solving some work performance problems. I had to release that as a pre-1.0 beta to mitigate people from using my unstable concurrent lru alpha code, so you can see review that as a reference and a basic analysis. |
|
Thanks @ben-manes for sharing your knowledge to the Camel community. The code for the Caffeine LRU Cache is here It may be that we can improve this and our situation with Camel is the cache sizes are small (1000) (never huge) And today maybe the warmup is not as needed (we run a lot of unit tests and start/stop Camel frequently, so fast startup in these test is preferred) |
|
Oh that’s great. I simply meant that if you want to make this existing lru cache concurrent without a dependency, then second chance is an effective approach that should be easy to maintain. Your pr changes also look reasonable. |
|
For warmup, a very long time ago the builder had a class loading issue because it selected the implementation by a string switch. That seemed to cause the factory to be slow to classload due to the constant pool. I don’t think it would have tried to load the implementations, but it was oddly visible. The cache uses codegen variants to minimize the memory footprint, e.g. have ttl entry fields only if expirable. That was fixed by using reflection which also trimmed the jar bloat. The current version can instantiate 180k/s. This has done in 2.6.1 (2017). An unreleased optimization cached that reflective load so it becomes a direct call on subsequent usages. That resulted in 4M/s. Happy to release it if the current time is still an issue and the snapshot jar shows an improvement. |
I'll try this one today. I think I can post the results by COB. |
|
Thanks a lot @ben-manes for sharing your knowledge. |
essobedo
force-pushed
the
lru-sync
branch
2 times, most recently
from
1b04d05 to
34184b5
Compare
|
@orpiske FYI, I've changed a bit the implementation for something easier to maintain and closer in terms of behavior to the initial non thread-safe implementation |
Thanks for the heads up. That's OK, the test should start in about 1 hour or so, so it should pick the updated one. |
essobedo
force-pushed
the
lru-sync
branch
3 times, most recently
from
83d9295 to
1c0a8e9
Compare
|
↩️ There are either too many changes to be tested in this PR or the code needs be rebased: (271 components likely to be affected) |
1 similar comment
|
↩️ There are either too many changes to be tested in this PR or the code needs be rebased: (271 components likely to be affected) |
|
↩️ There are either too many changes to be tested in this PR or the code needs be rebased: (271 components likely to be affected) |
4 similar comments
|
↩️ There are either too many changes to be tested in this PR or the code needs be rebased: (271 components likely to be affected) |
|
↩️ There are either too many changes to be tested in this PR or the code needs be rebased: (271 components likely to be affected) |
|
↩️ There are either too many changes to be tested in this PR or the code needs be rebased: (271 components likely to be affected) |
|
↩️ There are either too many changes to be tested in this PR or the code needs be rebased: (271 components likely to be affected) |
|
@essobedo Here's the results: Baseline (M3) Test Baseline Test Baseline: Test Baseline Test All in all, I think it's performing nicely! There's a few outliers (i.e.: like the |
|
Considering that the baseline is a non-thread-safe implementation that sounds good, thx @orpiske |
|
@ben-manes thx, for your priceless feedback, anything else to add regarding this PR? |
essobedo
changed the title
Indeed, thanks @ben-manes for the feedback! |
|
LGTM Great collaboration guys and thanks for all the performance tests. |
|
lgtm! |
|
Ok let's merge it and see how the tests go |
…se will OOME
Description
Target
camel-3.x, whereas Camel 4 uses themainbranch)Tracking
Apache Camel coding standards and style
mvn -Pformat,fastinstall install && mvn -Psourcecheck