First step optimizing tsdb doc values codec merging. #125403
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The doc values codec iterates a few times over the doc value instance that needs to be written to disk. In case when merging and index sorting is enabled, this is much more expensive, as each time the doc values instance is iterated an expensive doc id sorting is performed (in order to get the doc ids in order of index sorting). There are several reasons why the doc value instance is iterated multiple times: * To compute stats (num values, number of docs with value) required for writing values to disk. * To write bitset that indicate which documents have a value. (indexed disi, jump table) * To write the actual values to disk. * To write the addresses to disk (in case docs have multiple values) This applies for numeric doc values, but also for the ordinals of sorted (set) doc values. This PR addresses solving the first reason why doc value instance needs to be iterated. This is done only when in case of merging and when the segments to be merged with are also of type es87 doc values, codec version is the same and there are no deletes.
fixed sorted set dv added unit test with index sorting
martijnvg
force-pushed
the
mergeSortedNumericField_3
branch
from
52f3084 to
65d97e5
Compare
|
The attached micro benchmark to tests the tsdb doc value codec with force merge suggests the following: |
|
This PR adds a more code than I thought I needed, but the good thing is that the 'format 'code the writes to disk didn't need to be changed. |
|
Running elastic/logs track ( |
dnhatn
reviewed
Mar 25, 2025
| sumNumDocsWithField += entry.numDocsWithField; | ||
| } | ||
|
|
||
| // Documents marked as deleted should be rare. Maybe in the case of noop operation? |
There was a problem hiding this comment.
Should we check this before getting docValues?
| @Override | ||
| public void mergeNumericField(FieldInfo mergeFieldInfo, MergeState mergeState) throws IOException { | ||
| var result = compatibleWithOptimizedMerge(enableOptimizedMerge, mergeFieldInfo, mergeState, (docValuesProducer) -> { | ||
| var numeric = docValuesProducer.getNumeric(mergeFieldInfo); |
There was a problem hiding this comment.
Should we query the NumericEntry directly?
if (docValuesProducer instanceof ES87TSDBDocValuesProducer producer && producer.version == VERSION_CURRENT) {
var entry = producer.numerics.get(mergeFieldInfo.name);
return new DocValuesConsumerUtil.FieldEntry(entry.docsWithFieldOffset, entry.numValues, -1);
}
There was a problem hiding this comment.
This is what I had initially, however the type of producer isn't ES87TSDBDocValuesProducer, but is PerFieldDocValuesFormat.FieldsReader. So I ended up with the current workaround, not happy about it, but I don't see another way.
| }; | ||
| } | ||
|
|
||
| static NumericDocValues mergeNumericValues(List<NumericDocValuesSub> subs, boolean indexIsSorted) throws IOException { |
There was a problem hiding this comment.
Did we copy these from DocValuesConsumer? Is there any chance we can avoid copying this?
There was a problem hiding this comment.
Yes. I don't see another way here. All the logic that we need here is private to DocValuesConsumer only.
|
Running tsdb track without this change as baseline and with this change as contender: Unfortunately the improvement is less visible here. Looks like ~3% less time spent on merging. |
|
Thanks Nhat for the review! |
…ctly in compatibleWithOptimizedMerge(...) method.
martijnvg
added
auto-backport
|
Unfortunately it isn't possible to run the release tests due to: I did confirm that locally the unit tests pass with release build (meaning the feature flag is disabled): |
💔 Backport failed
You can use sqren/backport to manually backport by running |
martijnvg
added a commit
to martijnvg/elasticsearch
that referenced
this pull request
Apr 9, 2025
Build jump table (disi) when iterating over SortedNumericDocValues, instead of separately iterating over SortedNumericDocValues. In case when indexing sorting is active, this requires an additional merge sort. Follow up from elastic#125403
andreidan
pushed a commit
to andreidan/elasticsearch
that referenced
this pull request
Apr 9, 2025
…5933) The change contains the following changes: - The numDocsWithField field moved from SortedNumericEntry to NumericEntry. Making this statistic always available. - Store jump table after values in ES87TSDBDocValuesConsumer#writeField(...). Currently it is stored before storing values. This will allow us later to iterate over the SortedNumericDocValues once. When merging, this is expensive as a merge sort on the fly is being executed. This change will allow all the optimizations that are listed in elastic#125403
martijnvg
added a commit
to martijnvg/elasticsearch
that referenced
this pull request
Apr 15, 2025
Backporting elastic#125403 to the 8.x branch. The doc values codec iterates a few times over the doc value instance that needs to be written to disk. In case when merging and index sorting is enabled, this is much more expensive, as each time the doc values instance is iterated a merge sorting is performed (in order to get the doc ids of new segment in order of index sorting). There are several reasons why the doc value instance is iterated multiple times: * To compute stats (num values, number of docs with value) required for writing values to disk. * To write bitset that indicate which documents have a value. (indexed disi, jump table) * To write the actual values to disk. * To write the addresses to disk (in case docs have multiple values) This applies for numeric doc values, but also for the ordinals of sorted (set) doc values. This PR addresses solving the first reason why doc value instance needs to be iterated. This is done only when in case of merging and when the segments to be merged with are also of type es87 doc values, codec version is the same and there are no deletes. Note this optimized merged is behind a feature flag for now.
elasticsearchmachine
pushed a commit
that referenced
this pull request
Apr 17, 2025
* [8.x] First step optimizing tsdb doc values codec merging. Backporting #125403 to the 8.x branch. The doc values codec iterates a few times over the doc value instance that needs to be written to disk. In case when merging and index sorting is enabled, this is much more expensive, as each time the doc values instance is iterated a merge sorting is performed (in order to get the doc ids of new segment in order of index sorting). There are several reasons why the doc value instance is iterated multiple times: * To compute stats (num values, number of docs with value) required for writing values to disk. * To write bitset that indicate which documents have a value. (indexed disi, jump table) * To write the actual values to disk. * To write the addresses to disk (in case docs have multiple values) This applies for numeric doc values, but also for the ordinals of sorted (set) doc values. This PR addresses solving the first reason why doc value instance needs to be iterated. This is done only when in case of merging and when the segments to be merged with are also of type es87 doc values, codec version is the same and there are no deletes. Note this optimized merged is behind a feature flag for now. * fixed compile errors in benchmark * Fix DocValuesConsumerUtil (#126836) The compatibleWithOptimizedMerge() method doesn't handle codec readers that are wrapped by our source pruning filter codec reader. This change addresses that. Failing to detect this means that the optimized merge will not kick in.
martijnvg
added a commit
that referenced
this pull request
Apr 17, 2025
Build jump table (disi) while iterating over SortedNumericDocValues for encoding the values, instead of separately iterating over SortedNumericDocValues just to build the jump table. In case when indexing sorting is active, this requires an additional merge sort. Follow up from #125403
martijnvg
added a commit
to martijnvg/elasticsearch
that referenced
this pull request
Apr 17, 2025
Build jump table (disi) while iterating over SortedNumericDocValues for encoding the values, instead of separately iterating over SortedNumericDocValues just to build the jump table. In case when indexing sorting is active, this requires an additional merge sort. Follow up from elastic#125403
elasticsearchmachine
pushed a commit
that referenced
this pull request
Apr 17, 2025
Build jump table (disi) while iterating over SortedNumericDocValues for encoding the values, instead of separately iterating over SortedNumericDocValues just to build the jump table. In case when indexing sorting is active, this requires an additional merge sort. Follow up from #125403
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The doc values codec iterates a few times over the doc value instance that needs to be written to disk. In case when merging and index sorting is enabled, this is much more expensive, as each time the doc values instance is iterated a merge sorting is performed (in order to get the doc ids of new segment in order of index sorting).
There are several reasons why the doc value instance is iterated multiple times:
This applies for numeric doc values, but also for the ordinals of sorted (set) doc values.
This PR addresses solving the first reason why doc value instance needs to be iterated. This is done only when in case of merging and when the segments to be merged with are also of type es87 doc values, codec version is the same and there are no deletes. Note this optimized merged is behind a feature flag for now.