From afee04da9ef0e5cfeacb53a0dcd72993a98c1ae4 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Wed, 24 Aug 2022 14:08:56 -0400 Subject: [PATCH 1/2] Speed up synthetic source again When I added support for stored fields to synthetic _source (#87480) I accidentally caused a performance regression. Our friends working on building the nightly charts for tsdb caught it. It looked like: ``` | 50th percentile latency | default_1k | 20.1228 | 41.289 | 21.1662 | ms | +105.18% | | 90th percentile latency | default_1k | 26.7402 | 42.5878 | 15.8476 | ms | +59.27% | | 99th percentile latency | default_1k | 37.0881 | 45.586 | 8.49786 | ms | +22.91% | | 99.9th percentile latency | default_1k | 43.7346 | 48.222 | 4.48742 | ms | +10.26% | | 100th percentile latency | default_1k | 46.057 | 56.8676 | 10.8106 | ms | +23.47% | ``` This fixes the regression and puts us in line with how we were: ``` | 50th percentile latency | default_1k | 20.1228 | 24.023 | 3.90022 | ms | +19.38% | | 90th percentile latency | default_1k | 26.7402 | 29.7841 | 3.04392 | ms | +11.38% | | 99th percentile latency | default_1k | 37.0881 | 36.8038 | -0.28428 | ms | -0.77% | | 99.9th percentile latency | default_1k | 43.7346 | 39.0192 | -4.71531 | ms | -10.78% | | 100th percentile latency | default_1k | 46.057 | 42.9181 | -3.13889 | ms | -6.82% | ``` A 20% bump in the 50% latency isn't great, but it four microseconds per document which is acceptable. --- .../index/mapper/ObjectMapper.java | 18 ++++++++++++++++-- .../index/mapper/SourceLoader.java | 18 ++++++++++++++---- 2 files changed, 30 insertions(+), 6 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java index f391b0c9a7833..8c3c16f97b574 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java @@ -604,7 +604,21 @@ public DocValuesLoader docValuesLoader(LeafReader leafReader, int[] docIdsInLeaf loaders.add(loader); } } - return docId -> { + if (loaders.isEmpty()) { + return null; + } + return new ObjectDocValuesLoader(loaders); + } + + private class ObjectDocValuesLoader implements DocValuesLoader { + private final List loaders; + + private ObjectDocValuesLoader(List loaders) { + this.loaders = loaders; + } + + @Override + public boolean advanceToDoc(int docId) throws IOException { for (SourceLoader.SyntheticFieldLoader.DocValuesLoader docValueLoader : loaders) { boolean leafHasValue = docValueLoader.advanceToDoc(docId); hasValue |= leafHasValue; @@ -616,7 +630,7 @@ public DocValuesLoader docValuesLoader(LeafReader leafReader, int[] docIdsInLeaf * stored field. */ return hasValue; - }; + } } @Override diff --git a/server/src/main/java/org/elasticsearch/index/mapper/SourceLoader.java b/server/src/main/java/org/elasticsearch/index/mapper/SourceLoader.java index 70111f1e48ffe..83f60a7d7f35b 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/SourceLoader.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/SourceLoader.java @@ -106,8 +106,18 @@ public Set requiredStoredFields() { @Override public Leaf leaf(LeafReader reader, int[] docIdsInLeaf) throws IOException { - SyntheticFieldLoader.DocValuesLoader leaf = loader.docValuesLoader(reader, docIdsInLeaf); - return (fieldsVisitor, docId) -> { + return new SyntheticLeaf(loader.docValuesLoader(reader, docIdsInLeaf)); + } + + private class SyntheticLeaf implements Leaf { + private final SyntheticFieldLoader.DocValuesLoader docValuesLoader; + + private SyntheticLeaf(SyntheticFieldLoader.DocValuesLoader docValuesLoader) { + this.docValuesLoader = docValuesLoader; + } + + @Override + public BytesReference source(FieldsVisitor fieldsVisitor, int docId) throws IOException { if (fieldsVisitor != null) { for (Map.Entry> e : fieldsVisitor.fields().entrySet()) { SyntheticFieldLoader.StoredFieldLoader loader = storedFieldLoaders.get(e.getKey()); @@ -118,14 +128,14 @@ public Leaf leaf(LeafReader reader, int[] docIdsInLeaf) throws IOException { } // TODO accept a requested xcontent type try (XContentBuilder b = new XContentBuilder(JsonXContent.jsonXContent, new ByteArrayOutputStream())) { - if (leaf.advanceToDoc(docId)) { + if (docValuesLoader.advanceToDoc(docId)) { loader.write(b); } else { b.startObject().endObject(); } return BytesReference.bytes(b); } - }; + } } } From 430099b5ea1c980db3a066846a145f5c46cc500a Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Thu, 25 Aug 2022 10:02:05 -0400 Subject: [PATCH 2/2] Better split! --- .../index/mapper/ObjectMapper.java | 21 +++++----- ...dNumericDocValuesSyntheticFieldLoader.java | 5 +++ ...ortedSetDocValuesSyntheticFieldLoader.java | 5 +++ .../index/mapper/SourceLoader.java | 12 +++++- .../mapper/StringStoredFieldFieldLoader.java | 5 +++ .../index/mapper/ObjectMapperTests.java | 40 +++++++++++++++++++ .../AggregateDoubleMetricFieldMapper.java | 5 +++ .../mapper/ConstantKeywordFieldMapper.java | 20 +++++----- 8 files changed, 93 insertions(+), 20 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java index 8c3c16f97b574..47afb8adb42bf 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java @@ -619,20 +619,21 @@ private ObjectDocValuesLoader(List loaders) { @Override public boolean advanceToDoc(int docId) throws IOException { + boolean anyLeafHasDocValues = false; for (SourceLoader.SyntheticFieldLoader.DocValuesLoader docValueLoader : loaders) { boolean leafHasValue = docValueLoader.advanceToDoc(docId); - hasValue |= leafHasValue; + anyLeafHasDocValues |= leafHasValue; } - /* - * Important and kind of sneaky note: this will return true - * if there were any values loaded from stored fields as - * well. That *is* how we "wake up" objects that contain just - * stored field. - */ - return hasValue; + hasValue |= anyLeafHasDocValues; + return anyLeafHasDocValues; } } + @Override + public boolean hasValue() { + return hasValue; + } + @Override public void write(XContentBuilder b) throws IOException { if (hasValue == false) { @@ -640,7 +641,9 @@ public void write(XContentBuilder b) throws IOException { } startSyntheticField(b); for (SourceLoader.SyntheticFieldLoader field : fields) { - field.write(b); + if (field.hasValue()) { + field.write(b); + } } b.endObject(); hasValue = false; diff --git a/server/src/main/java/org/elasticsearch/index/mapper/SortedNumericDocValuesSyntheticFieldLoader.java b/server/src/main/java/org/elasticsearch/index/mapper/SortedNumericDocValuesSyntheticFieldLoader.java index 0ad69a0498a6c..6c479ed94bc5d 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/SortedNumericDocValuesSyntheticFieldLoader.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/SortedNumericDocValuesSyntheticFieldLoader.java @@ -63,6 +63,11 @@ public DocValuesLoader docValuesLoader(LeafReader reader, int[] docIdsInLeaf) th return loader; } + @Override + public boolean hasValue() { + return values.count() > 0; + } + @Override public void write(XContentBuilder b) throws IOException { switch (values.count()) { diff --git a/server/src/main/java/org/elasticsearch/index/mapper/SortedSetDocValuesSyntheticFieldLoader.java b/server/src/main/java/org/elasticsearch/index/mapper/SortedSetDocValuesSyntheticFieldLoader.java index 321d1620c41b6..49c692dc73c75 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/SortedSetDocValuesSyntheticFieldLoader.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/SortedSetDocValuesSyntheticFieldLoader.java @@ -67,6 +67,11 @@ public DocValuesLoader docValuesLoader(LeafReader reader, int[] docIdsInLeaf) th return loader; } + @Override + public boolean hasValue() { + return values.count() > 0; + } + @Override public void write(XContentBuilder b) throws IOException { switch (values.count()) { diff --git a/server/src/main/java/org/elasticsearch/index/mapper/SourceLoader.java b/server/src/main/java/org/elasticsearch/index/mapper/SourceLoader.java index 83f60a7d7f35b..e7d4f54abd1bb 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/SourceLoader.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/SourceLoader.java @@ -126,9 +126,12 @@ public BytesReference source(FieldsVisitor fieldsVisitor, int docId) throws IOEx } } } + if (docValuesLoader != null) { + docValuesLoader.advanceToDoc(docId); + } // TODO accept a requested xcontent type try (XContentBuilder b = new XContentBuilder(JsonXContent.jsonXContent, new ByteArrayOutputStream())) { - if (docValuesLoader.advanceToDoc(docId)) { + if (loader.hasValue()) { loader.write(b); } else { b.startObject().endObject(); @@ -185,6 +188,11 @@ public DocValuesLoader docValuesLoader(LeafReader leafReader, int[] docIdsInLeaf return null; } + @Override + public boolean hasValue() { + return false; + } + @Override public void write(XContentBuilder b) {} }; @@ -202,6 +210,8 @@ public void write(XContentBuilder b) {} */ DocValuesLoader docValuesLoader(LeafReader leafReader, int[] docIdsInLeaf) throws IOException; + boolean hasValue(); + /** * Write values for this document. */ diff --git a/server/src/main/java/org/elasticsearch/index/mapper/StringStoredFieldFieldLoader.java b/server/src/main/java/org/elasticsearch/index/mapper/StringStoredFieldFieldLoader.java index 0cd65d0ee3059..2c74afce43321 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/StringStoredFieldFieldLoader.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/StringStoredFieldFieldLoader.java @@ -39,6 +39,11 @@ public void load(List values) { this.values = values; } + @Override + public boolean hasValue() { + return values != null && values.isEmpty() == false; + } + @Override public void write(XContentBuilder b) throws IOException { if (values == null || values.isEmpty()) { diff --git a/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperTests.java index 4a4bea2c02c3a..c0b61a21a0938 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperTests.java @@ -23,6 +23,7 @@ import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.instanceOf; +import static org.hamcrest.Matchers.nullValue; public class ObjectMapperTests extends MapperServiceTestCase { @@ -516,4 +517,43 @@ public void testSubobjectsCannotBeUpdatedOnRoot() throws IOException { ); assertEquals("the [subobjects] parameter can't be updated for the object mapping [_doc]", exception.getMessage()); } + + /** + * Makes sure that an empty object mapper returns {@code null} from + * {@link SourceLoader.SyntheticFieldLoader#docValuesLoader}. This + * is important because it allows us to skip whole chains of empty + * fields when loading synthetic {@code _source}. + */ + public void testSyntheticSourceDocValuesEmpty() throws IOException { + DocumentMapper mapper = createDocumentMapper(mapping(b -> b.startObject("o").field("type", "object").endObject())); + ObjectMapper o = (ObjectMapper) mapper.mapping().getRoot().getMapper("o"); + assertThat(o.syntheticFieldLoader().docValuesLoader(null, null), nullValue()); + assertThat(mapper.mapping().getRoot().syntheticFieldLoader().docValuesLoader(null, null), nullValue()); + } + + /** + * Makes sure that an object mapper containing only fields without + * doc values returns {@code null} from + * {@link SourceLoader.SyntheticFieldLoader#docValuesLoader}. This + * is important because it allows us to skip whole chains of empty + * fields when loading synthetic {@code _source}. + */ + public void testSyntheticSourceDocValuesFieldWithout() throws IOException { + DocumentMapper mapper = createDocumentMapper(mapping(b -> { + b.startObject("o").startObject("properties"); + { + b.startObject("kwd"); + { + b.field("type", "keyword"); + b.field("doc_values", false); + b.field("store", true); + } + b.endObject(); + } + b.endObject().endObject(); + })); + ObjectMapper o = (ObjectMapper) mapper.mapping().getRoot().getMapper("o"); + assertThat(o.syntheticFieldLoader().docValuesLoader(null, null), nullValue()); + assertThat(mapper.mapping().getRoot().syntheticFieldLoader().docValuesLoader(null, null), nullValue()); + } } diff --git a/x-pack/plugin/mapper-aggregate-metric/src/main/java/org/elasticsearch/xpack/aggregatemetric/mapper/AggregateDoubleMetricFieldMapper.java b/x-pack/plugin/mapper-aggregate-metric/src/main/java/org/elasticsearch/xpack/aggregatemetric/mapper/AggregateDoubleMetricFieldMapper.java index bd17327ce36f4..576045db004c6 100644 --- a/x-pack/plugin/mapper-aggregate-metric/src/main/java/org/elasticsearch/xpack/aggregatemetric/mapper/AggregateDoubleMetricFieldMapper.java +++ b/x-pack/plugin/mapper-aggregate-metric/src/main/java/org/elasticsearch/xpack/aggregatemetric/mapper/AggregateDoubleMetricFieldMapper.java @@ -725,6 +725,11 @@ public DocValuesLoader docValuesLoader(LeafReader reader, int[] docIdsInLeaf) th return new AggregateDocValuesLoader(); } + @Override + public boolean hasValue() { + return metricHasValue.isEmpty() == false; + } + @Override public void write(XContentBuilder b) throws IOException { if (metricHasValue.isEmpty()) { diff --git a/x-pack/plugin/mapper-constant-keyword/src/main/java/org/elasticsearch/xpack/constantkeyword/mapper/ConstantKeywordFieldMapper.java b/x-pack/plugin/mapper-constant-keyword/src/main/java/org/elasticsearch/xpack/constantkeyword/mapper/ConstantKeywordFieldMapper.java index a070b828aa32b..257fe97de34ca 100644 --- a/x-pack/plugin/mapper-constant-keyword/src/main/java/org/elasticsearch/xpack/constantkeyword/mapper/ConstantKeywordFieldMapper.java +++ b/x-pack/plugin/mapper-constant-keyword/src/main/java/org/elasticsearch/xpack/constantkeyword/mapper/ConstantKeywordFieldMapper.java @@ -311,6 +311,11 @@ protected String contentType() { @Override public SourceLoader.SyntheticFieldLoader syntheticFieldLoader() { + String value = fieldType().value(); + ; + if (value == null) { + return SourceLoader.SyntheticFieldLoader.NOTHING; + } return new SourceLoader.SyntheticFieldLoader() { @Override public Stream> storedFieldLoaders() { @@ -319,19 +324,14 @@ public Stream> storedFieldLoaders() { @Override public DocValuesLoader docValuesLoader(LeafReader reader, int[] docIdsInLeaf) { - /* - * If there is a value we need to enable objects containing these - * fields. We could build something special for fields that are - * always "on", but constant_keyword fields are rare enough that - * having an extra doc values loader that always returns `true` - * isn't a big performance hit and gets the job done. - */ - if (fieldType().value == null) { - return null; - } return docId -> true; } + @Override + public boolean hasValue() { + return true; + } + @Override public void write(XContentBuilder b) throws IOException { if (fieldType().value != null) {