From ce5c094cda7b7638e586e797adc066411cb49534 Mon Sep 17 00:00:00 2001 From: Simon Willnauer Date: Mon, 19 Dec 2016 10:48:38 +0100 Subject: [PATCH] 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 --- .../settings/AbstractScopedSettings.java | 6 +- .../common/settings/Setting.java | 4 +- .../common/settings/Settings.java | 156 ++++++++++++--- .../common/settings/SettingsTests.java | 183 ++++++++++++++++++ 4 files changed, 318 insertions(+), 31 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java b/core/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java index 3622623987b..89a56f03ecc 100644 --- a/core/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java +++ b/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 exceptions = new ArrayList<>(); - // we want them sorted for deterministic error messages - SortedMap sortedSettings = new TreeMap<>(settings.getAsMap()); - for (Map.Entry entry : sortedSettings.entrySet()) { + for (String key : settings.getAsMap().keySet()) { // settings iterate in deterministic fashion try { - validate(entry.getKey(), settings); + validate(key, settings); } catch (RuntimeException ex) { exceptions.add(ex); } diff --git a/core/src/main/java/org/elasticsearch/common/settings/Setting.java b/core/src/main/java/org/elasticsearch/common/settings/Setting.java index 22c74afee7c..5d9adbc34c1 100644 --- a/core/src/main/java/org/elasticsearch/common/settings/Setting.java +++ b/core/src/main/java/org/elasticsearch/common/settings/Setting.java @@ -817,7 +817,9 @@ public class Setting extends ToXContentToBytes { @Override public void apply(Settings value, Settings current, Settings previous) { - logger.info("updating [{}] from [{}] to [{}]", key, getRaw(previous), getRaw(current)); + if (logger.isInfoEnabled()) { // getRaw can create quite some objects + logger.info("updating [{}] from [{}] to [{}]", key, getRaw(previous), getRaw(current)); + } consumer.accept(value); } diff --git a/core/src/main/java/org/elasticsearch/common/settings/Settings.java b/core/src/main/java/org/elasticsearch/common/settings/Settings.java index 819edc246ac..67bf8081479 100644 --- a/core/src/main/java/org/elasticsearch/common/settings/Settings.java +++ b/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 settings; + private Map settings; Settings(Map settings) { - // we use a sorted map for consistent serialization when using getAsMap() - this.settings = Collections.unmodifiableSortedMap(new TreeMap<>(settings)); + this.settings = Collections.unmodifiableMap(settings); } /** @@ -87,7 +87,8 @@ public final class Settings implements ToXContent { * @return an unmodifiable map of settings */ public Map 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(); - for (Map.Entry 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(); + return new Settings(new FilteredMap(this.settings, (k) -> k.startsWith(prefix), 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 predicate) { - Builder builder = new Builder(); - for (Map.Entry entry : getAsMap().entrySet()) { - if (predicate.test(entry.getKey())) { - builder.put(entry.getKey(), entry.getValue()); - } - } - return builder.build(); + return new Settings(new FilteredMap(this.settings, predicate, null)); } /** @@ -443,6 +428,7 @@ public final class Settings implements ToXContent { } return getGroupsInternal(settingPrefix, ignoreNonGrouped); } + private Map getGroupsInternal(String settingPrefix, boolean ignoreNonGrouped) throws SettingsException { // we don't really care that it might happen twice Map> 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 map = new LinkedHashMap<>(); + // we use a sorted map for consistent serialization when using getAsMap() + private final Map 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 { + private final Map delegate; + private final Predicate 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> entrySet() { + Set> delegateSet = delegate.entrySet(); + AbstractSet> filterSet = new AbstractSet>() { + + @Override + public Iterator> iterator() { + Iterator> iter = delegateSet.iterator(); + + return new Iterator>() { + private int numIterated; + private Entry 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 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 current = this.currentElement; + this.currentElement = null; + if (prefix == null) { + return current; + } + return new Entry() { + @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 delegate, Predicate 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; } } } diff --git a/core/src/test/java/org/elasticsearch/common/settings/SettingsTests.java b/core/src/test/java/org/elasticsearch/common/settings/SettingsTests.java index 346c5bc60de..62fa9ec82c4 100644 --- a/core/src/test/java/org/elasticsearch/common/settings/SettingsTests.java +++ b/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 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 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 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 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 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 iterator = fiteredMap.keySet().iterator(); + for (int i = 0; i < 10; i++) { + assertFalse(iterator.hasNext()); + } + expectThrows(NoSuchElementException.class, () -> iterator.next()); + } }