Use Caffeine for weak maps #2601
Conversation
…nstrumentation into caffeine-weakmap
anuraaga
requested review from
iNikem,
jkwatson,
mateuszrzeszutek,
pavolloffay,
trask and
tylerbenson
as code owners
…nstrumentation into caffeine-weakmap
| @@ -55,6 +65,9 @@ void bounded() { | |||
| void unbounded() { | |||
| Cache<String, String> cache = Cache.newBuilder().setWeakKeys().build(); | |||
|
|
|||
| assertThat(cache.computeIfAbsent("bear", unused -> "roar")).isEqualTo("roar"); | |||
| cache.remove("bear"); | |||
|
|
|||
| CaffeineCache<?, ?> caffeineCache = ((CaffeineCache<?, ?>) cache); | |||
| String cat = new String("cat"); | |||
There was a problem hiding this comment.
I detect that this code is problematic. According to the Performance (PERFORMANCE), Dm: Method invokes inefficient new String(String) constructor (DM_STRING_CTOR).
Using the java.lang.String(String) constructor wastes memory because the object so constructed will be functionally indistinguishable from the String passed as a parameter. Just use the argument String directly.
There was a problem hiding this comment.
@lillieMaiBauer Can you please disable this organization from your code review bot? We have satisfactory checks in place, including exclusions that need to be made in some cases like this.
| @@ -125,7 +126,7 @@ public static HelperInjector forDynamicTypes( | |||
| classLoader = BOOTSTRAP_CLASSLOADER_PLACEHOLDER; | |||
| } | |||
|
|
|||
| if (!injectedClassLoaders.containsKey(classLoader)) { | |||
| if (Boolean.FALSE == injectedClassLoaders.get(classLoader)) { | |||
There was a problem hiding this comment.
I detect that this code is problematic. According to the Bad practice (BAD_PRACTICE), RC: Suspicious reference comparison of Boolean values (RC_REF_COMPARISON_BAD_PRACTICE_BOOLEAN).
This method compares two Boolean values using the == or != operator. Normally, there are only two Boolean values (Boolean.TRUE and Boolean.FALSE), but it is possible to create other Boolean objects using the new Boolean(b) constructor. It is best to avoid such objects, but if they do exist, then checking Boolean objects for equality using == or != will give results than are different than you would get using .equals(...).
| @Threads(5) | ||
| public void caffeineMap_fiveThreads() { | ||
| caffeineMap.put(key, "foo"); | ||
| caffeineMap.get(key); |
There was a problem hiding this comment.
Inject Blackhole and use returned value. Otherwise who knows what compiler will do
| @@ -1,127 +1,127 @@ | |||
|
|
|||
| #javaagent | |||
| ##Dependency License Report | |||
|
Looking at dd-trace more closely, I noticed they still use weaklockfree for unbounded, so I've restored it to use to reduce gaps - there seem to be some tradeoffs but in our common case of adding a field to a user object, we'd expect low contention and weaklockfree is probably a bit better. |
| @@ -18,35 +22,15 @@ dependencies { | |||
| } | |||
|
|
|||
| jmh { | |||
| timeUnit = 'ms' // Output time unit. Available time units are: [m, s, ms, us, ns]. | |||
There was a problem hiding this comment.
I removed these since they override annotations. Benchmarks often need specific settings so copy-pasta of annotations is better than global control
| private static final WeakConcurrentMap<String, String> weakConcurrentMap = | ||
| new WeakConcurrentMap<>(true, true); | ||
|
|
||
| private static final WeakConcurrentMap<String, String> weakConcurrentMapInline = |
There was a problem hiding this comment.
Added inline expunction benchmark too. Found very little difference in performance so settled on it since it means not having to worry about cleaner threads which are hard to manage for library instrumentation (we were using a common task executor before)
|
|
||
| @Override | ||
| public V computeIfAbsent(K key, Function<? super K, ? extends V> mappingFunction) { | ||
| V value = get(key); |
There was a problem hiding this comment.
Implementation mostly same as WeakMapSuppliers before it, though it's using putIfAbsent so a bit better I think.
|
Made some fairly large changes to continue to use weaklockfree for unbounded like dd-trace, PTAL |
| @@ -18,35 +22,15 @@ dependencies { | |||
| } | |||
|
|
|||
| jmh { | |||
| timeUnit = 'ms' // Output time unit. Available time units are: [m, s, ms, us, ns]. | |||
| boolean value = hasResources(cl); | ||
| cache.put(cl, value); | ||
| return value; | ||
| return cache.computeIfAbsent(cl, this::hasResources); |
| // We only have compileOnly dependencies on these to make sure they don't leak into POMs. | ||
| licenseReportDependencies(deps.caffeine) { | ||
| transitive = false | ||
| } | ||
| licenseReportDependencies deps.weaklockfree |
I'd like to use weak maps in library instrumentation too in some cases and we already have it as part of the caching API. I checked some very basic benchmarks and it looks like weaklockfree could be a bit better at low contention with caffeine better at high contention, though they're pretty similar. I did notice with Datadog that they seem to use Caffeine for Java 8+ and presumably benchmarked much better than me so think this is ok. DataDog/dd-trace-java#2044 It wouldn't be hard to switch to weaklockfree in the future though if needed.
Edit: squashed just has a silly bug