Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify CodecCache #1018

Merged
merged 5 commits into from
Oct 18, 2022
Merged

Simplify CodecCache #1018

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b2ec80a
Simplify CodecCache
stIncMale Oct 17, 2022
35ccec5
Fix the failing test
stIncMale Oct 18, 2022
c4f2ff6
Fix the failing test
stIncMale Oct 18, 2022
12528a8
Cut more wobbling code down
stIncMale Oct 18, 2022
0ff9759
Revert "Cut more wobbling code down"
stIncMale Oct 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 11 additions & 23 deletions bson/src/main/org/bson/internal/CodecCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
package org.bson.internal;

import org.bson.codecs.Codec;
import org.bson.codecs.configuration.CodecConfigurationException;

import java.lang.reflect.Type;
import java.util.List;
Expand All @@ -26,7 +25,7 @@
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentMap;

import static java.lang.String.format;
import static org.bson.assertions.Assertions.assertNotNull;

final class CodecCache {

Expand Down Expand Up @@ -65,29 +64,18 @@ public String toString() {
}
}

private final ConcurrentMap<CodecCacheKey, Optional<Codec<?>>> codecCache = new ConcurrentHashMap<>();
private final ConcurrentMap<CodecCacheKey, Codec<?>> codecCache = new ConcurrentHashMap<>();

