Only run finalizers of accepted attributes

[JonathanBrouwer]

Locally this had insanely good perf, but lets see what reality thinks about this
r? @jdonszelmann

Attribute parsing consists of two stages:

• All attribute are "accepted" by one or more parsers, which means the unparsed attribute is parsed, information about it is stored in the attr parser struct
• After all attributes are parsed, we "finalize" all parsers, producing a single parsed attribute representation from the parser struct.

This two-stage process exist so multiple attributes can get merged into one parser representation. For example if you have two repr attributes #[repr(C)] #[repr(packed)] it will only produce one parsed Repr attribue.

The dumb thing we did was we would "finalize" all parsers, even the ones that never accepted an attribute. This PR only calls finalize on the parsers that accepted at least one attribute.

[rustbot]

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann

[JonathanBrouwer]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: cfe6720 (cfe6720f1acf68ce3af9bfc209d6f6813864cad2, parent: d940e56841ddcc05671ead99290e35ff2e98369f)

[rust-timer]

Finished benchmarking commit (cfe6720): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in 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.1%	[0.1%, 0.2%]	3
Improvements ✅ (primary)	-1.0%	[-3.2%, -0.1%]	187
Improvements ✅ (secondary)	-1.6%	[-10.1%, -0.1%]	118
All ❌✅ (primary)	-1.0%	[-3.2%, -0.1%]	187

Max RSS (memory usage)

Results (primary -0.1%, secondary -0.9%)

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

	mean	range	count
Regressions ❌ (primary)	2.4%	[2.4%, 2.4%]	1
Regressions ❌ (secondary)	2.5%	[1.3%, 3.8%]	3
Improvements ✅ (primary)	-1.4%	[-2.0%, -0.7%]	2
Improvements ✅ (secondary)	-3.5%	[-5.3%, -2.5%]	4
All ❌✅ (primary)	-0.1%	[-2.0%, 2.4%]	3

Cycles

Results (primary -3.0%, secondary -3.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)	3.8%	[2.4%, 5.0%]	4
Improvements ✅ (primary)	-3.0%	[-3.9%, -2.0%]	16
Improvements ✅ (secondary)	-5.1%	[-8.6%, -2.4%]	21
All ❌✅ (primary)	-3.0%	[-3.9%, -2.0%]	16

Binary size

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

Bootstrap: 473.058s -> 473.648s (0.12%)
Artifact size: 383.29 MiB -> 383.21 MiB (-0.02%)

[jdonszelmann]

Otherwise looks great. If you don't care about my nit, r=me

View changes since this review

[jdonszelmann]

Could be a cap of one when single is set I guess haha, or even a smallvec, idk if it makes a diff

[JonathanBrouwer]

I think attr.len() is low for almost all realistic scenarios, so checking if single would probably be more expensive than just allocating a few extra spaces sometimes.

I am planning to see if I can win some more performance in a followup PR, will experiment with this then :)

[JonathanBrouwer]

@bors r=jdonszelmann

[rust-bors]

📌 Commit 6dc27bf has been approved by jdonszelmann

It is now in the queue for this repository.

[rust-bors]

☀️ Test successful - CI
Approved by: jdonszelmann
Duration: 3h 18m 56s
Pushing d276646 to main...

[github-actions]

What is this?

This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing af6695c (parent) -> d276646 (this PR)

Test differences

Show 2 test diffs

2 doctest diffs were found. These are ignored, as they are noisy.

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard d276646872981067251b0fe70131561f4a4142d8 --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

1. dist-apple-various: 5111.0s -> 4394.1s (-14.0%)
2. i686-msvc-1: 9778.3s -> 11148.1s (+14.0%)
3. dist-i586-gnu-i586-i686-musl: 4896.2s -> 5441.3s (+11.1%)
4. dist-aarch64-msvc: 6672.9s -> 5934.5s (-11.1%)
5. aarch64-msvc-1: 7875.6s -> 7062.6s (-10.3%)
6. x86_64-gnu-llvm-20: 4333.3s -> 4747.6s (+9.6%)
7. x86_64-gnu-debug: 6774.6s -> 7399.4s (+9.2%)
8. x86_64-gnu-distcheck: 7501.1s -> 8181.5s (+9.1%)
9. aarch64-msvc-2: 5747.0s -> 6258.7s (+8.9%)
10. dist-x86_64-apple: 9179.0s -> 8428.0s (-8.2%)

How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

[rust-timer]

Finished benchmarking commit (d276646): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -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.0%	[0.0%, 0.0%]	1
Improvements ✅ (primary)	-1.0%	[-3.2%, -0.2%]	182
Improvements ✅ (secondary)	-1.7%	[-10.0%, -0.1%]	110
All ❌✅ (primary)	-1.0%	[-3.2%, -0.2%]	182

Max RSS (memory usage)

Results (primary -0.4%, secondary -0.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)	1.2%	[0.7%, 1.6%]	3
Improvements ✅ (primary)	-0.4%	[-0.4%, -0.4%]	1
Improvements ✅ (secondary)	-3.6%	[-4.5%, -2.7%]	2
All ❌✅ (primary)	-0.4%	[-0.4%, -0.4%]	1

Cycles

Results (primary -3.0%, secondary -5.0%)

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)	2.8%	[2.8%, 2.8%]	1
Improvements ✅ (primary)	-3.0%	[-4.3%, -2.0%]	24
Improvements ✅ (secondary)	-5.3%	[-8.5%, -2.2%]	31
All ❌✅ (primary)	-3.0%	[-4.3%, -2.0%]	24

Binary size

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

Bootstrap: 475.177s -> 472.604s (-0.54%)
Artifact size: 383.25 MiB -> 383.23 MiB (-0.01%)

[nnethercote]

Fantastic results! Do you know if this is gaining back performance that was gradually lost as attributes were ported to the new attr parsing system? Or does this represent a speed win over the old attr parsing system? Or something else?

[JonathanBrouwer]

(I have no data to back this up, mostly going on my intuition)
I'd guess this wins back most or all speed loss compared to the old attr parsing system. This PR fixes an oversight in the implementation of the new attribute parsers that was slowly eating more performance as more attributes were parsed.
Some attr ports had genuine perf wins tho, because the attribute was previously being parsed multiple times and now only once. The doc attribute had quite a nice perf win for example.
I have some more ideas on how to gain more perf, but prioritizing other work for now.

[nnethercote]

Thanks for the explanation. Sounds like the attr parser rewrite might end up being a small perf win overall on top of the large clarity/correctness win. Nice!