Speed up Settings Filtering (#78632)

Settings filtering can make use of the fact that we're working with a prefix tree in a number of cases.
Also, we can avoid a lot of redundant empty settings instances here and there and have faster `exists`
checks by using a hash-set instead of a tree-set even if there's no secure settings.

server/src/main/java/org/elasticsearch/common/settings/IndexScopedSettings.java

@@ -39,7 +39,6 @@ import java.util.Collections;
 import java.util.HashSet;
 import java.util.Map;
 import java.util.Set;
-import java.util.function.Predicate;
 
 /**
  * Encapsulates all valid index level settings.
@@ -47,8 +46,6 @@ import java.util.function.Predicate;
  */
 public final class IndexScopedSettings extends AbstractScopedSettings {
 
-    public static final Predicate<String> INDEX_SETTINGS_KEY_PREDICATE = (s) -> s.startsWith(IndexMetadata.INDEX_SETTING_PREFIX);
-
     private static final Set<Setting<?>> ALWAYS_ENABLED_BUILT_IN_INDEX_SETTINGS = Set.of(
             MaxRetryAllocationDecider.SETTING_ALLOCATION_MAX_RETRY,
             MergeSchedulerConfig.AUTO_THROTTLE_SETTING,

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

@@ -23,6 +23,7 @@ import org.elasticsearch.common.logging.LogConfigurator;
 import org.elasticsearch.common.unit.ByteSizeUnit;
 import org.elasticsearch.common.unit.ByteSizeValue;
 import org.elasticsearch.common.unit.MemorySizeValue;
+import org.elasticsearch.core.Nullable;
 import org.elasticsearch.core.TimeValue;
 import org.elasticsearch.common.xcontent.DeprecationHandler;
 import org.elasticsearch.common.xcontent.LoggingDeprecationHandler;
@@ -52,6 +53,7 @@ import java.util.Iterator;
 import java.util.List;
 import java.util.ListIterator;
 import java.util.Map;
+import java.util.NavigableMap;
 import java.util.NoSuchElementException;
 import java.util.Objects;
 import java.util.Set;
@@ -71,10 +73,10 @@ import static org.elasticsearch.core.TimeValue.parseTimeValue;
  */
 public final class Settings implements ToXContentFragment {
 
-    public static final Settings EMPTY = new Builder().build();
+    public static final Settings EMPTY = new Settings(Map.of(), null);
 
     /** The raw settings from the full key to raw string value. */
-    private final Map<String, Object> settings;
+    private final NavigableMap<String, Object> settings;
 
     /** The secure settings storage associated with these settings. */
     private final SecureSettings secureSettings;
@@ -88,9 +90,16 @@ public final class Settings implements ToXContentFragment {
      */
     private Set<String> keys;
 
+    private static Settings of(Map<String, Object> settings, SecureSettings secureSettings) {
+        if (secureSettings == null && settings.isEmpty()) {
+            return EMPTY;
+        }
+        return new Settings(settings, secureSettings);
+    }
+
     private Settings(Map<String, Object> settings, SecureSettings secureSettings) {
         // we use a sorted map for consistent serialization when using getAsMap()
-        this.settings = Collections.unmodifiableSortedMap(new TreeMap<>(settings));
+        this.settings = Collections.unmodifiableNavigableMap(new TreeMap<>(settings));
         this.secureSettings = secureSettings;
     }
 
@@ -198,15 +207,24 @@ public final class Settings implements ToXContentFragment {
      * A settings that are filtered (and key is removed) with the specified prefix.
      */
     public Settings getByPrefix(String prefix) {
-        return new Settings(new FilteredMap(this.settings, (k) -> k.startsWith(prefix), prefix), secureSettings == null ? null :
-            new PrefixedSecureSettings(secureSettings, prefix, s -> s.startsWith(prefix)));
+        if (prefix.isEmpty()) {
+            return this;
+        }
+        // create the the next prefix right after the given prefix, and use it as exclusive upper bound for the sub-map to filter by prefix
+        // below
+        char[] toPrefixCharArr = prefix.toCharArray();
+        toPrefixCharArr[toPrefixCharArr.length - 1]++;
+        String toPrefix = new String(toPrefixCharArr);
+        final Map<String, Object> subMap = settings.subMap(prefix, toPrefix);
+        return Settings.of(subMap.isEmpty() ? Map.of() : new FilteredMap(subMap, null, prefix),
+            secureSettings == null ? null : new PrefixedSecureSettings(secureSettings, prefix, s -> s.startsWith(prefix)));
     }
 
     /**
      * 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) {
-        return new Settings(new FilteredMap(this.settings, predicate, null), secureSettings == null ? null :
+        return Settings.of(new FilteredMap(this.settings, predicate, null), secureSettings == null ? null :
             new PrefixedSecureSettings(secureSettings, "", predicate));
     }
 
@@ -436,6 +454,9 @@ public final class Settings implements ToXContentFragment {
 
     private Map<String, Settings> getGroupsInternal(String settingPrefix, boolean ignoreNonGrouped) throws SettingsException {
         Settings prefixSettings = getByPrefix(settingPrefix);
+        if (prefixSettings.isEmpty()) {
+            return Map.of();
+        }
         Map<String, Settings> groups = new HashMap<>();
         for (String groupName : prefixSettings.names()) {
             Settings groupSettings = prefixSettings.getByPrefix(groupName + ".");
@@ -523,8 +544,11 @@ public final class Settings implements ToXContentFragment {
     }
 
     public static Settings readSettingsFromStream(StreamInput in) throws IOException {
-        Builder builder = new Builder();
         int numberOfSettings = in.readVInt();
+        if (numberOfSettings == 0) {
+            return EMPTY;
+        }
+        Builder builder = new Builder();
         for (int i = 0; i < numberOfSettings; i++) {
             String key = in.readString();
             Object value = in.readGenericValue();
@@ -689,12 +713,12 @@ public final class Settings implements ToXContentFragment {
         }
         final Set<String> newKeySet;
         if (secureSettings == null) {
-            newKeySet = settings.keySet();
+            newKeySet = Set.copyOf(settings.keySet());
         } else {
             // uniquify, since for legacy reasons the same setting name may exist in both
             final Set<String> merged = new HashSet<>(settings.keySet());
             merged.addAll(secureSettings.getSettingNames());
-            newKeySet = Collections.unmodifiableSet(merged);
+            newKeySet = Set.copyOf(merged);
         }
         keys = newKeySet;
         return newKeySet;
@@ -1182,19 +1206,25 @@ public final class Settings implements ToXContentFragment {
          * set on this builder.
          */
         public Settings build() {
+            final SecureSettings secSettings = secureSettings.get();
+            if (secSettings == null && map.isEmpty()) {
+                return EMPTY;
+            }
             processLegacyLists(map);
-            return new Settings(map, secureSettings.get());
+            return new Settings(map, secSettings);
         }
     }
 
     // TODO We could use an FST internally to make things even faster and more compact
     private static final class FilteredMap extends AbstractMap<String, Object> {
         private final Map<String, Object> delegate;
+        @Nullable
         private final Predicate<String> filter;
+        @Nullable
         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;
+        private int size;
         @Override
         public Set<Entry<String, Object>> entrySet() {
             Set<Entry<String, Object>> delegateSet = delegate.entrySet();
@@ -1217,7 +1247,7 @@ public final class Settings implements ToXContentFragment {
                                     return false;
                                 }
                                 while (iter.hasNext()) {
-                                    if (filter.test((currentElement = iter.next()).getKey())) {
+                                    if (test((currentElement = iter.next()).getKey())) {
                                         numIterated++;
                                         return true;
                                     }
@@ -1271,24 +1301,33 @@ public final class Settings implements ToXContentFragment {
             this.delegate = delegate;
             this.filter = filter;
             this.prefix = prefix;
+            if (filter == null) {
+                this.size = delegate.size();
+            } else {
+                this.size = -1;
+            }
         }
 
         @Override
         public Object get(Object key) {
             if (key instanceof String) {
                 final String theKey = prefix == null ? (String)key : prefix + key;
-                if (filter.test(theKey)) {
+                if (test(theKey)) {
                     return delegate.get(theKey);
                 }
             }
             return null;
         }
 
+        private boolean test(String theKey) {
+            return filter == null || filter.test(theKey);
+        }
+
         @Override
         public boolean containsKey(Object key) {
             if (key instanceof String) {
                 final String theKey = prefix == null ? (String) key : prefix + key;
-                if (filter.test(theKey)) {
+                if (test(theKey)) {
                     return delegate.containsKey(theKey);
                 }
             }

server/src/main/java/org/elasticsearch/index/IndexSettings.java

@@ -759,8 +759,10 @@ public final class IndexSettings {
      * @return true if the settings are the same, otherwise false
      */
     public static boolean same(final Settings left, final Settings right) {
-        return left.filter(IndexScopedSettings.INDEX_SETTINGS_KEY_PREDICATE)
-                .equals(right.filter(IndexScopedSettings.INDEX_SETTINGS_KEY_PREDICATE));
+        if (left.equals(right)) {
+            return true;
+        }
+        return left.getByPrefix(IndexMetadata.INDEX_SETTING_PREFIX).equals(right.getByPrefix(IndexMetadata.INDEX_SETTING_PREFIX));
     }
 
     /**

server/src/test/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdaterTests.java

@@ -29,6 +29,7 @@ import java.util.stream.Stream;
 
 import static java.util.Arrays.asList;
 import static org.elasticsearch.common.settings.AbstractScopedSettings.ARCHIVED_SETTINGS_PREFIX;
+import static org.hamcrest.Matchers.either;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.hasItem;
 import static org.hamcrest.Matchers.not;
@@ -603,7 +604,8 @@ public class SettingsUpdaterTests extends ESTestCase {
         Exception exception = expectThrows(IllegalArgumentException.class, () ->
             updater.updateSettings(finalCluster, Settings.builder().put(SETTING_FOO_HIGH.getKey(), 2).build(), Settings.EMPTY, logger));
 
-        assertThat(exception.getMessage(), equalTo("[high]=2 is lower than [low]=5"));
+        assertThat(exception.getMessage(),
+            either(equalTo("[high]=2 is lower than [low]=5")).or(equalTo("[low]=5 is higher than [high]=2")));
     }
 
 }

server/src/test/java/org/elasticsearch/common/settings/SettingsModuleTests.java

@@ -19,6 +19,7 @@ import java.util.function.Supplier;
 
 import static java.util.Collections.emptySet;
 import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.either;
 import static org.hamcrest.Matchers.hasToString;
 import static org.hamcrest.Matchers.is;
 
@@ -30,11 +31,12 @@ public class SettingsModuleTests extends ModuleTestCase {
             SettingsModule module = new SettingsModule(settings);
             assertInstanceBinding(module, Settings.class, (s) -> s == settings);
         }
+        final String balanceSettingMessage = "Failed to parse value [[2.0]] for setting [cluster.routing.allocation.balance.shard]";
         {
             Settings settings = Settings.builder().put("cluster.routing.allocation.balance.shard", "[2.0]").build();
             IllegalArgumentException ex = expectThrows(IllegalArgumentException.class,
                 () ->  new SettingsModule(settings));
-            assertEquals("Failed to parse value [[2.0]] for setting [cluster.routing.allocation.balance.shard]", ex.getMessage());
+            assertEquals(balanceSettingMessage, ex.getMessage());
         }
 
         {
@@ -42,10 +44,16 @@ public class SettingsModuleTests extends ModuleTestCase {
                 .put("some.foo.bar", 1).build();
             IllegalArgumentException ex = expectThrows(IllegalArgumentException.class,
                 () -> new SettingsModule(settings));
-            assertEquals("Failed to parse value [[2.0]] for setting [cluster.routing.allocation.balance.shard]", ex.getMessage());
+            final String unknownSettingMessage =
+                "unknown setting [some.foo.bar] please check that any required plugins are installed, or check the breaking "
+                    + "changes documentation for removed settings";
             assertEquals(1, ex.getSuppressed().length);
-            assertEquals("unknown setting [some.foo.bar] please check that any required plugins are installed, or check the breaking " +
-                "changes documentation for removed settings", ex.getSuppressed()[0].getMessage());
+            if (ex.getMessage().equals(balanceSettingMessage)) {
+                assertEquals(unknownSettingMessage, ex.getSuppressed()[0].getMessage());
+            } else {
+                assertEquals(unknownSettingMessage, ex.getMessage());
+                assertEquals(balanceSettingMessage, ex.getSuppressed()[0].getMessage());
+            }
         }
 
         {
@@ -124,7 +132,7 @@ public class SettingsModuleTests extends ModuleTestCase {
         {
             Settings settings = Settings.builder().put("logger._root", "BOOM").put("logger.transport", "WOW").build();
             IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, () -> new SettingsModule(settings));
-            assertEquals("Unknown level constant [BOOM].", ex.getMessage());
+            assertThat(ex.getMessage(), either(is("Unknown level constant [BOOM].")).or(is("Unknown level constant [WOW].")));
         }
     }
 

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

@@ -34,6 +34,7 @@ import java.util.Iterator;
 import java.util.Map;
 import java.util.NoSuchElementException;
 import java.util.Set;
+import java.util.TreeSet;
 import java.util.concurrent.TimeUnit;
 
 import static org.hamcrest.Matchers.contains;
@@ -318,7 +319,7 @@ public class SettingsTests extends ESTestCase {
         assertEquals("ab2", filteredSettings.get("a.b.c"));
         assertEquals("ab3", filteredSettings.get("a.b.c.d"));
 
-        Iterator<String> iterator = filteredSettings.keySet().iterator();
+        Iterator<String> iterator = new TreeSet<>(filteredSettings.keySet()).iterator();
         for (int i = 0; i < 10; i++) {
             assertTrue(iterator.hasNext());
         }
@@ -364,7 +365,7 @@ public class SettingsTests extends ESTestCase {
         assertEquals("ab1", prefixMap.get("b"));
         assertEquals("ab2", prefixMap.get("b.c"));
         assertEquals("ab3", prefixMap.get("b.c.d"));
-        Iterator<String> prefixIterator = prefixMap.keySet().iterator();
+        Iterator<String> prefixIterator = new TreeSet<>(prefixMap.keySet()).iterator();
         for (int i = 0; i < 10; i++) {
             assertTrue(prefixIterator.hasNext());
         }