public boolean containsKey(final CodecCacheKey codecCacheKey) {
return codecCache.containsKey(codecCacheKey);
public <T> Codec<T> putIfAbsent(final CodecCacheKey codecCacheKey, final Codec<T> codec) {
assertNotNull(codec);
@SuppressWarnings("unchecked")
Codec<T> prevCodec = (Codec<T>) codecCache.putIfAbsent(codecCacheKey, codec);
return prevCodec == null ? codec : prevCodec;
}

public void put(final CodecCacheKey codecCacheKey, final Codec<?> codec){
codecCache.put(codecCacheKey, Optional.ofNullable(codec));
}

@SuppressWarnings("unchecked")
public synchronized <T> Codec<T> putIfMissing(final CodecCacheKey codecCacheKey, final Codec<T> codec) {
Optional<Codec<?>> cachedCodec = codecCache.computeIfAbsent(codecCacheKey, clz -> Optional.of(codec));
if (cachedCodec.isPresent()) {
return (Codec<T>) cachedCodec.get();
}
codecCache.put(codecCacheKey, Optional.of(codec));
return codec;
}
Comment on lines -79 to -86
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of improving this method as I originally described here, I did more changes, further simplifying the cache. Before this PR we were caching failures of finding a codec as Optional.empty() values just for the sake of throwing an exception faster if asked for the same codec again. I see no point in trying to optimize that path.


@SuppressWarnings("unchecked")
public <T> Codec<T> getOrThrow(final CodecCacheKey codecCacheKey) {
return (Codec<T>) codecCache.getOrDefault(codecCacheKey, Optional.empty()).orElseThrow(
() -> new CodecConfigurationException(format("Can't find a codec for %s.", codecCacheKey)));
public <T> Optional<Codec<T>> get(final CodecCacheKey codecCacheKey) {
@SuppressWarnings("unchecked")
Codec<T> codec = (Codec<T>) codecCache.get(codecCacheKey);
return Optional.ofNullable(codec);
}
}
10 changes: 5 additions & 5 deletions bson/src/main/org/bson/internal/ProvidersCodecRegistry.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import org.bson.codecs.Codec;
import org.bson.codecs.Parameterizable;
import org.bson.codecs.configuration.CodecConfigurationException;
import org.bson.codecs.configuration.CodecProvider;
import org.bson.codecs.configuration.CodecRegistry;
import org.bson.internal.CodecCache.CodecCacheKey;
Expand Down Expand Up @@ -67,19 +68,18 @@ public <T> Codec<T> get(final Class<T> clazz, final CodecRegistry registry) {
@SuppressWarnings({"unchecked"})
public <T> Codec<T> get(final ChildCodecRegistry<T> context) {
CodecCacheKey codecCacheKey = new CodecCacheKey(context.getCodecClass(), context.getTypes().orElse(null));
if (!codecCache.containsKey(codecCacheKey)) {
return codecCache.<T>get(codecCacheKey).orElseGet(() -> {
for (CodecProvider provider : codecProviders) {
Codec<T> codec = provider.get(context.getCodecClass(), context);
if (codec != null) {
if (codec instanceof Parameterizable && context.getTypes().isPresent()) {
codec = (Codec<T>) ((Parameterizable) codec).parameterize(context, context.getTypes().get());
}
return codecCache.putIfMissing(codecCacheKey, codec);
return codecCache.putIfAbsent(codecCacheKey, codec);
Comment on lines +71 to +78
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have just realized that this whole thing can and should be done as a single compute. Will do.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out, IllegalStateException: Recursive update is a thing despite the map being concurrent :( Well, we can't have all the good things.

}
}
codecCache.put(codecCacheKey, null);
}
return codecCache.getOrThrow(codecCacheKey);
throw new CodecConfigurationException(format("Can't find a codec for %s.", codecCacheKey));
});
}

@Override
Expand Down
19 changes: 8 additions & 11 deletions bson/src/test/unit/org/bson/internal/CodecCacheSpecification.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
package org.bson.internal

import org.bson.codecs.MinKeyCodec
import org.bson.codecs.configuration.CodecConfigurationException
import org.bson.types.MinKey
import spock.lang.Specification

Expand All @@ -28,40 +27,38 @@ class CodecCacheSpecification extends Specification {
def codec = new MinKeyCodec()
def cache = new CodecCache()
def cacheKey = new CodecCache.CodecCacheKey(MinKey, null)
cache.put(cacheKey, codec)
cache.putIfAbsent(cacheKey, codec)

then:
cache.getOrThrow(cacheKey).is(codec)
cache.get(cacheKey).get().is(codec)
}

def 'should throw if codec for class does not exist'() {
def 'should return empty if codec for class does not exist'() {
when:
def cache = new CodecCache()
def cacheKey = new CodecCache.CodecCacheKey(MinKey, null)
cache.getOrThrow(cacheKey)

then:
thrown(CodecConfigurationException)
!cache.get(cacheKey).isPresent()
}

def 'should return the cached codec if a codec for the parameterized class exists'() {
when:
def codec = new MinKeyCodec()
def cache = new CodecCache()
def cacheKey = new CodecCache.CodecCacheKey(List, [Integer])
cache.put(cacheKey, codec)
cache.putIfAbsent(cacheKey, codec)

then:
cache.getOrThrow(cacheKey).is(codec)
cache.get(cacheKey).get().is(codec)
}

def 'should throw if codec for the parameterized class does not exist'() {
def 'should return empty if codec for the parameterized class does not exist'() {
when:
def cache = new CodecCache()
def cacheKey = new CodecCache.CodecCacheKey(List, [Integer])
cache.getOrThrow(cacheKey)

then:
thrown(CodecConfigurationException)
!cache.get(cacheKey).isPresent()
}
}
25 changes: 17 additions & 8 deletions bson/src/test/unit/org/bson/internal/ProvidersCodecRegistrySpecification.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,22 +115,32 @@ class ProvidersCodecRegistrySpecification extends Specification {

def 'get should use the codecCache'() {
given:
def provider = Mock(CodecProvider)
def codec = Mock(Codec)
def provider = new CodecProvider() {
private int counter = 0

@Override
Codec get(final Class clazz, final CodecRegistry registry) {
if (counter == 0) {
counter++
return codec
}
throw new AssertionError((Object)'Must not be called more than once.')
}
}
Comment on lines +118 to +130
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason, I failed to use Spock stubbing here (the stabbed provider works as expected if used directly from the test, but does not work when used via registry. So I had to stub the object manually.


when:
def registry = new ProvidersCodecRegistry([provider])
registry.get(MinKey)
def codecFromRegistry = registry.get(MinKey)

then:
thrown(CodecConfigurationException)
1 * provider.get(MinKey, _)
codecFromRegistry == codec

when:
registry.get(MinKey)
codecFromRegistry = registry.get(MinKey)

then:
thrown(CodecConfigurationException)
0 * provider.get(MinKey, _)
codecFromRegistry == codec
}

def 'get with codec registry should return the codec from the first source that has one'() {
Expand Down Expand Up @@ -537,4 +547,3 @@ class Nested {
class Simple {
int value = 0
}