From 831d53eaa301132b57c4150c177425ac8d6db641 Mon Sep 17 00:00:00 2001 From: Aikar Date: Mon, 3 Sep 2018 22:18:38 -0400 Subject: [PATCH 1/7] Fix concurrency and performance issues When implementing asynchronous chunk loading, numerous concurrency issues were found. Also replaced quite a few bad uses of Map.containsKey containsKey "reads" cleaner, but results in double the performance cost of all map operations as containsKey in MOST cases where null values are not used is identical to get() == null Considering how deep data fixers go in call stacks, with tons of map lookups, this micro optimization could provide some real gains. Additionally, many of the containsKey/get/put style operations were also a concurrency risk, resulting in multiple computation/insertions. --- .../com/mojang/datafixers/DataFixerUpper.java | 11 ++--- .../mojang/datafixers/NamedChoiceFinder.java | 4 +- .../datafixers/functions/PointFree.java | 2 +- .../com/mojang/datafixers/schemas/Schema.java | 45 +++++++++++-------- .../mojang/datafixers/types/DynamicOps.java | 6 +-- .../com/mojang/datafixers/types/Type.java | 30 +++++++++++-- .../types/families/RecursiveTypeFamily.java | 3 +- .../datafixers/types/templates/Tag.java | 4 +- .../types/templates/TaggedChoice.java | 21 +++++---- 9 files changed, 82 insertions(+), 44 deletions(-) diff --git a/src/main/java/com/mojang/datafixers/DataFixerUpper.java b/src/main/java/com/mojang/datafixers/DataFixerUpper.java index 8ce8cb92..b6eca89e 100644 --- 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 schemas; private final List globalList; private final IntSortedSet fixerVersions; - private final Long2ObjectMap rules = new Long2ObjectOpenHashMap<>(); + private final Long2ObjectMap rules = Long2ObjectMaps.synchronize(new Long2ObjectOpenHashMap<>()); protected DataFixerUpper(final Int2ObjectSortedMap schemas, final List 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 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 index 1786f776..095829d3 100644 --- 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 type, final Type 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 index 4bc53fdc..094080f9 100644 --- a/src/main/java/com/mojang/datafixers/functions/PointFree.java +++ b/src/main/java/com/mojang/datafixers/functions/PointFree.java @@ -16,7 +16,7 @@ public abstract class PointFree { private Function, T> value; @SuppressWarnings("ConstantConditions") - public Function, T> evalCached() { + public synchronized Function, T> evalCached() { if (!initialized) { initialized = true; value = eval(); diff --git a/src/main/java/com/mojang/datafixers/schemas/Schema.java b/src/main/java/com/mojang/datafixers/schemas/Schema.java index 5d0dd222..2f3d96fd 100644 --- 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,9 @@ 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.Object2IntMaps; +import it.unimi.dsi.fastutil.objects.Object2IntOpenHashMap; import java.util.List; import java.util.Map; @@ -22,8 +23,8 @@ import java.util.function.Supplier; public class Schema { - protected final Object2IntMap RECURSIVE_TYPES = new Object2IntOpenHashMap<>(); - private final Map> TYPE_TEMPLATES = Maps.newHashMap(); + protected final Object2IntMap RECURSIVE_TYPES = Object2IntMaps.synchronize(new Object2IntOpenHashMap<>()); + private final Map> TYPE_TEMPLATES = Maps.newConcurrentMap(); private final Map> TYPES; private final int versionKey; private final String name; @@ -39,25 +40,30 @@ public Schema(final int versionKey, final Schema parent) { } protected Map> buildTypes() { - final Map> types = Maps.newHashMap(); + final Map> types = Maps.newConcurrentMap(); final List templates = Lists.newArrayList(); - for (final Object2IntMap.Entry entry : RECURSIVE_TYPES.object2IntEntrySet()) { - templates.add(DSL.check(entry.getKey(), entry.getIntValue(), getTemplate(entry.getKey()))); + synchronized (RECURSIVE_TYPES) { + for (final Object2IntMap.Entry entry : RECURSIVE_TYPES.object2IntEntrySet()) { + templates.add(DSL.check(entry.getKey(), entry.getIntValue(), getTemplate(entry.getKey()))); + } } final TypeTemplate choice = templates.stream().reduce(DSL::or).get(); final TypeFamily family = new RecursiveTypeFamily(name, choice); - for (final String name : TYPE_TEMPLATES.keySet()) { - final Type type; - if (RECURSIVE_TYPES.containsKey(name)) { - type = family.apply(RECURSIVE_TYPES.getInt(name)); - } else { - type = getTemplate(name).apply(family).apply(-1); + synchronized (TYPE_TEMPLATES) { + for (final String name : TYPE_TEMPLATES.keySet()) { + final Type type; + int recurseId = RECURSIVE_TYPES.getOrDefault(name, -1); + if (recurseId != -1) { + type = family.apply(recurseId); + } else { + type = getTemplate(name).apply(family).apply(-1); + } + types.put(name, type); } - types.put(name, type); } return types; } @@ -91,8 +97,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)); + int id = RECURSIVE_TYPES.getOrDefault(name, -1); + if (id != -1) { + return DSL.id(id); } return getTemplate(name); } @@ -140,8 +147,10 @@ public void register(final Map> map, final String public void registerType(final boolean recursive, final DSL.TypeReference type, final Supplier template) { TYPE_TEMPLATES.put(type.typeName(), template); // TODO: calculate recursiveness instead of hardcoding - if (recursive && !RECURSIVE_TYPES.containsKey(type.typeName())) { - RECURSIVE_TYPES.put(type.typeName(), RECURSIVE_TYPES.size()); + synchronized (RECURSIVE_TYPES) { + if (recursive && !RECURSIVE_TYPES.containsKey(type.typeName())) { + RECURSIVE_TYPES.put(type.typeName(), RECURSIVE_TYPES.size()); + } } } diff --git a/src/main/java/com/mojang/datafixers/types/DynamicOps.java b/src/main/java/com/mojang/datafixers/types/DynamicOps.java index cc5b9059..f7ab335c 100644 --- a/src/main/java/com/mojang/datafixers/types/DynamicOps.java +++ b/src/main/java/com/mojang/datafixers/types/DynamicOps.java @@ -153,10 +153,8 @@ default Optional get(final T input, final String key) { default Optional 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(); + T value = map.get(key); + return value != null ? Optional.of(value) : Optional.empty(); }); } diff --git a/src/main/java/com/mojang/datafixers/types/Type.java b/src/main/java/com/mojang/datafixers/types/Type.java index 152f3ff0..be9ccdea 100644 --- a/src/main/java/com/mojang/datafixers/types/Type.java +++ b/src/main/java/com/mojang/datafixers/types/Type.java @@ -29,8 +29,10 @@ import java.util.Map; import java.util.Objects; import java.util.Optional; +import java.util.concurrent.CompletableFuture; public abstract class Type implements App { + private static final Map, TypeRewriteRule, PointFreeRule>, CompletableFuture>>> PENDING_REWRITE_CACHE = Maps.newConcurrentMap(); private static final Map, TypeRewriteRule, PointFreeRule>, Optional>> REWRITE_CACHE = Maps.newConcurrentMap(); public static class Mu implements K1 {} @@ -157,11 +159,33 @@ public T capWrite(final DynamicOps ops, final Type expectedType, fi @SuppressWarnings("unchecked") public Optional> rewrite(final TypeRewriteRule rule, final PointFreeRule fRule) { final Triple, TypeRewriteRule, PointFreeRule> key = Triple.of(this, rule, fRule); - if (!REWRITE_CACHE.containsKey(key)) { - final Optional> 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. + Optional> rewrite = REWRITE_CACHE.get(key); + //noinspection OptionalAssignedToNull + if (rewrite != null) { + return (Optional>) rewrite; + } + CompletableFuture>> pending; + boolean needsCreate; + synchronized (PENDING_REWRITE_CACHE) { + pending = PENDING_REWRITE_CACHE.get(key); + needsCreate = pending == null; + if (pending == null) { + pending = new CompletableFuture<>(); + PENDING_REWRITE_CACHE.put(key, pending); + } + } + if (needsCreate) { + Optional> 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; + } else { + return (Optional>) pending.join(); } - return (Optional>) REWRITE_CACHE.get(key); } public Type getSetType(final OpticFinder optic, final Type 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 index 234bb4fb..fcbb52f7 100644 --- 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> types = new Int2ObjectOpenHashMap<>(); + private final Int2ObjectMap> 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 index dfa3c7ad..c13e09ff 100644 --- 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 Pair> read(final DynamicOps ops, final T input) { final Optional> 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 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 index 03bad608..ac002d21 100644 --- a/src/main/java/com/mojang/datafixers/types/templates/TaggedChoice.java +++ b/src/main/java/com/mojang/datafixers/types/templates/TaggedChoice.java @@ -189,17 +189,22 @@ public TypeTemplate buildTemplate() { if (values.isPresent()) { final Map map = values.get(); final T nameObject = ops.createString(name); - if (map.containsKey(nameObject)) { - final Optional key = keyType.read(ops, map.get(nameObject)).getSecond(); - if (!key.isPresent() || !types.containsKey(key.get())) { + T mapValue = map.get(nameObject); + if (mapValue != null) { + final Optional key = keyType.read(ops, mapValue).getSecond(); + //noinspection OptionalIsPresent + K keyValue = key.isPresent() ? key.get() : null; + 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()); @@ -207,12 +212,12 @@ public TypeTemplate buildTemplate() { @Override public T write(final DynamicOps ops, final T rest, final Pair 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); } From 60b4f027a9fd7aea4b888256783967d90d18d944 Mon Sep 17 00:00:00 2001 From: Aikar Date: Sat, 6 Oct 2018 13:07:57 -0400 Subject: [PATCH 2/7] Use Optional.ofNullable --- src/main/java/com/mojang/datafixers/types/DynamicOps.java | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/main/java/com/mojang/datafixers/types/DynamicOps.java b/src/main/java/com/mojang/datafixers/types/DynamicOps.java index f7ab335c..032e9efe 100644 --- a/src/main/java/com/mojang/datafixers/types/DynamicOps.java +++ b/src/main/java/com/mojang/datafixers/types/DynamicOps.java @@ -152,10 +152,7 @@ default Optional get(final T input, final String key) { } default Optional getGeneric(final T input, final T key) { - return getMapValues(input).flatMap(map -> { - T value = map.get(key); - return value != null ? Optional.of(value) : Optional.empty(); - }); + return getMapValues(input).flatMap(map -> Optional.ofNullable(map.get(key))); } default T set(final T input, final String key, final T value) { From 0cc98408dd4edbd59a34df65bb6f3cdbcf5e5523 Mon Sep 17 00:00:00 2001 From: Aikar Date: Thu, 11 Oct 2018 22:51:30 -0400 Subject: [PATCH 3/7] Remove unnecessary synchronization --- .../datafixers/functions/PointFree.java | 12 ++++-- .../com/mojang/datafixers/schemas/Schema.java | 37 ++++++++----------- .../com/mojang/datafixers/types/Type.java | 12 +++--- 3 files changed, 29 insertions(+), 32 deletions(-) diff --git a/src/main/java/com/mojang/datafixers/functions/PointFree.java b/src/main/java/com/mojang/datafixers/functions/PointFree.java index 094080f9..bcdbaad7 100644 --- 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 { - private boolean initialized; + private volatile boolean initialized; @Nullable private Function, T> value; @SuppressWarnings("ConstantConditions") - public synchronized Function, T> evalCached() { + public Function, 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 index 2f3d96fd..4717aa9d 100644 --- a/src/main/java/com/mojang/datafixers/schemas/Schema.java +++ b/src/main/java/com/mojang/datafixers/schemas/Schema.java @@ -13,7 +13,6 @@ 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.Object2IntMaps; import it.unimi.dsi.fastutil.objects.Object2IntOpenHashMap; import java.util.List; @@ -23,8 +22,8 @@ import java.util.function.Supplier; public class Schema { - protected final Object2IntMap RECURSIVE_TYPES = Object2IntMaps.synchronize(new Object2IntOpenHashMap<>()); - private final Map> TYPE_TEMPLATES = Maps.newConcurrentMap(); + protected final Object2IntMap RECURSIVE_TYPES = new Object2IntOpenHashMap<>(); + private final Map> TYPE_TEMPLATES = Maps.newHashMap(); private final Map> TYPES; private final int versionKey; private final String name; @@ -40,30 +39,26 @@ public Schema(final int versionKey, final Schema parent) { } protected Map> buildTypes() { - final Map> types = Maps.newConcurrentMap(); + final Map> types = Maps.newHashMap(); final List templates = Lists.newArrayList(); - synchronized (RECURSIVE_TYPES) { - for (final Object2IntMap.Entry entry : RECURSIVE_TYPES.object2IntEntrySet()) { - templates.add(DSL.check(entry.getKey(), entry.getIntValue(), getTemplate(entry.getKey()))); - } + for (final Object2IntMap.Entry entry : RECURSIVE_TYPES.object2IntEntrySet()) { + templates.add(DSL.check(entry.getKey(), entry.getIntValue(), getTemplate(entry.getKey()))); } final TypeTemplate choice = templates.stream().reduce(DSL::or).get(); final TypeFamily family = new RecursiveTypeFamily(name, choice); - synchronized (TYPE_TEMPLATES) { - for (final String name : TYPE_TEMPLATES.keySet()) { - final Type type; - int recurseId = RECURSIVE_TYPES.getOrDefault(name, -1); - if (recurseId != -1) { - type = family.apply(recurseId); - } else { - type = getTemplate(name).apply(family).apply(-1); - } - types.put(name, type); + for (final String name : TYPE_TEMPLATES.keySet()) { + final Type type; + int recurseId = RECURSIVE_TYPES.getOrDefault(name, -1); + if (recurseId != -1) { + type = family.apply(recurseId); + } else { + type = getTemplate(name).apply(family).apply(-1); } + types.put(name, type); } return types; } @@ -147,10 +142,8 @@ public void register(final Map> map, final String public void registerType(final boolean recursive, final DSL.TypeReference type, final Supplier template) { TYPE_TEMPLATES.put(type.typeName(), template); // TODO: calculate recursiveness instead of hardcoding - synchronized (RECURSIVE_TYPES) { - if (recursive && !RECURSIVE_TYPES.containsKey(type.typeName())) { - RECURSIVE_TYPES.put(type.typeName(), RECURSIVE_TYPES.size()); - } + if (recursive && !RECURSIVE_TYPES.containsKey(type.typeName())) { + RECURSIVE_TYPES.put(type.typeName(), RECURSIVE_TYPES.size()); } } diff --git a/src/main/java/com/mojang/datafixers/types/Type.java b/src/main/java/com/mojang/datafixers/types/Type.java index be9ccdea..753519be 100644 --- a/src/main/java/com/mojang/datafixers/types/Type.java +++ b/src/main/java/com/mojang/datafixers/types/Type.java @@ -32,8 +32,8 @@ import java.util.concurrent.CompletableFuture; public abstract class Type implements App { - private static final Map, TypeRewriteRule, PointFreeRule>, CompletableFuture>>> PENDING_REWRITE_CACHE = Maps.newConcurrentMap(); - private static final Map, TypeRewriteRule, PointFreeRule>, Optional>> REWRITE_CACHE = Maps.newConcurrentMap(); + private final Map, CompletableFuture>>> PENDING_REWRITE_CACHE = Maps.newConcurrentMap(); + private final Map, Optional>> REWRITE_CACHE = Maps.newConcurrentMap(); public static class Mu implements K1 {} @@ -158,16 +158,16 @@ public T capWrite(final DynamicOps ops, final Type expectedType, fi @SuppressWarnings("unchecked") public Optional> rewrite(final TypeRewriteRule rule, final PointFreeRule fRule) { - final Triple, TypeRewriteRule, PointFreeRule> key = Triple.of(this, rule, fRule); + final Pair key = Pair.of(rule, fRule); // 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. - Optional> rewrite = REWRITE_CACHE.get(key); + Optional> rewrite = REWRITE_CACHE.get(key); //noinspection OptionalAssignedToNull if (rewrite != null) { - return (Optional>) rewrite; + return rewrite; } - CompletableFuture>> pending; + CompletableFuture>> pending; boolean needsCreate; synchronized (PENDING_REWRITE_CACHE) { pending = PENDING_REWRITE_CACHE.get(key); From 4d92380f9a3cd1215f725252229993cc88e44fdf Mon Sep 17 00:00:00 2001 From: Aikar Date: Thu, 11 Oct 2018 22:59:33 -0400 Subject: [PATCH 4/7] Remove import of triple --- src/main/java/com/mojang/datafixers/types/Type.java | 1 - 1 file changed, 1 deletion(-) diff --git a/src/main/java/com/mojang/datafixers/types/Type.java b/src/main/java/com/mojang/datafixers/types/Type.java index 753519be..c132834b 100644 --- a/src/main/java/com/mojang/datafixers/types/Type.java +++ b/src/main/java/com/mojang/datafixers/types/Type.java @@ -23,7 +23,6 @@ import com.mojang.datafixers.types.families.RecursiveTypeFamily; import com.mojang.datafixers.types.templates.TaggedChoice; import com.mojang.datafixers.types.templates.TypeTemplate; -import org.apache.commons.lang3.tuple.Triple; import javax.annotation.Nullable; import java.util.Map; From b7dff84dd5adf5777f10f1b666eafe1330d84cbd Mon Sep 17 00:00:00 2001 From: Aikar Date: Fri, 12 Oct 2018 00:58:01 -0400 Subject: [PATCH 5/7] Revert changes to rewrite cache --- src/main/java/com/mojang/datafixers/types/Type.java | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/main/java/com/mojang/datafixers/types/Type.java b/src/main/java/com/mojang/datafixers/types/Type.java index c132834b..be9ccdea 100644 --- a/src/main/java/com/mojang/datafixers/types/Type.java +++ b/src/main/java/com/mojang/datafixers/types/Type.java @@ -23,6 +23,7 @@ import com.mojang.datafixers.types.families.RecursiveTypeFamily; import com.mojang.datafixers.types.templates.TaggedChoice; import com.mojang.datafixers.types.templates.TypeTemplate; +import org.apache.commons.lang3.tuple.Triple; import javax.annotation.Nullable; import java.util.Map; @@ -31,8 +32,8 @@ import java.util.concurrent.CompletableFuture; public abstract class Type implements App { - private final Map, CompletableFuture>>> PENDING_REWRITE_CACHE = Maps.newConcurrentMap(); - private final Map, Optional>> REWRITE_CACHE = Maps.newConcurrentMap(); + private static final Map, TypeRewriteRule, PointFreeRule>, CompletableFuture>>> PENDING_REWRITE_CACHE = Maps.newConcurrentMap(); + private static final Map, TypeRewriteRule, PointFreeRule>, Optional>> REWRITE_CACHE = Maps.newConcurrentMap(); public static class Mu implements K1 {} @@ -157,16 +158,16 @@ public T capWrite(final DynamicOps ops, final Type expectedType, fi @SuppressWarnings("unchecked") public Optional> rewrite(final TypeRewriteRule rule, final PointFreeRule fRule) { - final Pair key = Pair.of(rule, fRule); + final Triple, TypeRewriteRule, PointFreeRule> key = Triple.of(this, rule, fRule); // 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. - Optional> rewrite = REWRITE_CACHE.get(key); + Optional> rewrite = REWRITE_CACHE.get(key); //noinspection OptionalAssignedToNull if (rewrite != null) { - return rewrite; + return (Optional>) rewrite; } - CompletableFuture>> pending; + CompletableFuture>> pending; boolean needsCreate; synchronized (PENDING_REWRITE_CACHE) { pending = PENDING_REWRITE_CACHE.get(key); From bc27ff04f2f12d050fda370a3d9e712a732f4e0f Mon Sep 17 00:00:00 2001 From: Aikar Date: Fri, 12 Oct 2018 02:37:31 -0400 Subject: [PATCH 6/7] finalize some variables --- src/main/java/com/mojang/datafixers/schemas/Schema.java | 4 ++-- src/main/java/com/mojang/datafixers/types/Type.java | 4 ++-- .../com/mojang/datafixers/types/templates/TaggedChoice.java | 6 +++--- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/main/java/com/mojang/datafixers/schemas/Schema.java b/src/main/java/com/mojang/datafixers/schemas/Schema.java index 4717aa9d..6ad85fda 100644 --- a/src/main/java/com/mojang/datafixers/schemas/Schema.java +++ b/src/main/java/com/mojang/datafixers/schemas/Schema.java @@ -52,7 +52,7 @@ protected Map> buildTypes() { for (final String name : TYPE_TEMPLATES.keySet()) { final Type type; - int recurseId = RECURSIVE_TYPES.getOrDefault(name, -1); + final int recurseId = RECURSIVE_TYPES.getOrDefault(name, -1); if (recurseId != -1) { type = family.apply(recurseId); } else { @@ -92,7 +92,7 @@ public TypeTemplate resolveTemplate(final String name) { } public TypeTemplate id(final String name) { - int id = RECURSIVE_TYPES.getOrDefault(name, -1); + final int id = RECURSIVE_TYPES.getOrDefault(name, -1); if (id != -1) { return DSL.id(id); } diff --git a/src/main/java/com/mojang/datafixers/types/Type.java b/src/main/java/com/mojang/datafixers/types/Type.java index be9ccdea..7d6f4f49 100644 --- a/src/main/java/com/mojang/datafixers/types/Type.java +++ b/src/main/java/com/mojang/datafixers/types/Type.java @@ -162,13 +162,13 @@ public T capWrite(final DynamicOps ops, final Type expectedType, fi // 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. - Optional> rewrite = REWRITE_CACHE.get(key); + final Optional> rewrite = REWRITE_CACHE.get(key); //noinspection OptionalAssignedToNull if (rewrite != null) { return (Optional>) rewrite; } CompletableFuture>> pending; - boolean needsCreate; + final boolean needsCreate; synchronized (PENDING_REWRITE_CACHE) { pending = PENDING_REWRITE_CACHE.get(key); needsCreate = pending == null; diff --git a/src/main/java/com/mojang/datafixers/types/templates/TaggedChoice.java b/src/main/java/com/mojang/datafixers/types/templates/TaggedChoice.java index ac002d21..847b1332 100644 --- a/src/main/java/com/mojang/datafixers/types/templates/TaggedChoice.java +++ b/src/main/java/com/mojang/datafixers/types/templates/TaggedChoice.java @@ -189,12 +189,12 @@ public TypeTemplate buildTemplate() { if (values.isPresent()) { final Map map = values.get(); final T nameObject = ops.createString(name); - T mapValue = map.get(nameObject); + final T mapValue = map.get(nameObject); if (mapValue != null) { final Optional key = keyType.read(ops, mapValue).getSecond(); //noinspection OptionalIsPresent - K keyValue = key.isPresent() ? key.get() : null; - Type type = keyValue != null ? types.get(keyValue) : null; + 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: " + keyValue + " in " + this); From 089500a2ac8cbb0fa776ce1b76d685c6e39a7bc6 Mon Sep 17 00:00:00 2001 From: Georgii Gavrichev Date: Mon, 15 Oct 2018 08:45:19 +0200 Subject: [PATCH 7/7] Cleaned up Type.rewrite a bit. --- .../com/mojang/datafixers/types/Type.java | 29 +++++++++---------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/src/main/java/com/mojang/datafixers/types/Type.java b/src/main/java/com/mojang/datafixers/types/Type.java index 7d6f4f49..2f44051b 100644 --- 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,6 +21,8 @@ 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; @@ -30,6 +30,7 @@ import java.util.Objects; import java.util.Optional; import java.util.concurrent.CompletableFuture; +import java.util.concurrent.atomic.AtomicReference; public abstract class Type implements App { private static final Map, TypeRewriteRule, PointFreeRule>, CompletableFuture>>> PENDING_REWRITE_CACHE = Maps.newConcurrentMap(); @@ -163,29 +164,25 @@ public T capWrite(final DynamicOps ops, final Type expectedType, fi // 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> rewrite = REWRITE_CACHE.get(key); - //noinspection OptionalAssignedToNull if (rewrite != null) { return (Optional>) rewrite; } - CompletableFuture>> pending; - final boolean needsCreate; - synchronized (PENDING_REWRITE_CACHE) { - pending = PENDING_REWRITE_CACHE.get(key); - needsCreate = pending == null; - if (pending == null) { - pending = new CompletableFuture<>(); - PENDING_REWRITE_CACHE.put(key, pending); - } - } - if (needsCreate) { + final AtomicReference>>> ref = new AtomicReference<>(); + + final CompletableFuture>> pending = PENDING_REWRITE_CACHE.computeIfAbsent(key, k -> { + final CompletableFuture>> value = new CompletableFuture<>(); + ref.set(value); + return value; + }); + + if (ref.get() != null) { Optional> 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; - } else { - return (Optional>) pending.join(); } + return (Optional>) pending.join(); } public Type getSetType(final OpticFinder optic, final Type newType) {