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.
This took me a few passes to understand, can we call it something like StartObjectEmitter?
There was a problem hiding this comment.
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.
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.
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.
@romseygeek and I brainstormed and replaced the callbacks with a cached boolean. Simple enough.
|
@romseygeek this is ready for you again any time! |
romseygeek
left a comment
There was a problem hiding this comment.
Thanks, this looks much nicer! A couple of nits but LGTM otherwise, no need for another review.
| 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.
Given this has no state I wonder if it's worth having it as a final instance on Leaf?
| * Load a field for {@link Synthetic}. | ||
| */ | ||
| interface SyntheticFieldLoader { | ||
| /** |
There was a problem hiding this comment.
Still worth having some javadoc on this I think?
There was a problem hiding this comment.
Not sure what I did there.
|
|
||
| /** | ||
| * Load values for this document. | ||
| * Write values for this document. |
|
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.
hasValuewithadvanceToDocreturningbooleanandcache the result.
ArrayListof 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.