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

[nyurik]

Similar to #130962 but for the compiler.

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? @petrochenkov

rustbot has assigned @petrochenkov.
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

[rustbot]

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

[workingjubilee]

@bors try @rust-timer queue

[bors]

⌛ Trying commit 340bd2a with merge aabe0bd...

[bors]

☀️ Try build successful - checks-actions
Build commit: aabe0bd (aabe0bddeb04c1b0dbe24d56a453156277fb8fee)

[rust-timer]

Finished benchmarking commit (aabe0bd): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (primary 2.4%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	2.4%	[2.4%, 2.4%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.4%	[2.4%, 2.4%]	1

Cycles

Results (secondary -2.7%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.7%	[-3.1%, -1.9%]	3
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 774.209s -> 771.637s (-0.33%)
Artifact size: 341.42 MiB -> 341.36 MiB (-0.02%)

[nyurik]

It seems some performance benchmarks are positive, while others are unchanged. While perf was not the primary reason for this PR, it could be a good bonus. 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)>, the difference grows to about 20%.

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);

[petrochenkov]

I don't think it always makes sense.

For example, in AST "qualified path" nodes are always stored as Option<P<ast::QSelf>>, so for code working with AST nodes (e.g. AST lowering) it's reasonable to take &Option<P<ast::QSelf>> to avoid many .as_ref()s.

I suggest the following policy: if the function is not public to other crates and all its calls require as_ref after the "&Option<T> -> Option<&T>" conversion, then keep &Option<T>.

@rustbot author

[nyurik]

I don't think it always makes sense.

For example, in AST "qualified path" nodes are always stored as Option<P<ast::QSelf>>, so for code working with AST nodes (e.g. AST lowering) it's reasonable to take &Option<P<ast::QSelf>> to avoid many .as_ref()s.

I suggest the following policy: if the function is not public to other crates and all its calls require as_ref after the "&Option<T> -> Option<&T>" conversion, then keep &Option<T>.

@rustbot author

Thanks for the feedback. My only concern with this is that we will miss some performance optimizations. The as_ref() are more verbose than & for sure, and I guess a more in-depth study of their effect is warranted, possibly even introducing a new as_ref() operator... @ or ~ ... (github has 1.5 million of them, but that requires waaaay more thinking). But it seems code style at the expense of ergonomics / performance... and yes, style is also part of ergonomics, making it even more complex.

[bors]

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

[Dylan-DPC]

@nyurik any updates on this? thanks

[nyurik]

@Dylan-DPC thx for the heads up. I updated the PR, but I think my comment is still relevant.

[bors]

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

[bors]

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