diff --git a/common/src/main/java/me/lucko/luckperms/common/context/ContextSetComparator.java b/common/src/main/java/me/lucko/luckperms/common/context/ContextSetComparator.java index 0fd7d9acb..8d5e43e1a 100644 --- a/common/src/main/java/me/lucko/luckperms/common/context/ContextSetComparator.java +++ b/common/src/main/java/me/lucko/luckperms/common/context/ContextSetComparator.java @@ -25,14 +25,14 @@ package me.lucko.luckperms.common.context; +import me.lucko.luckperms.common.context.contextset.ImmutableContextSetImpl; + import net.luckperms.api.context.Context; import net.luckperms.api.context.DefaultContextKeys; import net.luckperms.api.context.ImmutableContextSet; -import java.util.ArrayList; +import java.util.Arrays; import java.util.Comparator; -import java.util.Iterator; -import java.util.List; public class ContextSetComparator implements Comparator { @@ -53,40 +53,41 @@ public class ContextSetComparator implements Comparator { return 0; } + // compare presence of a server context int result = Boolean.compare(o1.containsKey(DefaultContextKeys.SERVER_KEY), o2.containsKey(DefaultContextKeys.SERVER_KEY)); if (result != 0) { return result; } + // compare presence of a world context result = Boolean.compare(o1.containsKey(DefaultContextKeys.WORLD_KEY), o2.containsKey(DefaultContextKeys.WORLD_KEY)); if (result != 0) { return result; } + // compare overall size result = Integer.compare(o1.size(), o2.size()); if (result != 0) { return result; } - // we *have* to maintain transitivity in this comparator. this may be expensive, but it's necessary, as this - // comparator is used in the PermissionHolder nodes treemap + // At this point, we don't really care about the order between the two sets. + // However, we *have* to maintain transitivity in this comparator (despite how + // expensive/complex it may be) as it is used in the PermissionHolder nodes treemap. - // in order to have consistent ordering, we have to compare the content of the context sets by ordering the - // elements and then comparing which set is greater. - List o1Entries = new ArrayList<>(o1.toSet()); - List o2Entries = new ArrayList<>(o2.toSet()); - o1Entries.sort(CONTEXT_COMPARATOR); - o2Entries.sort(CONTEXT_COMPARATOR); + // in order to have consistent ordering, we have to compare the content of the context sets + // by sorting the contents and then comparing which set is greater. + Context[] o1Array = o1 instanceof ImmutableContextSetImpl ? ((ImmutableContextSetImpl) o1).toArray() : o1.toSet().toArray(new Context[0]); + Context[] o2Array = o2 instanceof ImmutableContextSetImpl ? ((ImmutableContextSetImpl) o2).toArray() : o2.toSet().toArray(new Context[0]); - // size is definitely the same - Iterator it1 = o1Entries.iterator(); - Iterator it2 = o2Entries.iterator(); + Arrays.sort(o1Array, CONTEXT_COMPARATOR); + Arrays.sort(o2Array, CONTEXT_COMPARATOR); - while (it1.hasNext()) { - Context ent1 = it1.next(); - Context ent2 = it2.next(); + for (int i = 0; i < o1Array.length; i++) { + Context ent1 = o1Array[i]; + Context ent2 = o2Array[i]; - result = CONTEXT_COMPARATOR.compare(ent1, ent2); + result = compareContexts(ent1, ent2); if (result != 0) { return result; } @@ -95,19 +96,23 @@ public class ContextSetComparator implements Comparator { throw new AssertionError("sets are equal? " + o1 + " - " + o2); } - @SuppressWarnings("StringEquality") - private static final Comparator FAST_STRING_COMPARATOR = (o1, o2) -> o1 == o2 ? 0 : o1.compareTo(o2); + private static final Comparator CONTEXT_COMPARATOR = ContextSetComparator::compareContexts; - private static final Comparator CONTEXT_COMPARATOR = (o1, o2) -> { + private static int compareContexts(Context o1, Context o2) { if (o1 == o2) { return 0; } - int i = FAST_STRING_COMPARATOR.compare(o1.getKey(), o2.getKey()); + int i = compareStringsFast(o1.getKey(), o2.getKey()); if (i != 0) { return i; } - return FAST_STRING_COMPARATOR.compare(o1.getValue(), o2.getValue()); - }; + return compareStringsFast(o1.getValue(), o2.getValue()); + } + + @SuppressWarnings("StringEquality") + private static int compareStringsFast(String o1, String o2) { + return o1 == o2 ? 0 : o1.compareTo(o2); + } } diff --git a/common/src/main/java/me/lucko/luckperms/common/context/contextset/AbstractContextSet.java b/common/src/main/java/me/lucko/luckperms/common/context/contextset/AbstractContextSet.java index f2dc00125..87388ce5e 100644 --- a/common/src/main/java/me/lucko/luckperms/common/context/contextset/AbstractContextSet.java +++ b/common/src/main/java/me/lucko/luckperms/common/context/contextset/AbstractContextSet.java @@ -26,6 +26,7 @@ package me.lucko.luckperms.common.context.contextset; import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Iterators; import com.google.common.collect.SetMultimap; import net.luckperms.api.context.Context; @@ -35,9 +36,12 @@ import net.luckperms.api.context.DefaultContextKeys; import org.checkerframework.checker.nullness.qual.NonNull; +import java.util.Arrays; import java.util.Collection; +import java.util.Iterator; import java.util.Objects; import java.util.Set; +import java.util.Spliterator; public abstract class AbstractContextSet implements ContextSet { @@ -91,6 +95,18 @@ public abstract class AbstractContextSet implements ContextSet { protected abstract boolean otherContainsAll(ContextSet other, ContextSatisfyMode mode); + public abstract Context[] toArray(); + + @Override + public @NonNull Iterator iterator() { + return Iterators.forArray(toArray()); + } + + @Override + public Spliterator spliterator() { + return Arrays.spliterator(toArray()); + } + @Override public boolean isEmpty() { return backing().isEmpty(); diff --git a/common/src/main/java/me/lucko/luckperms/common/context/contextset/ImmutableContextSetImpl.java b/common/src/main/java/me/lucko/luckperms/common/context/contextset/ImmutableContextSetImpl.java index 2765a76b3..3e001d61d 100644 --- a/common/src/main/java/me/lucko/luckperms/common/context/contextset/ImmutableContextSetImpl.java +++ b/common/src/main/java/me/lucko/luckperms/common/context/contextset/ImmutableContextSetImpl.java @@ -25,7 +25,6 @@ package me.lucko.luckperms.common.context.contextset; -import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSetMultimap; @@ -42,11 +41,9 @@ import net.luckperms.api.context.MutableContextSet; import org.checkerframework.checker.nullness.qual.NonNull; import java.util.Collection; -import java.util.Iterator; import java.util.Map; import java.util.Objects; import java.util.Set; -import java.util.Spliterator; public final class ImmutableContextSetImpl extends AbstractContextSet implements ImmutableContextSet { public static final ImmutableContextSetImpl EMPTY = new ImmutableContextSetImpl(ImmutableSetMultimap.of()); @@ -64,11 +61,19 @@ public final class ImmutableContextSetImpl extends AbstractContextSet implements } private final ImmutableSetMultimap map; + private final Context[] array; private final int hashCode; ImmutableContextSetImpl(ImmutableSetMultimap contexts) { this.map = contexts; this.hashCode = this.map.hashCode(); + + Set> entries = this.map.entries(); + this.array = new Context[entries.size()]; + int i = 0; + for (Map.Entry e : entries) { + this.array[i++] = new ContextImpl(e.getKey(), e.getValue()); + } } @Override @@ -99,12 +104,7 @@ public final class ImmutableContextSetImpl extends AbstractContextSet implements @Override public @NonNull Set toSet() { - ImmutableSet.Builder builder = ImmutableSet.builder(); - Set> entries = this.map.entries(); - for (Map.Entry e : entries) { - builder.add(new ContextImpl(e.getKey(), e.getValue())); - } - return builder.build(); + return ImmutableSet.copyOf(this.array); } @Override @@ -122,24 +122,9 @@ public final class ImmutableContextSetImpl extends AbstractContextSet implements return m.build(); } - private ImmutableList toList() { - Set> entries = this.map.entries(); - Context[] array = new Context[entries.size()]; - int i = 0; - for (Map.Entry e : entries) { - array[i++] = new ContextImpl(e.getKey(), e.getValue()); - } - return ImmutableList.copyOf(array); - } - @Override - public @NonNull Iterator iterator() { - return toList().iterator(); - } - - @Override - public Spliterator spliterator() { - return toList().spliterator(); + public Context[] toArray() { + return this.array; } @Override diff --git a/common/src/main/java/me/lucko/luckperms/common/context/contextset/MutableContextSetImpl.java b/common/src/main/java/me/lucko/luckperms/common/context/contextset/MutableContextSetImpl.java index d7699131d..6f59291da 100644 --- a/common/src/main/java/me/lucko/luckperms/common/context/contextset/MutableContextSetImpl.java +++ b/common/src/main/java/me/lucko/luckperms/common/context/contextset/MutableContextSetImpl.java @@ -26,7 +26,6 @@ package me.lucko.luckperms.common.context.contextset; import com.google.common.collect.HashMultimap; -import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSetMultimap; @@ -43,11 +42,9 @@ import net.luckperms.api.context.MutableContextSet; import org.checkerframework.checker.nullness.qual.NonNull; import java.util.Collection; -import java.util.Iterator; import java.util.Map; import java.util.Objects; import java.util.Set; -import java.util.Spliterator; public final class MutableContextSetImpl extends AbstractContextSet implements MutableContextSet { private final SetMultimap map; @@ -132,7 +129,8 @@ public final class MutableContextSetImpl extends AbstractContextSet implements M return builder.build(); } - private ImmutableList toList() { + @Override + public Context[] toArray() { Set> entries = this.map.entries(); Context[] array; synchronized (this.map) { @@ -142,17 +140,7 @@ public final class MutableContextSetImpl extends AbstractContextSet implements M array[i++] = new ContextImpl(e.getKey(), e.getValue()); } } - return ImmutableList.copyOf(array); - } - - @Override - public @NonNull Iterator iterator() { - return toList().iterator(); - } - - @Override - public Spliterator spliterator() { - return toList().spliterator(); + return array; } @Override diff --git a/common/src/main/java/me/lucko/luckperms/common/node/comparator/NodeComparator.java b/common/src/main/java/me/lucko/luckperms/common/node/comparator/NodeComparator.java index d7ad6bfc9..dc2e639a5 100644 --- a/common/src/main/java/me/lucko/luckperms/common/node/comparator/NodeComparator.java +++ b/common/src/main/java/me/lucko/luckperms/common/node/comparator/NodeComparator.java @@ -31,7 +31,6 @@ import net.luckperms.api.node.types.PermissionNode; import java.util.Comparator; public class NodeComparator implements Comparator { - private static final Comparator INSTANCE = new NodeComparator(); private static final Comparator REVERSE = INSTANCE.reversed(); @@ -43,17 +42,20 @@ public class NodeComparator implements Comparator { return REVERSE; } + @SuppressWarnings({"ConstantConditions", "OptionalGetWithoutIsPresent"}) @Override public int compare(Node o1, Node o2) { if (o1.equals(o2)) { return 0; } + // compare whether nodes are temporary int result = Boolean.compare(o1.hasExpiry(), o2.hasExpiry()); if (result != 0) { return result; } + // compare whether nodes are wildcard nodes result = Boolean.compare( o1 instanceof PermissionNode && ((PermissionNode) o1).isWildcard(), o2 instanceof PermissionNode && ((PermissionNode) o2).isWildcard() @@ -62,6 +64,8 @@ public class NodeComparator implements Comparator { return result; } + // compare expiry times if both nodes are temporary + // due to the comparison earlier, either both nodes are temporary or neither are. if (o1.hasExpiry()) { result = o1.getExpiry().compareTo(o2.getExpiry()); if (result != 0) { @@ -69,20 +73,27 @@ public class NodeComparator implements Comparator { } } - if (o1 instanceof PermissionNode && ((PermissionNode) o1).isWildcard()) { - result = Integer.compare(((PermissionNode) o1).getWildcardLevel().getAsInt(), ((PermissionNode) o2).getWildcardLevel().getAsInt()); + // compare wildcard level if both nodes are wildcards + // due to the comparison earlier, either both nodes are wildcards or neither are + if (o1 instanceof PermissionNode && ((PermissionNode) o1).isWildcard()) { // implies o2.isWildcard too + int o1Level = ((PermissionNode) o1).getWildcardLevel().getAsInt(); + int o2Level = ((PermissionNode) o2).getWildcardLevel().getAsInt(); + + result = Integer.compare(o1Level, o2Level); if (result != 0) { return result; } } - // note vvv + // compare node keys lexicographically + // note that the order is reversed - A comes before Z result = -o1.getKey().compareTo(o2.getKey()); if (result != 0) { return result; } - // note vvv - we want false to have priority + // compare the boolean node values + // note that the order is reversed, false comes before true result = -Boolean.compare(o1.getValue(), o2.getValue()); if (result != 0) { return result; diff --git a/common/src/main/java/me/lucko/luckperms/common/node/comparator/NodeWithContextComparator.java b/common/src/main/java/me/lucko/luckperms/common/node/comparator/NodeWithContextComparator.java index 0358da3ed..2b84a53c5 100644 --- a/common/src/main/java/me/lucko/luckperms/common/node/comparator/NodeWithContextComparator.java +++ b/common/src/main/java/me/lucko/luckperms/common/node/comparator/NodeWithContextComparator.java @@ -27,6 +27,7 @@ package me.lucko.luckperms.common.node.comparator; import me.lucko.luckperms.common.context.ContextSetComparator; +import net.luckperms.api.context.ImmutableContextSet; import net.luckperms.api.node.Node; import java.util.Comparator; @@ -35,10 +36,8 @@ import java.util.Comparator; * Compares permission nodes based upon their supposed "priority". */ public class NodeWithContextComparator implements Comparator { - private NodeWithContextComparator() {} - - private static final Comparator INSTANCE = new NodeWithContextComparator(); - private static final Comparator REVERSE = INSTANCE.reversed(); + private static final Comparator INSTANCE = new NodeWithContextComparator(ContextSetComparator.normal(), NodeComparator.normal()); + private static final Comparator REVERSE = new NodeWithContextComparator(ContextSetComparator.reverse(), NodeComparator.reverse()); public static Comparator normal() { return INSTANCE; @@ -48,22 +47,26 @@ public class NodeWithContextComparator implements Comparator { return REVERSE; } + private final Comparator contextSetComparator; + private final Comparator nodeComparator; + + NodeWithContextComparator(Comparator contextSetComparator, Comparator nodeComparator) { + this.contextSetComparator = contextSetComparator; + this.nodeComparator = nodeComparator; + } + @Override public int compare(Node o1, Node o2) { if (o1.equals(o2)) { return 0; } - int result = ContextSetComparator.normal().compare( - o1.getContexts(), - o2.getContexts() - ); - + int result = this.contextSetComparator.compare(o1.getContexts(), o2.getContexts()); if (result != 0) { return result; } - return NodeComparator.normal().compare(o1, o2); + return this.nodeComparator.compare(o1, o2); } }