Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Eliminate closures capturing needlessly many type parameters #2821

Merged
merged 1 commit into from
Sep 6, 2024

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented Sep 6, 2024

Closures capture every type parameter that is in scope, even ones which cannot make a difference to the behavior of the closure. This was previously causing a whole new MapDeserializer to be monomorphized for every distinct Visitor type that we deserialize from Content or &Content (i.e. internally tagged and untagged enums).

Tested by measuring the size of the rlib produced from the following small library. cargo clean && cargo build --release && ls -l target/release/librepro.rlib

use serde::Deserialize;

#[derive(Deserialize)]
#[serde(untagged)]
pub enum Untagged {
    A {},
    B {},
}

pub fn from_str(json: &str) -> serde_json::Result<Untagged> {
    serde_json::from_str(json)
}

Before this PR: 127K. After this PR: 111K. This is 12% smaller.

@dtolnay dtolnay merged commit c82f258 into serde-rs:master Sep 6, 2024
15 checks passed
@dtolnay dtolnay deleted the clos branch September 6, 2024 23:54
Comment on lines +1118 to +1124
fn content_deserializer_pair<'de, E>(
(k, v): (Content<'de>, Content<'de>),
) -> (ContentDeserializer<'de, E>, ContentDeserializer<'de, E>) {
(ContentDeserializer::new(k), ContentDeserializer::new(v))
}

let map = content.into_iter().map(content_deserializer_pair);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You also can just pass content.into_iter() because Content implements IntoDeserializer which is used in MapAccess implementation:

serde/serde/src/de/value.rs

Lines 1200 to 1207 in c82f258

impl<'de, I, E> de::MapAccess<'de> for MapDeserializer<'de, I, E>
where
I: Iterator,
I::Item: private::Pair,
First<I::Item>: IntoDeserializer<'de, E>,
Second<I::Item>: IntoDeserializer<'de, E>,
E: de::Error,
{

The same implicit conversion can be applied to use of SeqDeserializer and, if implement IntoDeserializer for &Content, for ContentRefDeserializer.

Maybe this also could reduce generated size?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants