Fix concurrency and performance issues (#14)

Fixed concurrency and performance issues, replaced Map.containsKey with .get, cleaned up Type.rewrite a bit.
Mojang · Oct 16, 2018 · 6740eb6 · 6740eb6
1 parent d1eff04
commit 6740eb6
Show file tree

Hide file tree

Showing 9 changed files with 67 additions and 38 deletions.
diff --git a/src/main/java/com/mojang/datafixers/DataFixerUpper.java b/src/main/java/com/mojang/datafixers/DataFixerUpper.java
@@ -7,6 +7,7 @@
 import it.unimi.dsi.fastutil.ints.Int2ObjectSortedMap;
 import it.unimi.dsi.fastutil.ints.IntSortedSet;
 import it.unimi.dsi.fastutil.longs.Long2ObjectMap;
+import it.unimi.dsi.fastutil.longs.Long2ObjectMaps;
 import it.unimi.dsi.fastutil.longs.Long2ObjectOpenHashMap;
 import com.mojang.datafixers.functions.PointFreeRule;
 import com.mojang.datafixers.schemas.Schema;
@@ -67,7 +68,7 @@ public class DataFixerUpper implements DataFixer {
     private final Int2ObjectSortedMap<Schema> schemas;
     private final List<DataFix> globalList;
     private final IntSortedSet fixerVersions;
-    private final Long2ObjectMap<TypeRewriteRule> rules = new Long2ObjectOpenHashMap<>();
+    private final Long2ObjectMap<TypeRewriteRule> rules = Long2ObjectMaps.synchronize(new Long2ObjectOpenHashMap<>());
 
     protected DataFixerUpper(final Int2ObjectSortedMap<Schema> schemas, final List<DataFix> globalList, final IntSortedSet fixerVersions) {
         this.schemas = schemas;
@@ -127,7 +128,7 @@ protected TypeRewriteRule getRule(final int version, final int dataVersion) {
         final int expandedDataVersion = DataFixUtils.makeKey(dataVersion);
 
         final long key = (long) expandedVersion << 32 | expandedDataVersion;
-        if (!rules.containsKey(key)) {
+        return rules.computeIfAbsent(key, k -> {
             final List<TypeRewriteRule> rules = Lists.newArrayList();
             for (final DataFix fix : globalList) {
                 final int fixVersion = fix.getVersionKey();
@@ -139,9 +140,9 @@ protected TypeRewriteRule getRule(final int version, final int dataVersion) {
                     rules.add(fixRule);
                 }
             }
-            this.rules.put(key, TypeRewriteRule.seq(rules));
-        }
-        return rules.get(key);
+
+            return TypeRewriteRule.seq(rules);
+        });
     }
 
     protected IntSortedSet fixerVersions() {

diff --git a/src/main/java/com/mojang/datafixers/NamedChoiceFinder.java b/src/main/java/com/mojang/datafixers/NamedChoiceFinder.java
@@ -73,8 +73,8 @@ public Matcher(final String name, final Type<FT> type, final Type<FR> resultType
             }*/
             if (targetType instanceof TaggedChoice.TaggedChoiceType<?>) {
                 final TaggedChoice.TaggedChoiceType<?> choiceType = (TaggedChoice.TaggedChoiceType<?>) targetType;
-                if (choiceType.types().containsKey(name)) {
-                    final Type<?> elementType = choiceType.types().get(name);
+                final Type<?> elementType = choiceType.types().get(name);
+                if (elementType != null) {
                     if (!Objects.equals(type, elementType)) {
                         return Either.right(new Type.FieldNotFoundException(String.format("Type error for choice type \"%s\": expected type: %s, actual type: %s)", name, targetType, elementType)));
                     }

diff --git a/src/main/java/com/mojang/datafixers/functions/PointFree.java b/src/main/java/com/mojang/datafixers/functions/PointFree.java
@@ -11,15 +11,19 @@
 import java.util.function.Function;
 
 public abstract class PointFree<T> {
-    private boolean initialized;
+    private volatile boolean initialized;
     @Nullable
     private Function<DynamicOps<?>, T> value;
 
     @SuppressWarnings("ConstantConditions")
     public Function<DynamicOps<?>, T> evalCached() {
         if (!initialized) {
-            initialized = true;
-            value = eval();
+            synchronized (this) {
+                if (!initialized) {
+                    value = eval();
+                    initialized = true;
+                }
+            }
         }
         return value;
     }

diff --git a/src/main/java/com/mojang/datafixers/schemas/Schema.java b/src/main/java/com/mojang/datafixers/schemas/Schema.java
@@ -4,8 +4,6 @@
 
 import com.google.common.collect.Lists;
 import com.google.common.collect.Maps;
-import it.unimi.dsi.fastutil.objects.Object2IntMap;
-import it.unimi.dsi.fastutil.objects.Object2IntOpenHashMap;
 import com.mojang.datafixers.DSL;
 import com.mojang.datafixers.DataFixUtils;
 import com.mojang.datafixers.types.Type;
@@ -14,6 +12,8 @@
 import com.mojang.datafixers.types.templates.RecursivePoint;
 import com.mojang.datafixers.types.templates.TaggedChoice;
 import com.mojang.datafixers.types.templates.TypeTemplate;
+import it.unimi.dsi.fastutil.objects.Object2IntMap;
+import it.unimi.dsi.fastutil.objects.Object2IntOpenHashMap;
 
 import java.util.List;
 import java.util.Map;
@@ -52,8 +52,9 @@ protected Map<String, Type<?>> buildTypes() {
 
         for (final String name : TYPE_TEMPLATES.keySet()) {
             final Type<?> type;
-            if (RECURSIVE_TYPES.containsKey(name)) {
-                type = family.apply(RECURSIVE_TYPES.getInt(name));
+            final int recurseId = RECURSIVE_TYPES.getOrDefault(name, -1);
+            if (recurseId != -1) {
+                type = family.apply(recurseId);
             } else {
                 type = getTemplate(name).apply(family).apply(-1);
             }
@@ -91,8 +92,9 @@ public TypeTemplate resolveTemplate(final String name) {
     }
 
     public TypeTemplate id(final String name) {
-        if (RECURSIVE_TYPES.containsKey(name)) {
-            return DSL.id(RECURSIVE_TYPES.get(name));
+        final int id = RECURSIVE_TYPES.getOrDefault(name, -1);
+        if (id != -1) {
+            return DSL.id(id);
         }
         return getTemplate(name);
     }

diff --git a/src/main/java/com/mojang/datafixers/types/DynamicOps.java b/src/main/java/com/mojang/datafixers/types/DynamicOps.java
@@ -152,12 +152,7 @@ default Optional<T> get(final T input, final String key) {
     }
 
     default Optional<T> getGeneric(final T input, final T key) {
-        return getMapValues(input).flatMap(map -> {
-            if (map.containsKey(key)) {
-                return Optional.of(map.get(key));
-            }
-            return Optional.empty();
-        });
+        return getMapValues(input).flatMap(map -> Optional.ofNullable(map.get(key)));
     }
 
     default T set(final T input, final String key, final T value) {

diff --git a/src/main/java/com/mojang/datafixers/types/Type.java b/src/main/java/com/mojang/datafixers/types/Type.java
@@ -3,8 +3,6 @@
 package com.mojang.datafixers.types;
 
 import com.google.common.collect.Maps;
-import com.mojang.datafixers.util.Either;
-import com.mojang.datafixers.util.Pair;
 import com.mojang.datafixers.DSL;
 import com.mojang.datafixers.DataFixUtils;
 import com.mojang.datafixers.Dynamic;
@@ -23,14 +21,19 @@
 import com.mojang.datafixers.types.families.RecursiveTypeFamily;
 import com.mojang.datafixers.types.templates.TaggedChoice;
 import com.mojang.datafixers.types.templates.TypeTemplate;
+import com.mojang.datafixers.util.Either;
+import com.mojang.datafixers.util.Pair;
 import org.apache.commons.lang3.tuple.Triple;
 
 import javax.annotation.Nullable;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Optional;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.atomic.AtomicReference;
 
 public abstract class Type<A> implements App<Type.Mu, A> {
+    private static final Map<Triple<Type<?>, TypeRewriteRule, PointFreeRule>, CompletableFuture<Optional<? extends RewriteResult<?, ?>>>> PENDING_REWRITE_CACHE = Maps.newConcurrentMap();
     private static final Map<Triple<Type<?>, TypeRewriteRule, PointFreeRule>, Optional<? extends RewriteResult<?, ?>>> REWRITE_CACHE = Maps.newConcurrentMap();
 
     public static class Mu implements K1 {}
@@ -157,11 +160,29 @@ public <T, B> T capWrite(final DynamicOps<T> ops, final Type<?> expectedType, fi
     @SuppressWarnings("unchecked")
     public Optional<RewriteResult<A, ?>> rewrite(final TypeRewriteRule rule, final PointFreeRule fRule) {
         final Triple<Type<?>, TypeRewriteRule, PointFreeRule> key = Triple.of(this, rule, fRule);
-        if (!REWRITE_CACHE.containsKey(key)) {
-            final Optional<? extends RewriteResult<?, ?>> result = rule.rewrite(this).flatMap(r -> r.view().rewrite(fRule).map(view -> RewriteResult.create(view, r.recData())));
+        // This code under contention would generate multiple rewrites, so we use CompletableFuture for pending rewrites.
+        // We can not use computeIfAbsent because this is a recursive call that will block server startup
+        // during the Bootstrap phrase that's trying to pre cache these rewrites.
+        final Optional<? extends RewriteResult<?, ?>> rewrite = REWRITE_CACHE.get(key);
+        if (rewrite != null) {
+            return (Optional<RewriteResult<A, ?>>) rewrite;
+        }
+        final AtomicReference<CompletableFuture<Optional<? extends RewriteResult<?, ?>>>> ref = new AtomicReference<>();
+
+        final CompletableFuture<Optional<? extends RewriteResult<?, ?>>> pending = PENDING_REWRITE_CACHE.computeIfAbsent(key, k -> {
+            final CompletableFuture<Optional<? extends RewriteResult<?, ?>>> value = new CompletableFuture<>();
+            ref.set(value);
+            return value;
+        });
+
+        if (ref.get() != null) {
+            Optional<RewriteResult<A, ?>> result = rule.rewrite(this).flatMap(r -> r.view().rewrite(fRule).map(view -> RewriteResult.create(view, r.recData())));
             REWRITE_CACHE.put(key, result);
+            pending.complete(result);
+            PENDING_REWRITE_CACHE.remove(key);
+            return result;
         }
-        return (Optional<RewriteResult<A, ?>>) REWRITE_CACHE.get(key);
+        return (Optional<RewriteResult<A, ?>>) pending.join();
     }
 
     public <FT, FR> Type<?> getSetType(final OpticFinder<FT> optic, final Type<FR> newType) {

diff --git a/src/main/java/com/mojang/datafixers/types/families/RecursiveTypeFamily.java b/src/main/java/com/mojang/datafixers/types/families/RecursiveTypeFamily.java
@@ -19,6 +19,7 @@
 import com.mojang.datafixers.types.templates.TypeTemplate;
 import com.mojang.datafixers.util.Either;
 import it.unimi.dsi.fastutil.ints.Int2ObjectMap;
+import it.unimi.dsi.fastutil.ints.Int2ObjectMaps;
 import it.unimi.dsi.fastutil.ints.Int2ObjectOpenHashMap;
 
 import javax.annotation.Nullable;
@@ -34,7 +35,7 @@ public final class RecursiveTypeFamily implements TypeFamily {
     private final TypeTemplate template;
     private final int size;
 
-    private final Int2ObjectMap<RecursivePoint.RecursivePointType<?>> types = new Int2ObjectOpenHashMap<>();
+    private final Int2ObjectMap<RecursivePoint.RecursivePointType<?>> types = Int2ObjectMaps.synchronize(new Int2ObjectOpenHashMap<>());
     private final int hashCode;
 
     public RecursiveTypeFamily(final String name, final TypeTemplate template) {

diff --git a/src/main/java/com/mojang/datafixers/types/templates/Tag.java b/src/main/java/com/mojang/datafixers/types/templates/Tag.java
@@ -175,8 +175,8 @@ public TypeTemplate buildTemplate() {
         public <T> Pair<T, Optional<A>> read(final DynamicOps<T> ops, final T input) {
             final Optional<Map<T, T>> map = ops.getMapValues(input);
             final T nameObject = ops.createString(name);
-            if (map.isPresent() && map.get().containsKey(nameObject)) {
-                final T elementValue = map.get().get(nameObject);
+            final T elementValue;
+            if (map.isPresent() && (elementValue = map.get().get(nameObject)) != null) {
                 final Optional<A> value = element.read(ops, elementValue).getSecond();
                 if (value.isPresent()) {
                     return Pair.of(ops.createMap(map.get().entrySet().stream().filter(e -> !Objects.equals(e.getKey(), nameObject)).collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue))), value);

diff --git a/src/main/java/com/mojang/datafixers/types/templates/TaggedChoice.java b/src/main/java/com/mojang/datafixers/types/templates/TaggedChoice.java
@@ -189,30 +189,35 @@ public TypeTemplate buildTemplate() {
             if (values.isPresent()) {
                 final Map<T, T> map = values.get();
                 final T nameObject = ops.createString(name);
-                if (map.containsKey(nameObject)) {
-                    final Optional<K> key = keyType.read(ops, map.get(nameObject)).getSecond();
-                    if (!key.isPresent() || !types.containsKey(key.get())) {
+                final T mapValue = map.get(nameObject);
+                if (mapValue != null) {
+                    final Optional<K> key = keyType.read(ops, mapValue).getSecond();
+                    //noinspection OptionalIsPresent
+                    final K keyValue = key.isPresent() ? key.get() : null;
+                    final Type<?> type = keyValue != null ? types.get(keyValue) : null;
+                    if (type == null) {
                         if (DataFixerUpper.ERRORS_ARE_FATAL) {
-                            throw new IllegalArgumentException("Unsupported key: " + key.get() + " in " + this);
+                            throw new IllegalArgumentException("Unsupported key: " + keyValue + " in " + this);
                         } else {
-                            LOGGER.warn("Unsupported key: {} in {}", key.get(), this);
+                            LOGGER.warn("Unsupported key: {} in {}", keyValue, this);
                             return Pair.of(input, Optional.empty());
                         }
                     }
-                    return types.get(key.get()).read(ops, input).mapSecond(vo -> vo.map(v -> Pair.of(key.get(), v)));
+
+                    return type.read(ops, input).mapSecond(vo -> vo.map(v -> Pair.of(keyValue, v)));
                 }
             }
             return Pair.of(input, Optional.empty());
         }
 
         @Override
         public <T> T write(final DynamicOps<T> ops, final T rest, final Pair<K, ?> value) {
-            if (!types.containsKey(value.getFirst())) {
+            final Type<?> type = types.get(value.getFirst());
+            if (type == null) {
                 // TODO: better error handling?
                 // TODO: See todo in read method
                 throw new IllegalArgumentException("Unsupported key: " + value.getFirst() + " in " + this);
             }
-            final Type<?> type = types.get(value.getFirst());
             return capWrite(ops, type, value.getFirst(), value.getSecond(), rest);
         }