From c976bd106fee422c4bfa6be75fff9b1d3426b01b Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Wed, 12 Apr 2023 15:17:17 -0400 Subject: [PATCH] Opt out of the TypeFactory cache until we can resolve cache contention https://github.com/FasterXML/jackson-databind/issues/3876 https://github.com/FasterXML/jackson-benchmarks/pull/5 The linked benchmarks show a small advantage of caching over not caching in most cases, in the best case for a cache: when a single operation is repetedly executed. In practice, the cache is used for all operations and sees heavy evictions, where contention is more likely and has greater impact on users. Given the hash collision issues that we're aware of, and the impacts we've observed, we will remove the cache until a version is available which isn't impacted by the linked issue for performance that's more predictable. --- .../build.gradle | 1 - .../CaffeineCachingTypeFactory.java | 217 ------------------ .../serialization/NonCachingTypeFactory.java | 110 +++++++++ .../java/serialization/ObjectMappers.java | 4 +- .../src/main/metrics/jackson-metrics.yml | 13 -- .../java/serialization/ObjectMappersTest.java | 33 +-- 6 files changed, 113 insertions(+), 265 deletions(-) delete mode 100644 conjure-java-jackson-serialization/src/main/java/com/palantir/conjure/java/serialization/CaffeineCachingTypeFactory.java create mode 100644 conjure-java-jackson-serialization/src/main/java/com/palantir/conjure/java/serialization/NonCachingTypeFactory.java diff --git a/conjure-java-jackson-serialization/build.gradle b/conjure-java-jackson-serialization/build.gradle index 308b018b5..d8e39b80b 100644 --- a/conjure-java-jackson-serialization/build.gradle +++ b/conjure-java-jackson-serialization/build.gradle @@ -14,7 +14,6 @@ dependencies { api "com.palantir.safe-logging:preconditions" implementation "com.palantir.safe-logging:logger" implementation 'com.palantir.tritium:tritium-registry' - implementation 'com.github.ben-manes.caffeine:caffeine' testImplementation "junit:junit" testImplementation "org.assertj:assertj-core" diff --git a/conjure-java-jackson-serialization/src/main/java/com/palantir/conjure/java/serialization/CaffeineCachingTypeFactory.java b/conjure-java-jackson-serialization/src/main/java/com/palantir/conjure/java/serialization/CaffeineCachingTypeFactory.java deleted file mode 100644 index adc9ce5dc..000000000 --- a/conjure-java-jackson-serialization/src/main/java/com/palantir/conjure/java/serialization/CaffeineCachingTypeFactory.java +++ /dev/null @@ -1,217 +0,0 @@ -/* - * (c) Copyright 2023 Palantir Technologies Inc. All rights reserved. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package com.palantir.conjure.java.serialization; - -import com.codahale.metrics.Meter; -import com.fasterxml.jackson.databind.JavaType; -import com.fasterxml.jackson.databind.type.TypeFactory; -import com.fasterxml.jackson.databind.type.TypeModifier; -import com.fasterxml.jackson.databind.type.TypeParser; -import com.fasterxml.jackson.databind.util.ArrayBuilders; -import com.fasterxml.jackson.databind.util.LRUMap; -import com.fasterxml.jackson.databind.util.LookupCache; -import com.github.benmanes.caffeine.cache.Cache; -import com.github.benmanes.caffeine.cache.Caffeine; -import com.github.benmanes.caffeine.cache.RemovalCause; -import com.github.benmanes.caffeine.cache.stats.CacheStats; -import com.github.benmanes.caffeine.cache.stats.StatsCounter; -import com.google.common.primitives.Ints; -import com.palantir.logsafe.SafeArg; -import com.palantir.logsafe.logger.SafeLogger; -import com.palantir.logsafe.logger.SafeLoggerFactory; -import com.palantir.tritium.metrics.registry.SharedTaggedMetricRegistries; -import com.palantir.tritium.metrics.registry.TaggedMetricRegistry; -import java.util.Locale; -import java.util.function.Supplier; -import javax.annotation.Nullable; -import org.checkerframework.checker.index.qual.NonNegative; - -final class CaffeineCachingTypeFactory extends TypeFactory { - private static final SafeLogger log = SafeLoggerFactory.get(CaffeineCachingTypeFactory.class); - - CaffeineCachingTypeFactory() { - super(new CaffeineLookupCache()); - } - - CaffeineCachingTypeFactory( - CaffeineLookupCache typeCache, - TypeParser parser, - @Nullable TypeModifier[] modifiers, - ClassLoader classLoader) { - super(typeCache, parser, modifiers, classLoader); - } - - private static CaffeineLookupCache asCaffeine(LookupCache typeCache) { - if (typeCache instanceof CaffeineLookupCache) { - return (CaffeineLookupCache) typeCache; - } else if (typeCache == null || typeCache.size() == 0) { - // Use a caffeine cache instead of the provided empty cache - return new CaffeineLookupCache(); - } else if (typeCache instanceof LRUMap) { - CaffeineLookupCache cache = new CaffeineLookupCache(); - LRUMap defaultJacksonCacheType = (LRUMap) typeCache; - defaultJacksonCacheType.contents(cache::put); - return cache; - } else { - log.error( - "Unknown LookupCache, using a new caffeine cache instead", - SafeArg.of("cacheType", typeCache.getClass())); - return new CaffeineLookupCache(); - } - } - - @Override - public CaffeineCachingTypeFactory withModifier(TypeModifier mod) { - // Updating modifiers clears the cache - return new CaffeineCachingTypeFactory( - new CaffeineLookupCache(), _parser, computeModifiers(_modifiers, mod), _classLoader); - } - - @Nullable - private static TypeModifier[] computeModifiers(TypeModifier[] existing, TypeModifier newModifier) { - // Semantics are based on the jackson-databind `withModifier` implementation. - if (newModifier == null) { - return null; - } - if (existing == null || existing.length == 0) { - return new TypeModifier[] {newModifier}; - } - return ArrayBuilders.insertInListNoDup(existing, newModifier); - } - - @Override - public CaffeineCachingTypeFactory withClassLoader(ClassLoader classLoader) { - return new CaffeineCachingTypeFactory(asCaffeine(_typeCache), _parser, _modifiers, classLoader); - } - - @Override - public CaffeineCachingTypeFactory withCache(LRUMap cache) { - return withCache((LookupCache) cache); - } - - @Override - public CaffeineCachingTypeFactory withCache(LookupCache cache) { - return new CaffeineCachingTypeFactory(asCaffeine(cache), _parser, _modifiers, _classLoader); - } - - private static final class CaffeineLookupCache implements LookupCache { - private final Cache cache; - - CaffeineLookupCache() { - this.cache = Caffeine.newBuilder() - // max-size 1000 up from 200 default as of 2.14.2 - .maximumSize(1000) - // initial-size 128 up from 16 default as of 2.14.2 - .initialCapacity(128) - .recordStats(InstrumentedStatsCounter.SUPPLIER) - .build(); - } - - @Override - public int size() { - return Ints.saturatedCast(cache.estimatedSize()); - } - - @Override - @Nullable - public JavaType get(Object key) { - return cache.getIfPresent(key); - } - - @Override - public JavaType put(Object key, JavaType value) { - return cache.asMap().put(key, value); - } - - @Override - public JavaType putIfAbsent(Object key, JavaType value) { - return cache.asMap().putIfAbsent(key, value); - } - - @Override - public void clear() { - cache.invalidateAll(); - } - - @Override - public String toString() { - return "CaffeineLookupCache{" + cache + '}'; - } - } - - private static final class InstrumentedStatsCounter implements StatsCounter { - // Collecting metrics without broadening APIs to require a TaggedMetricRegistry - @SuppressWarnings("deprecation") - private static final StatsCounter INSTANCE = - new InstrumentedStatsCounter(SharedTaggedMetricRegistries.getSingleton()); - - @SuppressWarnings("UnnecessaryLambda") - private static final Supplier SUPPLIER = () -> INSTANCE; - - private final Meter hits; - private final Meter misses; - // Eviction meters are based on RemovalCause ordinal - private final Meter[] evictions; - - private InstrumentedStatsCounter(TaggedMetricRegistry registry) { - JsonDatabindTypefactoryCacheMetrics metrics = JsonDatabindTypefactoryCacheMetrics.of(registry); - this.hits = metrics.hit(); - this.misses = metrics.miss(); - RemovalCause[] causes = RemovalCause.values(); - this.evictions = new Meter[causes.length]; - for (int i = 0; i < causes.length; i++) { - evictions[i] = metrics.eviction(causes[i].name().toLowerCase(Locale.ROOT)); - } - } - - @Override - public void recordHits(@NonNegative int count) { - hits.mark(count); - } - - @Override - public void recordMisses(@NonNegative int count) { - misses.mark(count); - } - - @Override - public void recordLoadSuccess(@NonNegative long _loadTime) { - // nop - } - - @Override - public void recordLoadFailure(@NonNegative long _loadTime) { - // nop - } - - @Override - public void recordEviction(@NonNegative int _weight, RemovalCause cause) { - evictions[cause.ordinal()].mark(); - } - - @Override - public CacheStats snapshot() { - // Item weight is always 1, evictions count and weight are always identical. - // We don't measure load success/failure/timing information. - long evictionsCount = 0; - for (Meter meter : evictions) { - evictionsCount += meter.getCount(); - } - return CacheStats.of(hits.getCount(), misses.getCount(), 0, 0, 0, evictionsCount, evictionsCount); - } - } -} diff --git a/conjure-java-jackson-serialization/src/main/java/com/palantir/conjure/java/serialization/NonCachingTypeFactory.java b/conjure-java-jackson-serialization/src/main/java/com/palantir/conjure/java/serialization/NonCachingTypeFactory.java new file mode 100644 index 000000000..335865c19 --- /dev/null +++ b/conjure-java-jackson-serialization/src/main/java/com/palantir/conjure/java/serialization/NonCachingTypeFactory.java @@ -0,0 +1,110 @@ +/* + * (c) Copyright 2023 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.palantir.conjure.java.serialization; + +import com.fasterxml.jackson.databind.JavaType; +import com.fasterxml.jackson.databind.type.TypeFactory; +import com.fasterxml.jackson.databind.type.TypeModifier; +import com.fasterxml.jackson.databind.type.TypeParser; +import com.fasterxml.jackson.databind.util.ArrayBuilders; +import com.fasterxml.jackson.databind.util.LRUMap; +import com.fasterxml.jackson.databind.util.LookupCache; +import javax.annotation.Nullable; + +/** + * A {@link TypeFactory} implementation which does not use a cache. + * @see jackson-databind#3876 + * @see jackson-benchmarks#5 + */ +final class NonCachingTypeFactory extends TypeFactory { + + NonCachingTypeFactory() { + super(NoCacheLookupCache.INSTANCE); + } + + NonCachingTypeFactory(TypeParser parser, @Nullable TypeModifier[] modifiers, ClassLoader classLoader) { + super(NoCacheLookupCache.INSTANCE, parser, modifiers, classLoader); + } + + @Override + public NonCachingTypeFactory withModifier(TypeModifier mod) { + return new NonCachingTypeFactory(_parser, computeModifiers(_modifiers, mod), _classLoader); + } + + @Nullable + private static TypeModifier[] computeModifiers(TypeModifier[] existing, TypeModifier newModifier) { + // Semantics are based on the jackson-databind `withModifier` implementation. + if (newModifier == null) { + return null; + } + if (existing == null || existing.length == 0) { + return new TypeModifier[] {newModifier}; + } + return ArrayBuilders.insertInListNoDup(existing, newModifier); + } + + @Override + public NonCachingTypeFactory withClassLoader(ClassLoader classLoader) { + return new NonCachingTypeFactory(_parser, _modifiers, classLoader); + } + + @Override + public NonCachingTypeFactory withCache(LRUMap _cache) { + // Changes to the cache are ignored + return this; + } + + @Override + public NonCachingTypeFactory withCache(LookupCache _cache) { + // Changes to the cache are ignored + return this; + } + + private enum NoCacheLookupCache implements LookupCache { + INSTANCE; + + @Override + public int size() { + return 0; + } + + @Override + @Nullable + public JavaType get(Object _key) { + return null; + } + + @Override + @Nullable + public JavaType put(Object _key, JavaType _value) { + return null; + } + + @Override + public JavaType putIfAbsent(Object _key, JavaType _value) { + return null; + } + + @Override + public void clear() {} + + @Override + public String toString() { + return "NoCacheLookupCache{}"; + } + } +} diff --git a/conjure-java-jackson-serialization/src/main/java/com/palantir/conjure/java/serialization/ObjectMappers.java b/conjure-java-jackson-serialization/src/main/java/com/palantir/conjure/java/serialization/ObjectMappers.java index f23551247..06462bffc 100644 --- a/conjure-java-jackson-serialization/src/main/java/com/palantir/conjure/java/serialization/ObjectMappers.java +++ b/conjure-java-jackson-serialization/src/main/java/com/palantir/conjure/java/serialization/ObjectMappers.java @@ -170,7 +170,7 @@ public static ObjectMapper newSmileServerObjectMapper() { * */ public static > B withDefaultModules(B builder) { - return builder.typeFactory(new CaffeineCachingTypeFactory()) + return builder.typeFactory(new NonCachingTypeFactory()) .addModule(new GuavaModule()) .addModule(new ShimJdk7Module()) .addModule(new Jdk8Module().configureAbsentsAsNulls(true)) @@ -205,7 +205,7 @@ public static > B withDefa * */ public static ObjectMapper withDefaultModules(ObjectMapper mapper) { - return mapper.setTypeFactory(new CaffeineCachingTypeFactory()) + return mapper.setTypeFactory(new NonCachingTypeFactory()) .registerModule(new GuavaModule()) .registerModule(new ShimJdk7Module()) .registerModule(new Jdk8Module().configureAbsentsAsNulls(true)) diff --git a/conjure-java-jackson-serialization/src/main/metrics/jackson-metrics.yml b/conjure-java-jackson-serialization/src/main/metrics/jackson-metrics.yml index ec3947fce..83c56308b 100644 --- a/conjure-java-jackson-serialization/src/main/metrics/jackson-metrics.yml +++ b/conjure-java-jackson-serialization/src/main/metrics/jackson-metrics.yml @@ -10,16 +10,3 @@ namespaces: tags: - format docs: Histogram describing the length of strings parsed from input. - json.databind.typefactory.cache: - docs: Metrics produced by the Jackson Databind TypeFactory cache. - metrics: - hit: - type: meter - docs: Rate at which cache lookups are successful. - miss: - type: meter - docs: Rate at which cache lookups miss and require computation. - eviction: - type: meter - docs: Rate at which cache entries are removed, tagged by the cause for removal. - tags: [reason] diff --git a/conjure-java-jackson-serialization/src/test/java/com/palantir/conjure/java/serialization/ObjectMappersTest.java b/conjure-java-jackson-serialization/src/test/java/com/palantir/conjure/java/serialization/ObjectMappersTest.java index 880567a88..94e321363 100644 --- a/conjure-java-jackson-serialization/src/test/java/com/palantir/conjure/java/serialization/ObjectMappersTest.java +++ b/conjure-java-jackson-serialization/src/test/java/com/palantir/conjure/java/serialization/ObjectMappersTest.java @@ -20,7 +20,6 @@ import static org.assertj.core.api.Assertions.assertThatThrownBy; import com.codahale.metrics.Histogram; -import com.codahale.metrics.Meter; import com.fasterxml.jackson.annotation.JsonProperty; import com.fasterxml.jackson.core.JsonFactory; import com.fasterxml.jackson.core.JsonParseException; @@ -31,7 +30,6 @@ import com.fasterxml.jackson.databind.ObjectMapper; import com.fasterxml.jackson.databind.exc.InvalidFormatException; import com.fasterxml.jackson.databind.json.JsonMapper; -import com.fasterxml.jackson.databind.type.TypeFactory; import com.fasterxml.jackson.dataformat.smile.SmileFactory; import com.palantir.logsafe.Preconditions; import com.palantir.tritium.metrics.registry.SharedTaggedMetricRegistries; @@ -425,36 +423,7 @@ public void testTypeFactoryCache() { } private void testTypeFactory(ObjectMapper mapper) { - assertThat(mapper.getTypeFactory()).isInstanceOf(CaffeineCachingTypeFactory.class); - } - - @Test - public void testTypeFactoryCacheMetrics() { - TaggedMetricRegistry registry = SharedTaggedMetricRegistries.getSingleton(); - JsonDatabindTypefactoryCacheMetrics metrics = JsonDatabindTypefactoryCacheMetrics.of(registry); - Meter hit = metrics.hit(); - Meter miss = metrics.miss(); - TypeFactory typeFactory = ObjectMappers.newServerJsonMapper().getTypeFactory(); - - long hitBeforeFirst = hit.getCount(); - long missBeforeFirst = miss.getCount(); - typeFactory.constructType(SimpleSerializable.class); - assertThat(miss.getCount() - missBeforeFirst).isOne(); - assertThat(hit.getCount() - hitBeforeFirst).isZero(); - // After writing the same type again, we should observe hits and no additional misses. - long hitBeforeSecond = hit.getCount(); - long missBeforeSecond = miss.getCount(); - typeFactory.constructType(SimpleSerializable.class); - assertThat(miss.getCount() - missBeforeSecond).isZero(); - assertThat(hit.getCount() - hitBeforeSecond).isOne(); - } - - static final class SimpleSerializable { - @Override - @JsonProperty("str") - public String toString() { - return "stringValue"; - } + assertThat(mapper.getTypeFactory()).isInstanceOf(NonCachingTypeFactory.class); } @Test