From 4f2db82056323cf6634c2c075fd7ab198475f142 Mon Sep 17 00:00:00 2001 From: evanchooly Date: Wed, 14 Oct 2026 23:16:39 -0400 Subject: [PATCH] Cache entity codecs in a ConcurrentHashMap (plan item 2.8) MorphiaCodecProvider cached codecs in a plain HashMap that concurrent lookups mutated, and each thread that missed built and stored its own codec. It now uses a ConcurrentHashMap with putIfAbsent, so every caller shares the first codec stored. Not computeIfAbsent, because building a codec looks up other codecs that can land back in this provider. --- .../mapping/codec/MorphiaCodecProvider.java | 11 ++++-- .../codec/harness/CodecHarnessTest.java | 36 +++++++++++++++++++ design/mapping-game-plan.md | 2 +- 3 files changed, 45 insertions(+), 4 deletions(-) diff --git a/core/src/main/java/dev/morphia/mapping/codec/MorphiaCodecProvider.java b/core/src/main/java/dev/morphia/mapping/codec/MorphiaCodecProvider.java index 575711c54ad..13b93abe0b4 100644 --- a/core/src/main/java/dev/morphia/mapping/codec/MorphiaCodecProvider.java +++ b/core/src/main/java/dev/morphia/mapping/codec/MorphiaCodecProvider.java @@ -1,10 +1,10 @@ package dev.morphia.mapping.codec; import java.util.ArrayList; -import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.ServiceLoader; +import java.util.concurrent.ConcurrentHashMap; import com.mongodb.lang.Nullable; @@ -35,7 +35,8 @@ */ @MorphiaInternal public class MorphiaCodecProvider implements CodecProvider { - private final Map, Codec> codecs = new HashMap<>(); + // Looked up concurrently. Not computeIfAbsent: building a codec looks up other codecs, which may land back here. + private final Map, Codec> codecs = new ConcurrentHashMap<>(); private final Mapper mapper; private final Conversions conversions; private final List propertyCodecProviders = new ArrayList<>(); @@ -74,7 +75,11 @@ public Codec get(Class type, CodecRegistry registry) { if (model.hasLifecycle(PreLoad.class) || model.hasLifecycle(PostLoad.class) || mapper.hasListeners()) { codec.setDecoder(new LifecycleDecoder(codec)); } - codecs.put(type, codec); + // another thread may have built one meanwhile; keep the first so every caller shares it + MorphiaCodec existing = (MorphiaCodec) codecs.putIfAbsent(type, codec); + if (existing != null) { + codec = existing; + } } return codec; diff --git a/core/src/test/java/dev/morphia/test/mapping/codec/harness/CodecHarnessTest.java b/core/src/test/java/dev/morphia/test/mapping/codec/harness/CodecHarnessTest.java index 3014e3c136d..f333cc40299 100644 --- a/core/src/test/java/dev/morphia/test/mapping/codec/harness/CodecHarnessTest.java +++ b/core/src/test/java/dev/morphia/test/mapping/codec/harness/CodecHarnessTest.java @@ -13,6 +13,10 @@ import java.util.List; import java.util.Map; import java.util.Set; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; import java.util.concurrent.TimeUnit; import java.util.function.Supplier; @@ -25,6 +29,7 @@ import dev.morphia.config.MorphiaConfig; import dev.morphia.mapping.DiscriminatorFunction; import dev.morphia.mapping.MapperType; +import dev.morphia.mapping.codec.MorphiaCodecProvider; import dev.morphia.mapping.codec.pojo.EntityModel; import dev.morphia.mapping.codec.pojo.PropertyModel; import dev.morphia.mapping.codec.pojo.critter.CritterEntityModel; @@ -72,6 +77,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertInstanceOf; import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertSame; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.fail; @@ -226,6 +232,36 @@ void modelHashesAreStableAcrossEncoding() { } } + @Test + void concurrentCodecLookupsShareOneCodec() throws Exception { + int threads = 8; + ExecutorService executor = Executors.newFixedThreadPool(threads); + try { + for (MapperType mapper : MapperType.values()) { + MorphiaDatastore datastore = datastore(Variant.DEFAULTS, mapper); + for (int round = 0; round < 20; round++) { + MorphiaCodecProvider provider = new MorphiaCodecProvider(datastore); + CountDownLatch start = new CountDownLatch(1); + List>> results = new ArrayList<>(); + for (int i = 0; i < threads; i++) { + results.add(executor.submit(() -> { + start.await(); + return provider.get(HarnessScalars.class, datastore.getCodecRegistry()); + })); + } + start.countDown(); + + Codec first = results.get(0).get(); + for (Future> result : results) { + assertSame(first, result.get(), mapper + ": every lookup should get the one cached codec"); + } + } + } + } finally { + executor.shutdownNow(); + } + } + @Test void readsLoadOnlyProperties() { // @LoadOnly values are never written, so the fixtures can't show that they are read diff --git a/design/mapping-game-plan.md b/design/mapping-game-plan.md index 5de9f9f86b2..2781a8ec920 100644 --- a/design/mapping-game-plan.md +++ b/design/mapping-game-plan.md @@ -179,7 +179,7 @@ structural item covers are moved to that item (see Phases 4 and 5). This also removes the deep `equals` cost on `CopyOnWriteArraySet.add` and in `EntityEncoder`. - Audit tests for any that rely on structural equality. -### 2.8 `MorphiaCodecProvider.codecs` is a plain `HashMap` (D8) +### ~~2.8 `MorphiaCodecProvider.codecs` is a plain `HashMap` (D8)~~ - Switch to `ConcurrentHashMap`, using `get` + `putIfAbsent`. Not `computeIfAbsent`, because codec construction recurses into other codec lookups.