Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Speed up synthetic source #87882

Merged
merged 6 commits into from
Jun 22, 2022
Merged

Speed up synthetic source #87882

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
602f0d3
Speed up synthetic source
nik9000 Jun 21, 2022
cbf22e5
Big comment
nik9000 Jun 21, 2022
ca099ec
Changes with Alan
nik9000 Jun 21, 2022
9aebc5a
Rename method
nik9000 Jun 21, 2022
e772fbf
Fixup
nik9000 Jun 22, 2022
9762bd7
Format
nik9000 Jun 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 13 additions & 5 deletions server/src/main/java/org/elasticsearch/index/mapper/KeywordFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
import org.elasticsearch.common.lucene.Lucene;
import org.elasticsearch.common.lucene.search.AutomatonQueries;
import org.elasticsearch.common.unit.Fuzziness;
import org.elasticsearch.core.CheckedRunnable;
import org.elasticsearch.core.Nullable;
import org.elasticsearch.index.analysis.IndexAnalyzers;
import org.elasticsearch.index.analysis.NamedAnalyzer;
Expand Down Expand Up @@ -1078,21 +1079,28 @@ public BytesSyntheticFieldLoader(String name, String simpleName) {
@Override
public Leaf leaf(LeafReader reader) throws IOException {
SortedSetDocValues leaf = DocValues.getSortedSet(reader, name);
if (leaf.getValueCount() == 0) {
return SourceLoader.SyntheticFieldLoader.NOTHING.leaf(reader);
}
return new SourceLoader.SyntheticFieldLoader.Leaf() {
private boolean hasValue;

@Override
public void advanceToDoc(int docId) throws IOException {
hasValue = leaf.advanceExact(docId);
public boolean empty() {
return false;
}

@Override
public boolean hasValue() {
return hasValue;
public void advanceToDoc(int docId) throws IOException {
hasValue = leaf.advanceExact(docId);
}

@Override
public void load(XContentBuilder b) throws IOException {
public void load(XContentBuilder b, CheckedRunnable<IOException> before) throws IOException {
if (false == hasValue) {
return;
}
before.run();
long first = leaf.nextOrd();
long next = leaf.nextOrd();
if (next == SortedSetDocValues.NO_MORE_ORDS) {
Expand Down
33 changes: 27 additions & 6 deletions server/src/main/java/org/elasticsearch/index/mapper/NumberFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import org.apache.lucene.index.DocValues;
import org.apache.lucene.index.LeafReader;
import org.apache.lucene.index.LeafReaderContext;
import org.apache.lucene.index.NumericDocValues;
import org.apache.lucene.index.SortedNumericDocValues;
import org.apache.lucene.sandbox.document.HalfFloatPoint;
import org.apache.lucene.sandbox.search.IndexSortSortedNumericDocValuesRangeQuery;
Expand All @@ -32,6 +33,7 @@
import org.elasticsearch.common.settings.Setting;
import org.elasticsearch.common.settings.Setting.Property;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.core.CheckedRunnable;
import org.elasticsearch.index.fielddata.IndexFieldData;
import org.elasticsearch.index.fielddata.IndexNumericFieldData.NumericType;
import org.elasticsearch.index.fielddata.plain.SortedDoublesIndexFieldData;
Expand Down Expand Up @@ -1629,24 +1631,43 @@ protected NumericSyntheticFieldLoader(String name, String simpleName) {
this.simpleName = simpleName;
}

private SortedNumericDocValues dv(LeafReader reader) throws IOException {
SortedNumericDocValues dv = reader.getSortedNumericDocValues(name);
if (dv != null) {
return dv;
}
NumericDocValues single = reader.getNumericDocValues(name);
if (single != null) {
return DocValues.singleton(single);
}
return null;
}
nik9000 marked this conversation as resolved.
Show resolved Hide resolved

@Override
public Leaf leaf(LeafReader reader) throws IOException {
SortedNumericDocValues leaf = DocValues.getSortedNumeric(reader, name);
SortedNumericDocValues leaf = dv(reader);
if (leaf == null) {
return SourceLoader.SyntheticFieldLoader.NOTHING.leaf(reader);
}
return new SourceLoader.SyntheticFieldLoader.Leaf() {
private boolean hasValue;

@Override
public void advanceToDoc(int docId) throws IOException {
hasValue = leaf.advanceExact(docId);
public boolean empty() {
return false;
}

@Override
public boolean hasValue() {
return hasValue;
public void advanceToDoc(int docId) throws IOException {
hasValue = leaf.advanceExact(docId);
}

@Override
public void load(XContentBuilder b) throws IOException {
public void load(XContentBuilder b, CheckedRunnable<IOException> before) throws IOException {
if (false == hasValue) {
return;
}
before.run();
if (leaf.docValueCount() == 1) {
b.field(simpleName);
loadNextValue(b, leaf.nextValue());
Expand Down
60 changes: 33 additions & 27 deletions server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import org.elasticsearch.common.logging.DeprecationCategory;
import org.elasticsearch.common.logging.DeprecationLogger;
import org.elasticsearch.common.xcontent.support.XContentMapValues;
import org.elasticsearch.core.CheckedRunnable;
import org.elasticsearch.index.mapper.MapperService.MergeReason;
import org.elasticsearch.xcontent.ToXContent;
import org.elasticsearch.xcontent.XContentBuilder;
Expand Down Expand Up @@ -554,51 +555,56 @@ protected void doXContent(XContentBuilder builder, Params params) throws IOExcep

@Override
public SourceLoader.SyntheticFieldLoader syntheticFieldLoader() {
List<SourceLoader.SyntheticFieldLoader> fields = new ArrayList<>();
mappers.values().stream().sorted(Comparator.comparing(Mapper::name)).forEach(sub -> {
SourceLoader.SyntheticFieldLoader subLoader = sub.syntheticFieldLoader();
if (subLoader != null) {
fields.add(subLoader);
}
});
List<SourceLoader.SyntheticFieldLoader> fields = mappers.values()
.stream()
.sorted(Comparator.comparing(Mapper::name))
.map(Mapper::syntheticFieldLoader)
.filter(l -> l != null)
.toList();
return new SourceLoader.SyntheticFieldLoader() {
@Override
public Leaf leaf(LeafReader reader) throws IOException {
List<SourceLoader.SyntheticFieldLoader.Leaf> leaves = new ArrayList<>();
List<SourceLoader.SyntheticFieldLoader.Leaf> l = new ArrayList<>();
for (SourceLoader.SyntheticFieldLoader field : fields) {
leaves.add(field.leaf(reader));
Leaf leaf = field.leaf(reader);
if (false == leaf.empty()) {
l.add(leaf);
}
}
SourceLoader.SyntheticFieldLoader.Leaf[] leaves = l.toArray(SourceLoader.SyntheticFieldLoader.Leaf[]::new);
return new SourceLoader.SyntheticFieldLoader.Leaf() {
@Override
public void advanceToDoc(int docId) throws IOException {
for (SourceLoader.SyntheticFieldLoader.Leaf leaf : leaves) {
leaf.advanceToDoc(docId);
}
public boolean empty() {
return leaves.length == 0;
}

@Override
public boolean hasValue() {
public void advanceToDoc(int docId) throws IOException {
for (SourceLoader.SyntheticFieldLoader.Leaf leaf : leaves) {
if (leaf.hasValue()) {
return true;
}
leaf.advanceToDoc(docId);
}
return false;
}

@Override
public void load(XContentBuilder b) throws IOException {
boolean started = false;
for (SourceLoader.SyntheticFieldLoader.Leaf leaf : leaves) {
if (leaf.hasValue()) {
if (false == started) {
started = true;
startSyntheticField(b);
public void load(XContentBuilder b, CheckedRunnable<IOException> before) throws IOException {
class HasValue implements CheckedRunnable<IOException> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This took me a few passes to understand, can we call it something like StartObjectEmitter?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, dumb question - does this work if we change advanceToDoc() to return a boolean that says whether or not it is positioned? We have to call it on every subfield anyway, and then the object already knows whether or not it contains any values on the current doc before we get to load.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me have a look. I thought it wouldn't be man I would love to get rid of this thing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me have a look. I thought it wouldn't be man I would love to get rid of this thing.

I... can't.... So close....

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@romseygeek and I brainstormed and replaced the callbacks with a cached boolean. Simple enough.

boolean hasValue;

@Override
public void run() throws IOException {
if (hasValue) {
return;
}
leaf.load(b);
hasValue = true;
before.run();
startSyntheticField(b);
}
}
if (started) {
HasValue hasValue = new HasValue();
for (SourceLoader.SyntheticFieldLoader.Leaf leaf : leaves) {
leaf.load(b, hasValue);
}
if (hasValue.hasValue) {
b.endObject();
}
}
Expand Down
49 changes: 33 additions & 16 deletions server/src/main/java/org/elasticsearch/index/mapper/SourceLoader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

import org.apache.lucene.index.LeafReader;
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.core.CheckedRunnable;
import org.elasticsearch.index.fieldvisitor.FieldsVisitor;
import org.elasticsearch.xcontent.XContentBuilder;
import org.elasticsearch.xcontent.json.JsonXContent;
Expand Down Expand Up @@ -82,15 +83,34 @@ public boolean reordersFieldValues() {
@Override
public Leaf leaf(LeafReader reader) throws IOException {
SyntheticFieldLoader.Leaf leaf = loader.leaf(reader);
if (leaf.empty()) {
return new Leaf() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given this has no state I wonder if it's worth having it as a final instance on Leaf?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@Override
public BytesReference source(FieldsVisitor fieldsVisitor, int docId) throws IOException {
// TODO accept a requested xcontent type
try (XContentBuilder b = new XContentBuilder(JsonXContent.jsonXContent, new ByteArrayOutputStream())) {
return BytesReference.bytes(b.startObject().endObject());
}
}
};
}
return new Leaf() {
@Override
public BytesReference source(FieldsVisitor fieldsVisitor, int docId) throws IOException {
class HasValue implements CheckedRunnable<IOException> {
boolean hasValue;

@Override
public void run() throws IOException {
hasValue = true;
}
}
HasValue hasValue = new HasValue();
// TODO accept a requested xcontent type
try (XContentBuilder b = new XContentBuilder(JsonXContent.jsonXContent, new ByteArrayOutputStream())) {
leaf.advanceToDoc(docId);
if (leaf.hasValue()) {
leaf.load(b);
} else {
leaf.load(b, hasValue);
if (hasValue.hasValue == false) {
b.startObject().endObject();
}
return BytesReference.bytes(b);
Expand All @@ -104,20 +124,17 @@ public BytesReference source(FieldsVisitor fieldsVisitor, int docId) throws IOEx
* Load a field for {@link Synthetic}.
*/
interface SyntheticFieldLoader {
/**
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still worth having some javadoc on this I think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what I did there.

* Load no values.
*/
SyntheticFieldLoader NOTHING = r -> new Leaf() {
@Override
public void advanceToDoc(int docId) throws IOException {}
public boolean empty() {
return true;
}

@Override
public boolean hasValue() {
return false;
}
public void advanceToDoc(int docId) throws IOException {}

@Override
public void load(XContentBuilder b) throws IOException {}
public void load(XContentBuilder b, CheckedRunnable<IOException> before) throws IOException {}
};

/**
Expand All @@ -130,19 +147,19 @@ public void load(XContentBuilder b) throws IOException {}
*/
interface Leaf {
/**
* Position the loader at a document.
* Is this entirely empty?
*/
void advanceToDoc(int docId) throws IOException;
boolean empty();

/**
* Is there a value for this field in this document?
* Position the loader at a document.
*/
boolean hasValue();
void advanceToDoc(int docId) throws IOException;

/**
* Load values for this document.
*/
void load(XContentBuilder b) throws IOException;
void load(XContentBuilder b, CheckedRunnable<IOException> before) throws IOException;
}
}

Expand Down