twilight-rs / twilight

Powerful, flexible, and scalable ecosystem of Rust libraries for the Discord API.
https://discord.gg/twilight-rs
ISC License
643 stars 129 forks source link

Make `ListBody<T>` lazily deserializable #2310

Open vilgotf opened 5 months ago

vilgotf commented 5 months ago

Instead of Response::models being an async fn -> Result<Vec<T>, E> it should return a type implementing Stream<Item = Result<T, E>> and IntoFuture<Output = Result<Vec<T>, E>>. This allows for lazy deserializing the response and potentially skip the (often unused) Vec buffer.

Inspiration: https://github.com/abdolence/axum-streams-rs

Erk- commented 5 months ago

How would we even do lazy deserialzation? Or do you simply mean to defer the deserialzation to when it is actually looked at?

laralove143 commented 5 months ago

its possible in theory but i doubt serde supports it and im not sure the complexity this brings is worth the performance gain, in some cases it may even lead to worse performance when people repeatedly deserialize the stream's items

vilgotf commented 5 months ago

How would we even do lazy deserialzation? Or do you simply mean to defer the deserialzation to when it is actually looked at?

Hyper streams the response body by default so we just need two streaming adapters: &[u8] -> &str and &str -> T. For inspiration, tokio-websockets contains a streaming UTF-8 validator.

its possible in theory but i doubt serde supports it

Serde does not support streaming adapters but it's trivial to do so ourselves. We deserialize upon hitting a comma if our counter of braces and brackets are zero (i.e. the comma separates elements in the top level array).

and im not sure the complexity this brings is worth the performance gain, in some cases it may even lead to worse performance when people repeatedly deserialize the stream's items

The stream (AsyncIterator) would consume the response body

Erk- commented 5 months ago

Would this not cause potentially more memory to be used since the string has to valid for as long as the deserilizer is?