Use optimized text in match_only_text fields #129371
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
jordan-powers
added
>non-issue
auto-backport
|
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
jordan-powers
commented
Jun 12, 2025
| /** | ||
| * Reader that decodes UTF-8 formatted bytes into chars. | ||
| */ | ||
| public class UTF8DecodingReader extends Reader { |
There was a problem hiding this comment.
This is basically equivalent to:
final var reader = new InputStreamReader(new ByteArrayInputStream(utfBytes.bytes(), utfBytes.offset(), utfBytes.length()), StandardCharsets.UTF_8);But according to the microbenchmarks, using the InputStreamReader/ByteArrayInputStream is really slow, and was actually slower than the original string-based implementation.
There was a problem hiding this comment.
Here are the results from the microbenchmark:
Benchmark (nDocs) Mode Cnt Score Error Units
OptimizedTextBenchmark.indexDocuments (baseline) 1048576 avgt 5 581.242 ± 3.050 ms/op
OptimizedTextBenchmark.indexDocuments (UTF8DecodingReader) 1048576 avgt 5 544.477 ± 4.961 ms/op
OptimizedTextBenchmark.indexDocuments (InputStreamReader) 1048576 avgt 5 852.380 ± 6.238 ms/op
There was a problem hiding this comment.
Good catch. Yes, this implementation makes a lot of sense then. Did you also run this PR against elastic/logs (enterprise) benchmark?
There was a problem hiding this comment.
I haven't yet, but I plan to today
There was a problem hiding this comment.
Maybe make this a final class?
jordan-powers
changed the title
7 tasks
martijnvg
reviewed
Jun 13, 2025
| /** | ||
| * Reader that decodes UTF-8 formatted bytes into chars. | ||
| */ | ||
| public class UTF8DecodingReader extends Reader { |
There was a problem hiding this comment.
Good catch. Yes, this implementation makes a lot of sense then. Did you also run this PR against elastic/logs (enterprise) benchmark?
...per-extras/src/main/java/org/elasticsearch/index/mapper/extras/MatchOnlyTextFieldMapper.java
Show resolved
Hide resolved
...per-extras/src/main/java/org/elasticsearch/index/mapper/extras/SourceConfirmedTextQuery.java
Show resolved
Hide resolved
martijnvg
approved these changes
Jun 16, 2025
...per-extras/src/main/java/org/elasticsearch/index/mapper/extras/MatchOnlyTextFieldMapper.java
Show resolved
Hide resolved
| /** | ||
| * Reader that decodes UTF-8 formatted bytes into chars. | ||
| */ | ||
| public class UTF8DecodingReader extends Reader { |
There was a problem hiding this comment.
Maybe make this a final class?
|
Looks like serverless check failed, because of: Which killed the search node and many tests failed because of this. I don't think it is related to this change. |
|
Ok, I benchmarked this with the In light of that, I'm not sure if we want to continue forward with this change. |
Given that even median indexing throughput varies, I think that this is within noise. I don't think this change makes things slower. I suspect we don't see a gain here, given that that are many other moving parts here. The micro benchmark suggested a minor improvement. I think not having to create java strings during indexing for match only fields is a beneficial. Additionally block loaders for match only text fields can work with bytes refs instead of strings, which I think is beneficial as well.
So I think we can move forward with the change and merge it in. |
|
Ok, I'll merge it and keep an eye on the nightlies. We can always revert it if we see something we don't like. |
💚 Backport successful
|
elasticsearchmachine
pushed a commit
that referenced
this pull request
Jun 18, 2025
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.
Follow-up to #126492 to use the json parsing optimizations for
match_only_textfields.Relates to #129072.