Migrate Cargo's &Option<T> into Option<&T>

[nyurik]

As part of this, also modify Option<&String> to Option<&str>, same for PathBuf

Trying out my new lint rust-lang/rust-clippy#13336 - according to the video, this could lead to some performance and memory optimizations.

Basic thoughts expressed in the video that seem to make sense:

• &Option<T> in an API breaks encapsulation:
  • caller must own T and move it into an Option to call with it
  • if returned, the owner must store it as Option internally in order to return it
• Performance is subject to compiler optimization, but at the basics, &Option<T> points to memory that has presence flag + value, whereas Option<&T> by specification is always optimized to a single pointer.

[rustbot]

r? @weihanglo

rustbot has assigned @weihanglo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[nyurik]

note that in this change the fn signature is changed to take the ownership - seems to be ok, but maybe less efficient?

[nyurik]

this seems wrong -- if both v and self are owned, why can't I take ownership of v.inner.precise ?

[epage]

Note that we intentionally run with very few clippy lints enabled and there are times we intentionally do what this PR changes away from because it actually makes the code better and a change like this is unlikely to have a significant impact on performance.

This might make some parts nicer but having to make the connection between one API change and how it changes callers is hard when everything is bundled in one PR.

[nyurik]

@epage do you disagree with the arguments presented in the video? Or do you propose to simply break this PR into multiple ones to make reviewing easier? If so, could you give some guidance on the partitioning. Thx!

[epage]

The video is about API design and micro-optimizations without data to back it.

Those are good baseline principles to operate on but they hardly map to every case in reality. In particular, cargo is not developed as a library, but as a binary that we don't block people from accessing the internals. So API design principles are thrown out for that package and what matters is what helps with the implementation of Cargo. Take https://github.com/rust-lang/cargo/blob/master/src/cargo/util/toml/targets.rs, that commits various "sins" around references, both &Option<T> and Option<&Vec<T>> rather than Option<&[T]>. Thats because that is whats easiest for the caller. And of all of the performance hits we take in manifest parsing, no one will notice the difference.

Particularly when you get into

modify Option<&String> to Option<&str>, same for PathBuf

you hit limits. Yes, you can change &String to &str and &Vec<T> to &[T] to reduce ownership restrictions but then what happens when you have &Vec<String>? The only way to remove restrictions is to use impl Iterator<Item=&str> and thats not always convenient.

I'm not against people generally improving this as they make changes (putting it into its own commit of course) or having tools call out it for the dev to reconsider (which is why I think we need a new lint level), but I don't want to dig into a 33 file PR to see if there are places this made it worse.

[weihanglo]

Note that we intentionally run with very few clippy lints enabled and there are times we intentionally do what this PR changes away from because it actually makes the code better and a change like this is unlikely to have a significant impact on performance.

To expand this a bit, without enabling the lint in Cargo's [lints] table, we may need to do it again in the future. I would suggest at least not to do it at this moment, and revisit when the lint is in clippy officially.

[weihanglo]

Trying out my new lint rust-lang/rust-clippy#13336 - according to the video, this could lead to some performance and memory optimizations.

Does that mean you're looking for crate maintainers' feekback for the lint, or you just want to understand how the new lint impacts a medium-size codecase?

[nyurik]

thx for feedback @weihanglo and @epage! The lint is part of pedantic, and I do want to see how it impacts the codebases as well as get maintainers' perspectives. Ed does bring valid point that this is more important to a crate's external API rather than internal function signatures, and performance benefits are yet to be determined. The video's author made a lot of sense in their logic, but in this PR i did end up adding a lot of .as_ref() and .as_deref() instead of &, but at the same time a few of them were removed on the caller's side, plus using None instead of &None did look cleaner. I think this will be important for the crate authors in building the right APIs, but internal usefulness and perf is TBD.

[nyurik]

Micro-benchmarking is notoriously hard to do right. I tried running this benchmark (assuming I got it done right), it shows about 11% difference. If I change Option<Vec<usize>> to Option<(usize, usize)> and add two numbers, the difference grows to about 20% (1.4ns vs 1.12ns on my laptop).

use criterion::{criterion_group, criterion_main, Criterion};

#[inline(never)]
pub fn do_ref(val: &Option<Vec<usize>>) -> usize {
    match val {
        None => 42,
        Some(val) => val.iter().sum(),
    }
}

#[inline(never)]
pub fn do_as_ref(val: Option<&Vec<usize>>) -> usize {
    match val {
        None => 42,
        Some(val) => val.iter().sum(),
    }
}

fn criterion_benchmark(c: &mut Criterion) {
    let data: Option<Vec<usize>> = Some(vec![0; 2]);
    c.bench_function("ref", |b| b.iter(|| do_ref(&data)));
    c.bench_function("as_ref", |b| b.iter(|| do_as_ref(data.as_ref())));
}

criterion_group!(benches, criterion_benchmark);
criterion_main!(benches);

[epage]

To be clear, I'm less interested in micro-benchmarks because this kind of performance chance is unlikely to beat out the algorithmic complexity challenges in Cargo.

[bors]

☔ The latest upstream changes (presumably #14600) made this pull request unmergeable. Please resolve the merge conflicts.