Shrink ast::Expr64

[nnethercote]

View all comments

From 72 bytes to 64 bytes, by boxing ForLoop, which is the biggest
variant.

There are now 11 ExprKind variants that are 31 bytes, so future
improvements here will be very difficult.

[nnethercote]

@bors try @rust-timer queue

[nnethercote]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 05fc14a (05fc14a7ef6f521e0ff88a111b396eb0d1729253)
Base parent: c397dae (c397dae808f70caebab1fc4e11b3edf7e59f58c7)

[nnethercote]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: fe91f04 (fe91f043227f275ce6e4ba832c64d25a38f779fa)
Base parent: be8e824 (be8e82435eb04fbe75ed5286b52735366e160bed)

[rust-timer]

Finished benchmarking commit (fe91f04): comparison URL.

Overall result: ❌✅ regressions and improvements - please read:

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.

Next, please: If you can, justify the regressions found in this try perf run in writing along with @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

@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.4%	[0.2%, 0.7%]	5
Regressions ❌ (secondary)	0.5%	[0.0%, 1.1%]	26
Improvements ✅ (primary)	-0.3%	[-0.3%, -0.3%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.3%	[-0.3%, 0.7%]	6

Max RSS (memory usage)

Results (primary 0.6%, secondary 1.2%)

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

	mean	range	count
Regressions ❌ (primary)	1.1%	[0.7%, 1.4%]	16
Regressions ❌ (secondary)	5.7%	[2.2%, 10.9%]	14
Improvements ✅ (primary)	-1.5%	[-3.3%, -0.4%]	3
Improvements ✅ (secondary)	-4.6%	[-8.4%, -0.5%]	11
All ❌✅ (primary)	0.6%	[-3.3%, 1.4%]	19

Cycles

Results (primary -2.0%, secondary -4.7%)

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)	6.2%	[3.7%, 8.8%]	2
Improvements ✅ (primary)	-2.0%	[-2.0%, -2.0%]	2
Improvements ✅ (secondary)	-6.3%	[-13.6%, -2.0%]	14
All ❌✅ (primary)	-2.0%	[-2.0%, -2.0%]	2

Binary size

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

Bootstrap: 488.088s -> 489.645s (0.32%)
Artifact size: 389.26 MiB -> 389.33 MiB (0.02%)

[nnethercote]

Perf results are positive but require some explanation. tuple-stress and deep-vector are the most affected, because they both involve zillions of expressions (literals). ucd and coercions and unicode-normalization are also expression-heavy.

• icounts: a bit worse, oddly enough.
• cycles: some huge improvements, -13% in the best case for tuple-stress.
• wall-time: some big improvements, -10% in the best case for tuple-stress.
• cache misses: some huge improvements, -26% in the best case for tuple-stress.

Not sure why the icounts are worse, but as usual, when wall-time (and to a lesser extent, cycles) gives a clear signal, it's more trustworthy than icounts. And conceptually it makes sense that shrinking an extremely hot type from 72 byte to 64 bytes (now fits in a cache line!) is good news, especially when it only requires a slight pessimization of the relatively rare ForLoop variant.

@rustbot label: +perf-regression-triaged

[nnethercote]

r? @folkertdev

[rustbot]

rustfmt is developed in its own repository. If possible, consider making this change to rust-lang/rustfmt instead.

cc @rust-lang/rustfmt

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

cc @fmease

clippy is developed in its own repository. If possible, consider making this change to rust-lang/rust-clippy instead.

cc @rust-lang/clippy

[rustbot]

r? @oli-obk

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

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler, parser
• compiler, parser expanded to 75 candidates
• Random selection from 14 candidates

[folkertdev]

idk if assigning a second reviewer was just a race condition? If you want, r=me or you can wait for Oli to also have a look.

View changes since this review