-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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.
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
Speed up synthetic source #87882
Conversation
This speeds up synthetic source, especially when there are many fields in the index that are declared in the mapping but don't have values. This is fairly common with ECS, and the tsdb rally track uses that. And this improves fetch performance of that track: ``` | 50th percentile service time | default | 6.24029 | 4.85568 | ms | -22.19% | | 90th percentile service time | default | 7.89923 | 6.52069 | ms | -17.45% | | 99th percentile service time | default | 12.0306 | 16.435 | ms | +36.61% | | 100th percentile service time | default | 14.2873 | 17.1175 | ms | +19.81% | | 50th percentile service time | default_1k | 158.425 | 25.3236 | ms | -84.02% | | 90th percentile service time | default_1k | 165.46 | 30.8655 | ms | -81.35% | | 99th percentile service time | default_1k | 168.954 | 33.3342 | ms | -80.27% | | 100th percentile service time | default_1k | 174.341 | 34.8344 | ms | -80.02% | ``` There's a slight increase in the 99th and 100th percentile service time for fetching ten document which think is unlucky jitter. Hopefully. The average performance of fetching ten docs improves anyway so I think we're ok. Fetching a thousand documents improves 80% across the board which is lovely. This works by doing three things: 1. Teach the "leaf" layer of source loader to detect when the field is empty in that segment and remove it from the synthesis process entirely. This brings most of the speed up in tsdb. 2. Replace `hasValue` with a callback when writing the first value. `hasValue` was resulting in a 2^n-like number of calls that really showed up in the profiler. 3. Replace the `ArrayList` of leaf loaders with an array. Before fixing the other two issues the `ArrayList`'s iterator really showed up in the profiling. Probably much less worth it now, but it's small. All of this brings synthetic source much closer to the fetch performance of standard _source: ``` | 50th percentile service time | default_1k | 11.4016 | 25.3236 | ms | +122.11% | | 90th percentile service time | default_1k | 13.7212 | 30.8655 | ms | +124.95% | | 99th percentile service time | default_1k | 15.8785 | 33.3342 | ms | +109.93% | | 100th percentile service time | default_1k | 16.9715 | 34.8344 | ms | +105.25% | ``` One important thing, these perf numbers come from fetching *hot* blocks on disk. They mostly compare CPU overhead and not disk overhead.
Pinging @elastic/es-analytics-geo (Team:Analytics) |
labelbot I have set a label |
server/src/main/java/org/elasticsearch/index/mapper/NumberFieldMapper.java
Outdated
Show resolved
Hide resolved
started = true; | ||
startSyntheticField(b); | ||
public void load(XContentBuilder b, CheckedRunnable<IOException> before) throws IOException { | ||
class HasValue implements CheckedRunnable<IOException> { |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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....
There was a problem hiding this comment.
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.
@romseygeek this is ready for you again any time! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this looks much nicer! A couple of nits but LGTM otherwise, no need for another review.
@@ -82,14 +82,24 @@ public boolean reordersFieldValues() { | |||
@Override | |||
public Leaf leaf(LeafReader reader) throws IOException { | |||
SyntheticFieldLoader.Leaf leaf = loader.leaf(reader); | |||
if (leaf.empty()) { | |||
return new Leaf() { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -104,20 +114,19 @@ public BytesReference source(FieldsVisitor fieldsVisitor, int docId) throws IOEx | |||
* Load a field for {@link Synthetic}. | |||
*/ | |||
interface SyntheticFieldLoader { | |||
/** |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 values for this document. | ||
* Write values for this document. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
Pinging @elastic/es-search (Team:Search) |
This speeds up synthetic source, especially when there are many fields
in the index that are declared in the mapping but don't have values.
This is fairly common with ECS, and the tsdb rally track uses that. And
this improves fetch performance of that track:
There's a slight increase in the 99th and 100th percentile service time
for fetching ten document which think is unlucky jitter. Hopefully. The
average performance of fetching ten docs improves anyway so I think
we're ok. Fetching a thousand documents improves 80% across the board
which is lovely.
This works by doing three things:
empty in that segment and remove it from the synthesis process
entirely. This brings most of the speed up in tsdb.
hasValue
withadvanceToDoc
returningboolean
andcache the result.
ArrayList
of leaf loaders with an array. Before fixingthe other two issues the
ArrayList
's iterator really showed up inthe profiling. Probably much less worth it now, but it's small.
All of this brings synthetic source much closer to the fetch performance
of standard _source:
One important thing, these perf numbers come from fetching hot blocks
on disk. They mostly compare CPU overhead and not disk overhead.