Speed up filter and prefix settings operations (#22249)

Today if a settings object has many keys ie. if somebody specifies
a gazillion synonym in-line (arrays are keys ending with ordinals) operations like
`Settings#getByPrefix` have a linear runtime. This can cause index creations to be
very slow producing lots of garbage at the same time. Yet, `Settings#getByPrefix` is called
quite frequently by group settings etc. which can cause heavy load on the system.

While it's not recommended to have synonym lists with 25k entries in-line these use-cases should
not have such a large impact on the cluster / node. This change introduces a view-like map
that filters based on the prefixes referencing the actual source map instead of copying all values
over and over again. A benchmark that adds a single key with 25k random synonyms between 2 and 5 chars
takes 16 seconds to get the synonym prefix 200 times while the filtered view takes 4 ms for the 200 iterations.

This relates to https://discuss.elastic.co/t/200-cpu-elasticsearch-5-index-creation-very-slow-with-a-huge-synonyms-list/69052

core/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java

@@ -239,11 +239,9 @@ public abstract class AbstractScopedSettings extends AbstractComponent {
      */
     public final void validate(Settings settings) {
         List<RuntimeException> exceptions = new ArrayList<>();
-        // we want them sorted for deterministic error messages
+        for (String key : settings.getAsMap().keySet()) { // settings iterate in deterministic fashion
-        SortedMap<String, String> sortedSettings = new TreeMap<>(settings.getAsMap());
-        for (Map.Entry<String, String> entry : sortedSettings.entrySet()) {
             try {
-                validate(entry.getKey(), settings);
+                validate(key, settings);
             } catch (RuntimeException ex) {
                 exceptions.add(ex);
             }

core/src/main/java/org/elasticsearch/common/settings/Setting.java

@@ -817,7 +817,9 @@ public class Setting<T> extends ToXContentToBytes {
 
                     @Override
                     public void apply(Settings value, Settings current, Settings previous) {
+                        if (logger.isInfoEnabled()) { // getRaw can create quite some objects
                             logger.info("updating [{}] from [{}] to [{}]", key, getRaw(previous), getRaw(current));
+                        }
                         consumer.accept(value);
                     }
 

core/src/main/java/org/elasticsearch/common/settings/Settings.java

@@ -42,6 +42,8 @@ import java.io.InputStreamReader;
 import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.nio.file.Path;
+import java.util.AbstractMap;
+import java.util.AbstractSet;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
@@ -52,16 +54,15 @@ import java.util.Iterator;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.NoSuchElementException;
 import java.util.Objects;
 import java.util.Set;
-import java.util.SortedMap;
 import java.util.TreeMap;
 import java.util.concurrent.TimeUnit;
 import java.util.function.Function;
 import java.util.function.Predicate;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
-import java.util.stream.Collectors;
 
 import static org.elasticsearch.common.unit.ByteSizeValue.parseBytesSizeValue;
 import static org.elasticsearch.common.unit.SizeValue.parseSizeValue;
@@ -75,11 +76,10 @@ public final class Settings implements ToXContent {
     public static final Settings EMPTY = new Builder().build();
     private static final Pattern ARRAY_PATTERN = Pattern.compile("(.*)\\.\\d+$");
 
-    private SortedMap<String, String> settings;
+    private Map<String, String> settings;
 
     Settings(Map<String, String> settings) {
-        // we use a sorted map for consistent serialization when using getAsMap()
+        this.settings = Collections.unmodifiableMap(settings);
-        this.settings = Collections.unmodifiableSortedMap(new TreeMap<>(settings));
     }
 
     /**
@@ -87,7 +87,8 @@ public final class Settings implements ToXContent {
      * @return an unmodifiable map of settings
      */
     public Map<String, String> getAsMap() {
-        return Collections.unmodifiableMap(this.settings);
+        // settings is always unmodifiable
+        return this.settings;
     }
 
     /**
@@ -186,30 +187,14 @@ public final class Settings implements ToXContent {
      * A settings that are filtered (and key is removed) with the specified prefix.
      */
     public Settings getByPrefix(String prefix) {
-        Builder builder = new Builder();
+        return new Settings(new FilteredMap(this.settings, (k) -> k.startsWith(prefix), prefix));
-        for (Map.Entry<String, String> entry : getAsMap().entrySet()) {
-            if (entry.getKey().startsWith(prefix)) {
-                if (entry.getKey().length() < prefix.length()) {
-                    // ignore this. one
-                    continue;
-                }
-                builder.put(entry.getKey().substring(prefix.length()), entry.getValue());
-            }
-        }
-        return builder.build();
     }
 
     /**
      * Returns a new settings object that contains all setting of the current one filtered by the given settings key predicate.
      */
     public Settings filter(Predicate<String> predicate) {
-        Builder builder = new Builder();
+        return new Settings(new FilteredMap(this.settings, predicate, null));
-        for (Map.Entry<String, String> entry : getAsMap().entrySet()) {
-            if (predicate.test(entry.getKey())) {
-                builder.put(entry.getKey(), entry.getValue());
-            }
-        }
-        return builder.build();
     }
 
     /**
@@ -443,6 +428,7 @@ public final class Settings implements ToXContent {
         }
         return getGroupsInternal(settingPrefix, ignoreNonGrouped);
     }
+
     private Map<String, Settings> getGroupsInternal(String settingPrefix, boolean ignoreNonGrouped) throws SettingsException {
         // we don't really care that it might happen twice
         Map<String, Map<String, String>> map = new LinkedHashMap<>();
@@ -602,7 +588,8 @@ public final class Settings implements ToXContent {
 
         public static final Settings EMPTY_SETTINGS = new Builder().build();
 
-        private final Map<String, String> map = new LinkedHashMap<>();
+        // we use a sorted map for consistent serialization when using getAsMap()
+        private final Map<String, String> map = new TreeMap<>();
 
         private Builder() {
 
@@ -1032,7 +1019,124 @@ public final class Settings implements ToXContent {
          * set on this builder.
          */
         public Settings build() {
-            return new Settings(Collections.unmodifiableMap(map));
+            return new Settings(map);
+        }
+    }
+
+    // TODO We could use an FST internally to make things even faster and more compact
+    private static final class FilteredMap extends AbstractMap<String, String> {
+        private final Map<String, String> delegate;
+        private final Predicate<String> filter;
+        private final String prefix;
+        // we cache that size since we have to iterate the entire set
+        // this is safe to do since this map is only used with unmodifiable maps
+        private int size = -1;
+        @Override
+        public Set<Entry<String, String>> entrySet() {
+            Set<Entry<String, String>> delegateSet = delegate.entrySet();
+            AbstractSet<Entry<String, String>> filterSet = new AbstractSet<Entry<String, String>>() {
+
+                @Override
+                public Iterator<Entry<String, String>> iterator() {
+                    Iterator<Entry<String, String>> iter = delegateSet.iterator();
+
+                    return new Iterator<Entry<String, String>>() {
+                        private int numIterated;
+                        private Entry<String, String> currentElement;
+                        @Override
+                        public boolean hasNext() {
+                            if (currentElement != null) {
+                                return true; // protect against calling hasNext twice
+                            } else {
+                                if (numIterated == size) { // early terminate
+                                    assert size != -1 : "size was never set: " + numIterated + " vs. " + size;
+                                    return false;
+                                }
+                                while (iter.hasNext()) {
+                                    if (filter.test((currentElement = iter.next()).getKey())) {
+                                        numIterated++;
+                                        return true;
+                                    }
+                                }
+                                // we didn't find anything
+                                currentElement = null;
+                                return false;
+                            }
+                        }
+
+                        @Override
+                        public Entry<String, String> next() {
+                            if (currentElement == null && hasNext() == false) { // protect against no #hasNext call or not respecting it
+
+                                throw new NoSuchElementException("make sure to call hasNext first");
+                            }
+                            final Entry<String, String> current = this.currentElement;
+                            this.currentElement = null;
+                            if (prefix == null) {
+                                return current;
+                            }
+                            return new Entry<String, String>() {
+                                @Override
+                                public String getKey() {
+                                    return current.getKey().substring(prefix.length());
+                                }
+
+                                @Override
+                                public String getValue() {
+                                    return current.getValue();
+                                }
+
+                                @Override
+                                public String setValue(String value) {
+                                    throw new UnsupportedOperationException();
+                                }
+                            };
+                        }
+                    };
+                }
+
+                @Override
+                public int size() {
+                    return FilteredMap.this.size();
+                }
+            };
+            return filterSet;
+        }
+
+        private FilteredMap(Map<String, String> delegate, Predicate<String> filter, String prefix) {
+            this.delegate = delegate;
+            this.filter = filter;
+            this.prefix = prefix;
+        }
+
+        @Override
+        public String get(Object key) {
+            if (key instanceof String) {
+                final String theKey = prefix == null ? (String)key : prefix + key;
+                if (filter.test(theKey)) {
+                    return delegate.get(theKey);
+                }
+            }
+            return null;
+        }
+
+        @Override
+        public boolean containsKey(Object key) {
+            if (key instanceof String) {
+                final String theKey = prefix == null ? (String) key : prefix + key;
+                if (filter.test(theKey)) {
+                    return delegate.containsKey(theKey);
+                }
+            }
+            return false;
+        }
+
+        @Override
+        public int size() {
+            if (size == -1) {
+                size = Math.toIntExact(delegate.keySet().stream().filter((e) -> filter.test(e)).count());
+            }
+            return size;
         }
     }
 }

core/src/test/java/org/elasticsearch/common/settings/SettingsTests.java

@@ -20,12 +20,17 @@
 package org.elasticsearch.common.settings;
 
 import org.elasticsearch.common.settings.loader.YamlSettingsLoader;
+import org.elasticsearch.common.unit.TimeValue;
 import org.elasticsearch.test.ESTestCase;
 import org.hamcrest.Matchers;
 
 import java.io.IOException;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
+import java.util.NoSuchElementException;
 import java.util.Set;
 
 import static org.hamcrest.Matchers.allOf;
@@ -131,15 +136,49 @@ public class SettingsTests extends ESTestCase {
 
     public void testGetAsSettings() {
         Settings settings = Settings.builder()
+                .put("bar", "hello world")
                 .put("foo", "abc")
                 .put("foo.bar", "def")
                 .put("foo.baz", "ghi").build();
 
         Settings fooSettings = settings.getAsSettings("foo");
+        assertFalse(fooSettings.isEmpty());
+        assertEquals(2, fooSettings.getAsMap().size());
         assertThat(fooSettings.get("bar"), equalTo("def"));
         assertThat(fooSettings.get("baz"), equalTo("ghi"));
     }
 
+    public void testMultLevelGetPrefix() {
+        Settings settings = Settings.builder()
+            .put("1.2.3", "hello world")
+            .put("1.2.3.4", "abc")
+            .put("2.3.4", "def")
+            .put("3.4", "ghi").build();
+
+        Settings firstLevelSettings = settings.getByPrefix("1.");
+        assertFalse(firstLevelSettings.isEmpty());
+        assertEquals(2, firstLevelSettings.getAsMap().size());
+        assertThat(firstLevelSettings.get("2.3.4"), equalTo("abc"));
+        assertThat(firstLevelSettings.get("2.3"), equalTo("hello world"));
+
+        Settings secondLevelSetting = firstLevelSettings.getByPrefix("2.");
+        assertFalse(secondLevelSetting.isEmpty());
+        assertEquals(2, secondLevelSetting.getAsMap().size());
+        assertNull(secondLevelSetting.get("2.3.4"));
+        assertNull(secondLevelSetting.get("1.2.3.4"));
+        assertNull(secondLevelSetting.get("1.2.3"));
+        assertThat(secondLevelSetting.get("3.4"), equalTo("abc"));
+        assertThat(secondLevelSetting.get("3"), equalTo("hello world"));
+
+        Settings thirdLevelSetting = secondLevelSetting.getByPrefix("3.");
+        assertFalse(thirdLevelSetting.isEmpty());
+        assertEquals(1, thirdLevelSetting.getAsMap().size());
+        assertNull(thirdLevelSetting.get("2.3.4"));
+        assertNull(thirdLevelSetting.get("3.4"));
+        assertNull(thirdLevelSetting.get("1.2.3"));
+        assertThat(thirdLevelSetting.get("4"), equalTo("abc"));
+    }
+
     public void testNames() {
         Settings settings = Settings.builder()
                 .put("bar", "baz")
@@ -298,4 +337,148 @@ public class SettingsTests extends ESTestCase {
         assertThat(settings.getAsMap().size(), equalTo(1));
         assertThat(settings.get("foo.test"), equalTo("test"));
     }
+
+    public void testFilteredMap() {
+        Settings.Builder builder = Settings.builder();
+        builder.put("a", "a1");
+        builder.put("a.b", "ab1");
+        builder.put("a.b.c", "ab2");
+        builder.put("a.c", "ac1");
+        builder.put("a.b.c.d", "ab3");
+
+
+        Map<String, String> fiteredMap = builder.build().filter((k) -> k.startsWith("a.b")).getAsMap();
+        assertEquals(3, fiteredMap.size());
+        int numKeys = 0;
+        for (String k : fiteredMap.keySet()) {
+            numKeys++;
+            assertTrue(k.startsWith("a.b"));
+        }
+
+        assertEquals(3, numKeys);
+        int numValues = 0;
+
+        for (String v : fiteredMap.values()) {
+            numValues++;
+            assertTrue(v.startsWith("ab"));
+        }
+        assertEquals(3, numValues);
+        assertFalse(fiteredMap.containsKey("a.c"));
+        assertFalse(fiteredMap.containsKey("a"));
+        assertTrue(fiteredMap.containsKey("a.b"));
+        assertTrue(fiteredMap.containsKey("a.b.c"));
+        assertTrue(fiteredMap.containsKey("a.b.c.d"));
+        expectThrows(UnsupportedOperationException.class, () ->
+            fiteredMap.remove("a.b"));
+        assertEquals("ab1", fiteredMap.get("a.b"));
+        assertEquals("ab2", fiteredMap.get("a.b.c"));
+        assertEquals("ab3", fiteredMap.get("a.b.c.d"));
+
+        Iterator<String> iterator = fiteredMap.keySet().iterator();
+        for (int i = 0; i < 10; i++) {
+            assertTrue(iterator.hasNext());
+        }
+        assertEquals("a.b", iterator.next());
+        if (randomBoolean()) {
+            assertTrue(iterator.hasNext());
+        }
+        assertEquals("a.b.c", iterator.next());
+        if (randomBoolean()) {
+            assertTrue(iterator.hasNext());
+        }
+        assertEquals("a.b.c.d", iterator.next());
+        assertFalse(iterator.hasNext());
+        expectThrows(NoSuchElementException.class, () -> iterator.next());
+
+    }
+
+    public void testPrefixMap() {
+        Settings.Builder builder = Settings.builder();
+        builder.put("a", "a1");
+        builder.put("a.b", "ab1");
+        builder.put("a.b.c", "ab2");
+        builder.put("a.c", "ac1");
+        builder.put("a.b.c.d", "ab3");
+
+        Map<String, String> prefixMap = builder.build().getByPrefix("a.").getAsMap();
+        assertEquals(4, prefixMap.size());
+        int numKeys = 0;
+        for (String k : prefixMap.keySet()) {
+            numKeys++;
+            assertTrue(k, k.startsWith("b") || k.startsWith("c"));
+        }
+
+        assertEquals(4, numKeys);
+        int numValues = 0;
+
+        for (String v : prefixMap.values()) {
+            numValues++;
+            assertTrue(v, v.startsWith("ab") || v.startsWith("ac"));
+        }
+        assertEquals(4, numValues);
+        assertFalse(prefixMap.containsKey("a"));
+        assertTrue(prefixMap.containsKey("c"));
+        assertTrue(prefixMap.containsKey("b"));
+        assertTrue(prefixMap.containsKey("b.c"));
+        assertTrue(prefixMap.containsKey("b.c.d"));
+        expectThrows(UnsupportedOperationException.class, () ->
+            prefixMap.remove("a.b"));
+        assertEquals("ab1", prefixMap.get("b"));
+        assertEquals("ab2", prefixMap.get("b.c"));
+        assertEquals("ab3", prefixMap.get("b.c.d"));
+        Iterator<String> prefixIterator = prefixMap.keySet().iterator();
+        for (int i = 0; i < 10; i++) {
+            assertTrue(prefixIterator.hasNext());
+        }
+        assertEquals("b", prefixIterator.next());
+        if (randomBoolean()) {
+            assertTrue(prefixIterator.hasNext());
+        }
+        assertEquals("b.c", prefixIterator.next());
+        if (randomBoolean()) {
+            assertTrue(prefixIterator.hasNext());
+        }
+        assertEquals("b.c.d", prefixIterator.next());
+        if (randomBoolean()) {
+            assertTrue(prefixIterator.hasNext());
+        }
+        assertEquals("c", prefixIterator.next());
+        assertFalse(prefixIterator.hasNext());
+        expectThrows(NoSuchElementException.class, () -> prefixIterator.next());
+    }
+
+    public void testEmptyFilterMap() {
+        Settings.Builder builder = Settings.builder();
+        builder.put("a", "a1");
+        builder.put("a.b", "ab1");
+        builder.put("a.b.c", "ab2");
+        builder.put("a.c", "ac1");
+        builder.put("a.b.c.d", "ab3");
+
+        Map<String, String> fiteredMap = builder.build().filter((k) -> false).getAsMap();
+        assertEquals(0, fiteredMap.size());
+        for (String k : fiteredMap.keySet()) {
+            fail("no element");
+
+        }
+        for (String v : fiteredMap.values()) {
+            fail("no element");
+        }
+        assertFalse(fiteredMap.containsKey("a.c"));
+        assertFalse(fiteredMap.containsKey("a"));
+        assertFalse(fiteredMap.containsKey("a.b"));
+        assertFalse(fiteredMap.containsKey("a.b.c"));
+        assertFalse(fiteredMap.containsKey("a.b.c.d"));
+        expectThrows(UnsupportedOperationException.class, () ->
+            fiteredMap.remove("a.b"));
+        assertNull(fiteredMap.get("a.b"));
+        assertNull(fiteredMap.get("a.b.c"));
+        assertNull(fiteredMap.get("a.b.c.d"));
+
+        Iterator<String> iterator = fiteredMap.keySet().iterator();
+        for (int i = 0; i < 10; i++) {
+            assertFalse(iterator.hasNext());
+        }
+        expectThrows(NoSuchElementException.class, () -> iterator.next());
+    }
 }