Skip to content

Use serde to encode/decode various TOML formats - #3864

Merged
djc merged 13 commits into
masterfrom
toml
Jun 9, 2024
Merged

Use serde to encode/decode various TOML formats#3864
djc merged 13 commits into
masterfrom
toml

Conversation

@djc

@djcdjc commented Jun 8, 2024

Copy link
Copy Markdown
Contributor

This saves a bunch of code (-190 lines) and is generally more idiomatic. It also saves about 8% of latency on proxy invocations, after (both best of 3 runs):

djc-2021 toml rustup $ RUSTUP_FORCE_ARG0=rustc hyperfine "./target/release/rustup-init --version"
Benchmark 1: ./target/release/rustup-init --version
Time (mean ± σ): 32.6 ms ± 0.6 ms [User: 23.3 ms, System: 5.5 ms]
Range (min … max): 31.4 ms … 34.6 ms 86 runs

before:

djc-2021 master rustup $ RUSTUP_FORCE_ARG0=rustc hyperfine "./target/release/rustup-init --version"
Benchmark 1: ./target/release/rustup-init --version
Time (mean ± σ): 35.3 ms ± 0.6 ms [User: 25.6 ms, System: 5.8 ms]
Range (min … max): 33.6 ms … 36.6 ms 79 runs

@djc
djc requested a review from rami3lJune 8, 2024 12:46
@djc
djcforce-pushed the toml branch 3 times, most recently from c1838e7 to 686defaCompareJune 8, 2024 13:12
Comment threadsrc/config.rs Outdated
})
}
_ => Err(RustupError::UnknownMetadataVersion(current_version).into()),
MetadataVersion::V12 => {

@rami3lrami3lJun 9, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

To me it's clear that the happy path in the original code is for MetadataVersion::default(), but now it's for MetadataVersion::V12. I know at the moment being it's all the same, but the meaning being conveyed seems to be a bit different, and that might cause problems when the default value changes 🤔

@djcdjcJun 9, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I've reverted the logic to something closer to what was originally there.

Comment threadsrc/settings.rs Outdated
Comment threadsrc/dist/config.rs Outdated
Comment threadsrc/dist/config.rs Outdated

@rami3lrami3l left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice improvements overall! Somehow I thought we were using serde for such things already 🤣

@djc
djc enabled auto-merge June 9, 2024 07:55
@djc
djc added this pull request to the merge queue Jun 9, 2024
Merged via the queue into master with commit 81a7164Jun 9, 2024
@djc
djc deleted the toml branch June 9, 2024 08:27
@rami3lrami3l added this to the 1.28.0 milestone Jun 9, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@djc@rami3l