Open Ten0 opened 1 year ago
Hello @dtolnay,
If you validate that you would be open to such a change we would gladly provide the further analysis and the PR.
Thanks a have a nice day!
The main issue I see is with review capacity. There are already lots of open PRs, so even if the change is desirable and made, the PR will go unreviewed for many months.
Hello @oli-obk,
Thanks for the feedback!
Is there any thing we can do to shorten the review time, I have in mind:
Something else?
Thanks 🙏
That's unfortunately not the issue. The PRs we get are almost exclusively well documented and tested. We have 46 PRs open, and manage to merge around 1-2 PRs per month, while around 2-10 PRs are opened per month, so we just can't keep up at all.
Specifically, it appears to be possible to alter the
Deserialize
derive in a fully backwards-compatible manner with regards to the current behavior of#[serde(flatten)]
and leaving the existing trait system untouched, such that deserializing a struct that contains a singleflatten
ed field does not allocate a temporary storage for all fields that are unknown from the outer struct.This seems to have a typical performance impact of ~50% cut on deserialization.
This has an even more significant performance impact (for my simple benchmark I get an 86% cut) for use-cases where the program deserializing cares only about a subset of all the fields, as these are completely skipped with this approach, whereas they would be allocated and re-processed with the current approach.
This only works for structs that contain one flattened field (structs that have multiple
#[serde(flatten)]
fields would have to fallback to the current implementation).How it works
The general idea is that we can pass a wrapped
Deserializer
to the flattened struct, which will pass a wrappedMapAccess
to its visitor, which will only yield keys that don't match the outer struct. If they do match, the serializer will fill these fields in the outer struct instead.Because the original
MapAccess
has to be driven by the flattened field (becauseVisitor
s drive theMapAcces
es), it appears to not be possible to make this approach work when there are multiple flattened fields.Pros and cons
Pros
#[serde(flatten)]
uses. (On our pretty large Rust codebase, we have 108#[serde(flatten)]
, and 100% of the deserializations among those only contain a single flattened field, so that seems to represent the large majority of use-cases.)#[serde(flatten)]
typically now works in no-std environmentsCons
flatten
feature. Notably the code generated by#[derive(Deserialize)]
is significantly different because theMapAccess
has to be driven by the flattened field. Basically no complexity is removed, because support for multiple flattened fields would still depend on the current implementation.Should we finish the work?
This is clearly only at the POC state currently - notably enums are unsupported and some edge-cases may not be properly considered yet.
I'd like to know whether this looks like something that likely could be integrated into serde before I dedicate more time into making this closer to a PR-able state.