Add a caching API based on caffeine for use from instrumentation, not just javaagent #2477
Conversation
anuraaga
requested review from
iNikem,
jkwatson,
mateuszrzeszutek,
pavolloffay,
trask and
tylerbenson
as code owners
|
@ben-manes After I experimented with gradle-shadow-plugin's minimization feature, I found that it's only about a 350K dependency to add caffeine for our purposes, so we're going with it instead of the concurrentlinkedhashmap. If you have a chance to skim this to see if it's a sane approach, that would be much appreciated. |
| }); | ||
| assertThat(cache.computeIfAbsent(dog, unused -> "bark")).isEqualTo("bark"); | ||
| dog = null; | ||
| System.gc(); |
There was a problem hiding this comment.
GcFinalizable from guava testlib is great for this as more predictable
There was a problem hiding this comment.
Thanks for the hint! I've always wondered about this but in practice have yet to see a failure due to only calling gc once and continue with this pattern. Looking at it
I feel as if it would be mostly the same to move System.gc into the await() predicate, possibly tweaking the polling interval, which is nice to reduce the number of ways of doing something. Might do one or the other if this flakes in the future.
|
That’s great! |
| } | ||
| jar { | ||
| from(sourceSets.patch.output) { | ||
| include 'com/github/benmanes/caffeine/cache/BoundedLocalCache$PerformCleanupTask.class' |
There was a problem hiding this comment.
Will this file be shaded too? Also, why javaagent-bootstrap and not the new module?
There was a problem hiding this comment.
Oh good catch, I don't think this is doing anything because of the non shading (I'll need to come up with a bespoke test for this I guess). It's here since only the agent cares, not library instrumentation.
There was a problem hiding this comment.
For additional reference there is a gap here vs datadog - they keep Caffeine in the agent classloader but we are shading and putting in the bootstrap classloader instead to allow the same instrumentation to work for both library and agent.
There was a problem hiding this comment.
I fixed the package name but couldn't come up with a way to add a test for this. Anyone have any ideas?
There was a problem hiding this comment.
Was able to come up with a test, a bit annoying since I need a different Gradle project to be able to check the patched jar itself.
| @SuppressWarnings("unchecked") | ||
| com.github.benmanes.caffeine.cache.Cache<K, V> delegate = | ||
| (com.github.benmanes.caffeine.cache.Cache<K, V>) caffeine.build(); |
There was a problem hiding this comment.
Why not add <K, V> as type parameters of CacheBuilder?
There was a problem hiding this comment.
Yeah Caffeine has them - since we don't have loading cache at least for now I couldn't find a reason to have them on the builder. This is the same trick Caffeine uses to convert Caffeine<Object, Object> to Cache<K, V>.
| assertThat(caffeineCache.keySet()).hasSize(1); | ||
|
|
||
| assertThat(cache.computeIfAbsent("dog", unused -> "bark")).isEqualTo("bark"); | ||
| caffeineCache.cleanup(); |
There was a problem hiding this comment.
Just wondering: what does cleanup() exactly do here, does it remove values over max size?
Does this test pass without it? Would adding it to unbounded() make that test fail?
There was a problem hiding this comment.
Yup it ensures values over max are removed since Caffeine does that asynchronously. Without it out fails, but for non evictions it's not needed
There was a problem hiding this comment.
If you prefer, set the executor to Runnable::run for a direct call. The async isn’t needed for the cache’s own maintenance as that is fast, but we have optional callbacks to user code so it’s protected if that is slow.
There was a problem hiding this comment.
@ben-manes Thanks for the tip! Going to leave it as is for now to use the defaults as a baseline. Will set up a simple benchmark for our own sanity checking later, compare the two, and then pick one.
…nstrumentation into caching-api
…nstrumentation into caching-api
For now just adds the most minimal API,
computeIfAbsent.