ESQL: Speed up reading many nulls #105088
Merged
Conversation
This file contains 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
Sometimes we produce output with a *ton* of `null`s and when we do a significant amount of time is spent on tracking the memory usage of each chunk of null colunms. Whole milliseconds in a request that takes dozens of milliseconds. We can avoid all of this by sharing all constant-null vectors produced for a single block. When running `FROM *` in ESQL on a data-less coordinating node this cuts reading reading pages from 15% of the time to 3.4% of the time, cutting the entire operation from 634ms to 599ms. Note that elastic#105067 is already open and should save about 540ms from the operation. It's unlikely that combining these two will yield a 59ms operation, but we live in hope. Relates to elastic#103369
|
Hi @nik9000, I've created a changelog YAML for you. |
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
elasticsearchmachine
added
the
Team:Analytics
2 tasks
dnhatn
approved these changes
Feb 3, 2024
| */ | ||
| Block readConstantNullBlock() throws IOException { | ||
| int positions = readVInt(); | ||
| if (lastConstantNullBlock == null) { |
There was a problem hiding this comment.
if (lastConstantNullBlock == null || lastConstantNullBlock.getPositionCount() != positions || lastConstantNullBlock.tryIncRef() == false) {
lastConstantNullBlock = blockFactory.newConstantNullBlock(positions);
}
return lastConstantNullBlock;If we don't retain the reference in this stream, and use tryIncRef(), then we could avoid managing the life cycle of this input stream. WDYT? However, I am okay with this approach too.
There was a problem hiding this comment.
Spent 20 minutes trying to decide if it was ok to have a reference that may be invalid. I hadn't noticed tryIncRef, but that does make sense. Let me think some more! I do feel like "this is a ref, let's count it". But it is a funny ref. And tryIncRef would do the right stuff.
There was a problem hiding this comment.
I think I'll keep it the way I have it. We can flip it later if we feel the need.
martijnvg
pushed a commit
to martijnvg/elasticsearch
that referenced
this pull request
Feb 12, 2024
Sometimes we produce output with a *ton* of `null`s and when we do a significant amount of time is spent on tracking the memory usage of each chunk of null colunms. Whole milliseconds in a request that takes dozens of milliseconds. We can avoid all of this by sharing all constant-null vectors produced for a single block. When running `FROM *` in ESQL on a data-less coordinating node this cuts reading reading pages from 15% of the time to 3.4% of the time, cutting the entire operation from 634ms to 599ms. Note that elastic#105067 is already open and should save about 540ms from the operation. It's unlikely that combining these two will yield a 59ms operation, but we live in hope. Relates to elastic#103369
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.
Sometimes we produce output with a ton of
nulls and when we do a significant amount of time is spent on tracking the memory usage of each chunk of null colunms. Whole milliseconds in a request that takes dozens of milliseconds. We can avoid all of this by sharing all constant-null vectors produced for a single block.When running
FROM *in ESQL on a data-less coordinating node this cuts reading reading pages from 15% of the time to 3.4% of the time, cutting the entire operation from 634ms to 599ms. Note that #105067 is already open and should save about 540ms from the operation. It's unlikely that combining these two will yield a 59ms operation, but we live in hope.Relates to #103369