Reduce aggregation result footprint #5661
Conversation
| /// A cousin of [`quickwit_search::SearchResponseRest`] that implements [`Deserialize`] | ||
| /// | ||
| /// This version of the response is necessary because | ||
| /// `serde_json_borrow::OwnedValue` is not deserializeable. |
There was a problem hiding this comment.
ah we can ask @PSeitz to make it Deserializable.
There was a problem hiding this comment.
OwnedValue is a little bit special, because it needs to to take ownership of the original String
There was a problem hiding this comment.
couldn't it be possible to still make it deserializable by keeping a state in the deserializer, and building a (non-original) String with the concatenation of all of the Strings?
There was a problem hiding this comment.
Yes, that should be possible, but it would require to have a special deserialization logic. Currently it's just a thin wrapper
There was a problem hiding this comment.
Yes, I've looked into it and it seemed pretty hairy 😄
Given that we use deserialisation only in one place and that is in the client, I ended up giving up to the dumb solution of having this DTO style object.
|
TODO BEFORE MERGING: add the same thing on the ES API search path |
rdettai
force-pushed
the
agg-res-footprint
branch
2 times, most recently
from
622819b to
c5d7e86
Compare
|
@fulmicoton I made some changes to also use the same borrowed deserialization on the ES API code path. If you could take a look that would be great! |
rdettai
force-pushed
the
agg-res-footprint
branch
from
c5d7e86 to
7593098
Compare
Description
Profiling showed that parsing the json serialized aggregation string returned by the root searcher into a
serde_json::Valuecan take 5x time the memory size of the returned payload. On a loaded system, this can represent a lot of memory that is consumed even if results are served from cache.By using
serde_json_borrowed::OwnedValue, we can reduce this tp 3x the size of the returned payload.How was this PR tested?
Heaptrack profiling.