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

Fix concurrency and performance issues #14

Merged
merged 8 commits into from
Oct 16, 2018
Merged

Fix concurrency and performance issues #14

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
831d53e
Fix concurrency and performance issues
aikar Sep 4, 2018
60b4f02
Use Optional.ofNullable
aikar Oct 6, 2018
a0318c3
Merge remote-tracking branch 'upstream/master' into fix-concurrency
aikar Oct 12, 2018
0cc9840
Remove unnecessary synchronization
aikar Oct 12, 2018
4d92380
Remove import of triple
aikar Oct 12, 2018
b7dff84
Revert changes to rewrite cache
aikar Oct 12, 2018
bc27ff0
finalize some variables
aikar Oct 12, 2018
089500a
Cleaned up Type.rewrite a bit.
fry-mojang Oct 15, 2018
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
11 changes: 6 additions & 5 deletions src/main/java/com/mojang/datafixers/DataFixerUpper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand All @@ -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() {
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/com/mojang/datafixers/NamedChoiceFinder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)));
}
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/mojang/datafixers/functions/PointFree.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ public abstract class PointFree<T> {
private Function<DynamicOps<?>, T> value;

@SuppressWarnings("ConstantConditions")
public Function<DynamicOps<?>, T> evalCached() {
public synchronized Function<DynamicOps<?>, T> evalCached() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems too heavy-weight, since evalCached should be a hot spot. There might be a better option, like https://en.wikipedia.org/wiki/Double-checked_locking

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is something I was concerned about too and was considering a double checked lock also. I will update it.

Wasn't able to detect any performance concerns considering we have usage of datafixers all done on the thread pool now, but helping improve conversion speed is always good.

if (!initialized) {
initialized = true;
value = eval();
Expand Down
45 changes: 27 additions & 18 deletions src/main/java/com/mojang/datafixers/schemas/Schema.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -22,8 +23,8 @@
import java.util.function.Supplier;

public class Schema {
protected final Object2IntMap<String> RECURSIVE_TYPES = new Object2IntOpenHashMap<>();
private final Map<String, Supplier<TypeTemplate>> TYPE_TEMPLATES = Maps.newHashMap();
protected final Object2IntMap<String> RECURSIVE_TYPES = Object2IntMaps.synchronize(new Object2IntOpenHashMap<>());
Copy link
Contributor

Choose a reason for hiding this comment

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

All schema construction should be done during bootstrap and frozen after that, synchronization inside buildTypes doesn't quite make sense since it's a one-off operation anyway. This class can hopefully be cleaned up slightly, and made to construct immutable maps instead, but synchronization is not the right approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. I went a bit heavy handed to make sure I covered any risks, but looking at it closer I agree shouldn't be needed since it's all done in constructor.

private final Map<String, Supplier<TypeTemplate>> TYPE_TEMPLATES = Maps.newConcurrentMap();
private final Map<String, Type<?>> TYPES;
private final int versionKey;
private final String name;
Expand All @@ -39,25 +40,30 @@ public Schema(final int versionKey, final Schema parent) {
}

protected Map<String, Type<?>> buildTypes() {
final Map<String, Type<?>> types = Maps.newHashMap();
final Map<String, Type<?>> types = Maps.newConcurrentMap();

final List<TypeTemplate> templates = Lists.newArrayList();

for (final Object2IntMap.Entry<String> entry : RECURSIVE_TYPES.object2IntEntrySet()) {
templates.add(DSL.check(entry.getKey(), entry.getIntValue(), getTemplate(entry.getKey())));
synchronized (RECURSIVE_TYPES) {
for (final Object2IntMap.Entry<String> 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;
}
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -140,8 +147,10 @@ public void register(final Map<String, Supplier<TypeTemplate>> map, final String
public void registerType(final boolean recursive, final DSL.TypeReference type, final Supplier<TypeTemplate> 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());
}
}
}

Expand Down
6 changes: 2 additions & 4 deletions src/main/java/com/mojang/datafixers/types/DynamicOps.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,10 +153,8 @@ 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();
T value = map.get(key);
return value != null ? Optional.of(value) : Optional.empty();
aikar marked this conversation as resolved.
Show resolved Hide resolved
});
}

Expand Down
30 changes: 27 additions & 3 deletions src/main/java/com/mojang/datafixers/types/Type.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<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 {}
Expand Down Expand Up @@ -157,11 +159,33 @@ 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.
Optional<? extends RewriteResult<?, ?>> rewrite = REWRITE_CACHE.get(key);
//noinspection OptionalAssignedToNull
if (rewrite != null) {
return (Optional<RewriteResult<A, ?>>) rewrite;
Copy link
Contributor

Choose a reason for hiding this comment

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

Cast should not be neccesary if the map stores CompletableFuture<? extends Optional<? extends RewriteResult<?, ?>>>>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not worth changing this. That breaks everything else. The class uses a generic but uses a static cache, so the cast is required unless the caches are moved to be instance caches instead.

Copy link
Contributor Author

@aikar aikar Oct 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

just noticed the cache key references this anyways... I'm just going to move it to an instance property
edit: not doing that, breaks all the things

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the reason you failed is that you need another static field with an object that you sync around instead of the instance cache, isn't it?

}
CompletableFuture<Optional<? extends RewriteResult<?, ?>>> 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<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;
} else {
return (Optional<RewriteResult<A, ?>>) pending.join();
}
return (Optional<RewriteResult<A, ?>>) REWRITE_CACHE.get(key);
}

public <FT, FR> Type<?> getSetType(final OpticFinder<FT> optic, final Type<FR> newType) {
Expand Down
3 changes: 2 additions & 1 deletion src/main/java/com/mojang/datafixers/types/families/RecursiveTypeFamily.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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) {
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/com/mojang/datafixers/types/templates/Tag.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
21 changes: 13 additions & 8 deletions src/main/java/com/mojang/datafixers/types/templates/TaggedChoice.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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())) {
T mapValue = map.get(nameObject);
if (mapValue != null) {
final Optional<K> 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());
}

@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);
}

Expand Down