view-types: store borrows of view types in the AST

[scrabsha]

View all comments

Tracking issue: #155938.

Instead of discarding view types, we store them in the AST now!

[JonathanBrouwer]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 9d3dea7 (9d3dea74651f758e35177070e510be8f3639a7fd, parent: 6769f690f947f12e36db6b6503bab265b7b2cced)

[rust-timer]

Finished benchmarking commit (9d3dea7): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

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

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.7%	[-1.4%, -0.3%]	6
Improvements ✅ (secondary)	-0.2%	[-0.2%, -0.1%]	2
All ❌✅ (primary)	-0.7%	[-1.4%, -0.3%]	6

Max RSS (memory usage)

Results (primary 0.1%, secondary -2.6%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	1.9%	[1.9%, 1.9%]	1
Regressions ❌ (secondary)	1.2%	[1.2%, 1.2%]	1
Improvements ✅ (primary)	-0.8%	[-0.8%, -0.7%]	2
Improvements ✅ (secondary)	-4.4%	[-7.3%, -1.5%]	2
All ❌✅ (primary)	0.1%	[-0.8%, 1.9%]	3

Cycles

Results (secondary 3.4%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	5.0%	[0.5%, 9.9%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.3%	[-1.3%, -1.3%]	1
All ❌✅ (primary)	-	-	0

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 503.61s -> 496.571s (-1.40%)
Artifact size: 394.46 MiB -> 394.47 MiB (0.00%)

[scrabsha]

^

• rebased to main to fix conflicts
• fixed view-types: store borrows of view types in the AST #156016 (comment)

r? nikomatsakis

[rustbot]

The Rustfmt subtree was changed

cc @rust-lang/rustfmt

The parser was modified, potentially altering the grammar of (stable) Rust
which would be a breaking change.

cc @fmease

Changes to the size of AST and/or HIR nodes.

cc @nnethercote

The Clippy subtree was changed

cc @rust-lang/clippy

Some changes occurred in compiler/rustc_builtin_macros/src/autodiff.rs

cc @ZuseZ4

[nnethercote]

This is very unfortunate, because Ty/TyKind are very common. It looks like the ViewKind::Partial variant is responsible. Boxing its fields would help, though might not be enough to get back to 64/40 bytes. A more awkward possibility would be to introduce RefPartial and PinnedRefPartial and box all the fields within. Presumably partial views will be much rarer than full views.

View changes since the review

[scrabsha]

That was my intuition too, but all the perf run seems to show is ~noise (on the green side).

I would like to avoid RefPartial and RefPinnedPartial because it would require duplicating a lot of patterns.

Unless you have a better idea, I will keep the data structures as is because "perf says it's ok", and add a comment saying we could box the content of ViewKind if it becomes perf-sensitive.

[scrabsha]

Something I thought about last night is: do we really need to keep the span field in ViewKind::Partial. I'll try removing it first.

[nnethercote]

I'm generally loathe to let the common struct kinds get bigger. Perhaps it's just superstition, but it feels like a recipe for slow perf degradation over time, with unpredictable memory/cache effects that CI runs might not catch, given that they're on a single machine.

[scrabsha]

I ended up removing the span field (it was never used anywhere else), which brings the size to 72/48.

As I wrote earlier today, I am a bit worried about creating RefPartial and RefPinnedPartial variants. Feel free to reopen this thread if you want me to go on that route.

[nnethercote]

I think it would be worth trying to see how painful it is, or if it inspires another approach to avoid the size growth.

[scrabsha]

This makes me think that the variant should be View, so we can parse T.{} as well. Might be worth it actually!