Skip to content

Commit

Permalink
Faster CollectionUtils.ensureNoSelfReferences (#93433)
Browse files Browse the repository at this point in the history
  • Loading branch information
@joegallo
joegallo committed Feb 2, 2023
1 parent bc05481 commit 94d16a2
Show file tree
Hide file tree
Showing 3 changed files with 111 additions and 51 deletions.
70 changes: 40 additions & 30 deletions server/src/main/java/org/elasticsearch/common/util/CollectionUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
package org.elasticsearch.common.util;

import org.elasticsearch.common.Strings;
import org.elasticsearch.common.collect.Iterators;

import java.nio.file.Path;
import java.util.AbstractList;
Expand All @@ -21,7 +20,6 @@
import java.util.IdentityHashMap;
import java.util.List;
import java.util.ListIterator;
import java.util.Locale;
import java.util.Map;
import java.util.Objects;
import java.util.RandomAccess;
Expand Down Expand Up @@ -100,44 +98,56 @@ public static int[] toArray(Collection<Integer> ints) {
* @param messageHint A string to be included in the exception message if the call fails, to provide
* more context to the handler of the exception
*/
public static void ensureNoSelfReferences(Object value, String messageHint) {
Iterable<?> it = convert(value);
if (it != null) {
ensureNoSelfReferences(it, value, Collections.newSetFromMap(new IdentityHashMap<>()), messageHint);
}
public static void ensureNoSelfReferences(final Object value, final String messageHint) {
ensureNoSelfReferences(value, Collections.newSetFromMap(new IdentityHashMap<>()), messageHint);
}

@SuppressWarnings("unchecked")
private static Iterable<?> convert(Object value) {
if (value == null) {
return null;
}
if (value instanceof Map<?, ?> map) {
return () -> Iterators.concat(map.keySet().iterator(), map.values().iterator());
} else if ((value instanceof Iterable) && (value instanceof Path == false)) {
return (Iterable<?>) value;
private static void ensureNoSelfReferences(final Object value, final Set<Object> ancestors, final String messageHint) {
// these instanceof checks are a bit on the ugly side, but it's important for performance that we have
// a separate dispatch point for Maps versus for Iterables. a polymorphic version of this code would
// be prettier, but it would also likely be quite a bit slower. this is a hot path for ingest pipelines,
// and performance here is important.
if (value == null || value instanceof String || value instanceof Number || value instanceof Boolean) {
// noop
} else if (value instanceof Map<?, ?> m && m.isEmpty() == false) {
ensureNoSelfReferences(m, ancestors, messageHint);
} else if ((value instanceof Iterable<?> i) && (value instanceof Path == false)) {
ensureNoSelfReferences(i, i, ancestors, messageHint);
} else if (value instanceof Object[]) {
return Arrays.asList((Object[]) value);
} else {
return null;
// note: the iterable and reference arguments are different
ensureNoSelfReferences(Arrays.asList((Object[]) value), value, ancestors, messageHint);
}
}

private static void ensureNoSelfReferences(final Map<?, ?> reference, final Set<Object> ancestors, final String messageHint) {
addToAncestorsOrThrow(reference, ancestors, messageHint);
for (Map.Entry<?, ?> e : reference.entrySet()) {
ensureNoSelfReferences(e.getKey(), ancestors, messageHint);
ensureNoSelfReferences(e.getValue(), ancestors, messageHint);
}
ancestors.remove(reference);
}

private static void ensureNoSelfReferences(
final Iterable<?> value,
Object originalReference,
final Iterable<?> iterable,
final Object reference,
final Set<Object> ancestors,
String messageHint
final String messageHint
) {
if (value != null) {
if (ancestors.add(originalReference) == false) {
String suffix = Strings.isNullOrEmpty(messageHint) ? "" : String.format(Locale.ROOT, " (%s)", messageHint);
throw new IllegalArgumentException("Iterable object is self-referencing itself" + suffix);
}
for (Object o : value) {
ensureNoSelfReferences(convert(o), o, ancestors, messageHint);
addToAncestorsOrThrow(reference, ancestors, messageHint);
for (Object o : iterable) {
ensureNoSelfReferences(o, ancestors, messageHint);
}
ancestors.remove(reference);
}

private static void addToAncestorsOrThrow(Object reference, Set<Object> ancestors, String messageHint) {
if (ancestors.add(reference) == false) {
StringBuilder sb = new StringBuilder("Iterable object is self-referencing itself");
if (Strings.hasLength(messageHint)) {
sb.append(" (").append(messageHint).append(")");
}
ancestors.remove(originalReference);
throw new IllegalArgumentException(sb.toString());
}
}

Expand Down
89 changes: 70 additions & 19 deletions server/src/test/java/org/elasticsearch/common/util/CollectionUtilsTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,28 +8,29 @@

package org.elasticsearch.common.util;

import org.elasticsearch.common.collect.Iterators;
import org.elasticsearch.test.ESTestCase;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.Comparator;
import java.util.HashMap;
import java.util.HashSet;
import java.util.LinkedList;
import java.util.List;
import java.util.Map;
import java.util.Set;

import static java.util.Collections.emptyMap;
import static org.elasticsearch.common.util.CollectionUtils.eagerPartition;
import static org.elasticsearch.common.util.CollectionUtils.ensureNoSelfReferences;
import static org.elasticsearch.common.util.CollectionUtils.limitSize;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.lessThan;

public class CollectionUtilsTests extends ESTestCase {
public void testRotateEmpty() {
assertTrue(CollectionUtils.rotate(Collections.emptyList(), randomInt()).isEmpty());
assertTrue(CollectionUtils.rotate(List.of(), randomInt()).isEmpty());
}

public void testRotate() {
Expand Down Expand Up @@ -77,59 +78,109 @@ public void testUniquify() {
}

public void testEmptyPartition() {
assertEquals(Collections.emptyList(), eagerPartition(Collections.emptyList(), 1));
assertEquals(List.of(), eagerPartition(List.of(), 1));
}

public void testSimplePartition() {
assertEquals(
Arrays.asList(Arrays.asList(1, 2), Arrays.asList(3, 4), Arrays.asList(5)),
eagerPartition(Arrays.asList(1, 2, 3, 4, 5), 2)
);
assertEquals(List.of(List.of(1, 2), List.of(3, 4), List.of(5)), eagerPartition(List.of(1, 2, 3, 4, 5), 2));
}

public void testSingletonPartition() {
assertEquals(
Arrays.asList(Arrays.asList(1), Arrays.asList(2), Arrays.asList(3), Arrays.asList(4), Arrays.asList(5)),
eagerPartition(Arrays.asList(1, 2, 3, 4, 5), 1)
);
assertEquals(List.of(List.of(1), List.of(2), List.of(3), List.of(4), List.of(5)), eagerPartition(List.of(1, 2, 3, 4, 5), 1));
}

public void testOversizedPartition() {
assertEquals(Arrays.asList(Arrays.asList(1, 2, 3, 4, 5)), eagerPartition(Arrays.asList(1, 2, 3, 4, 5), 15));
assertEquals(List.of(List.of(1, 2, 3, 4, 5)), eagerPartition(List.of(1, 2, 3, 4, 5), 15));
}

public void testPerfectPartition() {
assertEquals(
Arrays.asList(Arrays.asList(1, 2, 3, 4, 5, 6), Arrays.asList(7, 8, 9, 10, 11, 12)),
eagerPartition(Arrays.asList(1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12), 6)
List.of(List.of(1, 2, 3, 4, 5, 6), List.of(7, 8, 9, 10, 11, 12)),
eagerPartition(List.of(1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12), 6)
);
}

public void testEnsureNoSelfReferences() {
CollectionUtils.ensureNoSelfReferences(emptyMap(), "test with empty map");
CollectionUtils.ensureNoSelfReferences(null, "test with null");
ensureNoSelfReferences("string", "test with a string");
ensureNoSelfReferences(2, "test with a number");
ensureNoSelfReferences(true, "test with a boolean");
ensureNoSelfReferences(Map.of(), "test with an empty map");
ensureNoSelfReferences(Set.of(), "test with an empty set");
ensureNoSelfReferences(List.of(), "test with an empty list");
ensureNoSelfReferences(new Object[0], "test with an empty array");
ensureNoSelfReferences((Iterable<?>) Collections::emptyIterator, "test with an empty iterable");
}

public void testEnsureNoSelfReferencesMap() {
// map value
{
Map<String, Object> map = new HashMap<>();
map.put("field", map);

IllegalArgumentException e = expectThrows(
IllegalArgumentException.class,
() -> CollectionUtils.ensureNoSelfReferences(map, "test with self ref value")
() -> ensureNoSelfReferences(map, "test with self ref value")
);
assertThat(e.getMessage(), containsString("Iterable object is self-referencing itself (test with self ref value)"));
}
// map key
{
Map<Object, Object> map = new HashMap<>();
map.put(map, 1);

IllegalArgumentException e = expectThrows(
IllegalArgumentException.class,
() -> CollectionUtils.ensureNoSelfReferences(map, "test with self ref key")
() -> ensureNoSelfReferences(map, "test with self ref key")
);
assertThat(e.getMessage(), containsString("Iterable object is self-referencing itself (test with self ref key)"));
}
// nested map value
{
Map<String, Object> map = new HashMap<>();
map.put("field", Set.of(List.of((Iterable<?>) () -> Iterators.single(new Object[] { map }))));

IllegalArgumentException e = expectThrows(
IllegalArgumentException.class,
() -> ensureNoSelfReferences(map, "test with self ref nested value")
);
assertThat(e.getMessage(), containsString("Iterable object is self-referencing itself (test with self ref nested value)"));
}
}

public void testEnsureNoSelfReferencesSet() {
Set<Object> set = new HashSet<>();
set.add("foo");
set.add(set);

IllegalArgumentException e = expectThrows(
IllegalArgumentException.class,
() -> ensureNoSelfReferences(set, "test with self ref set")
);
assertThat(e.getMessage(), containsString("Iterable object is self-referencing itself (test with self ref set)"));
}

public void testEnsureNoSelfReferencesList() {
List<Object> list = new ArrayList<>();
list.add("foo");
list.add(list);

IllegalArgumentException e = expectThrows(
IllegalArgumentException.class,
() -> ensureNoSelfReferences(list, "test with self ref list")
);
assertThat(e.getMessage(), containsString("Iterable object is self-referencing itself (test with self ref list)"));
}

public void testEnsureNoSelfReferencesArray() {
Object[] array = new Object[2];
array[0] = "foo";
array[1] = array;

IllegalArgumentException e = expectThrows(
IllegalArgumentException.class,
() -> ensureNoSelfReferences(array, "test with self ref array")
);
assertThat(e.getMessage(), containsString("Iterable object is self-referencing itself (test with self ref array)"));
}

public void testLimitSizeOfShortList() {
Expand Down
3 changes: 1 addition & 2 deletions server/src/test/java/org/elasticsearch/common/xcontent/BaseXContentTestCase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
import org.elasticsearch.common.geo.GeoPoint;
import org.elasticsearch.common.text.Text;
import org.elasticsearch.common.unit.DistanceUnit;
import org.elasticsearch.common.util.CollectionUtils;
import org.elasticsearch.core.PathUtils;
import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.xcontent.NamedObjectNotFoundException;
Expand Down Expand Up @@ -954,7 +953,7 @@ public void testEnsureNoSelfReferences() throws IOException {

/**
* Test that the same map written multiple times do not trigger the self-reference check in
* {@link CollectionUtils#ensureNoSelfReferences(Object, String)} (Object)}
* {@link XContentBuilder#ensureNoSelfReferences(Object)}
*/
public void testRepeatedMapsAndNoSelfReferences() throws Exception {
Map<String, Object> mapB = singletonMap("b", "B");
Expand Down

0 comments on commit 94d16a2

Please sign in to comment.