DRAFT: Consume fieldcaps node response while deserializing it #120010
+258
−124
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
The API implemented here could definitely use some extra love. But the idea seems to work quite well according to some quick testing and this approach can potentially save a massive amount of memory not just in field caps. Instead of deserializing a full list of per-index responses before getting to processing them, we should consume each response right after deserializing it without any intermediary list. This should be safe to do since we deserialize responses on the same thread that we consume the full response on today. Not only does this save a lot of heap by merging responses directly, it also might save quite a bit of runtime from better cache locality.
original-brownbear
added
WIP
:Search Foundations/Search
original-brownbear
added a commit
to original-brownbear/elasticsearch
that referenced
this pull request
Jan 11, 2025
This field is only used (by security) for requests, having it in responses is redundant. Also, we have a couple of responses that are singletons/quasi-enums where setting the value needlessly might introduce some strange contention even though it's a plain store. This isn't just a cosmetic change. It makes it clear at compile time that each response instance is exclusively defined by the bytes that it is read from. This makes it easier to reason about the validity of suggested optimizations like elastic#120010
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.
Just to illustrate the point of where we can save memory here, the concrete API could be made nicer but functionally this thing seems fine and comes with a measurable performance boost from some quick and dirty benchmarking.
The API implemented here could definitely use some extra love. But the idea seems to work quite well according to some quick testing and this approach can potentially save a massive amount of memory not just in field caps.
Instead of deserializing a full list of per-index responses before getting to processing them, we should consume each response right after deserializing it without any intermediary list. This should be safe to do since we deserialize responses on the same thread that we consume the full response on today.
Not only does this save a lot of heap by merging responses directly, it also might save quite a bit of runtime from better cache locality. Also, having a stateful deserializer allows for a couple helpful tricks in around deduplication across multiple responses for fan-out style APIs as well.