Remove HashStable impl for [hir::Attribute].

[nnethercote]

View all comments

This impl skips:

• All doc comments
• A handful of other attributes, mostly rustc_* ones related to incremental compilation testing.

This skipping originated in #36025 and was extended a couple of times, e.g. in #36370. Those PRs don't have any explanation of why the skipping exists. Perhaps the reasoning was that doc comments should only affect rustdoc and rustdoc doesn't use incremental compilation? But doc comments end up in metadata, and there is a query attrs_for_def that returns a &'tcx [hir::Attribute]. So skipping some attributes just seems plainly wrong.

This commit removes the impl, which means [hir::Attribute] hashing falls back to the default impl for [T]. This has no noticeable effect on the test suite. It does slightly hurt performance, because of the doc comments. This perf regression seems worth it for the correctness benefits.

[nnethercote]

I'm looking into this because this impl is blocking a larger clean-up of stable hashing that I am working on.

@bors try

[rust-bors]

☀️ Try build successful (CI)
Build commit: a656fa4 (a656fa4315473dea4478da10f2220537a4beddc2, parent: bcded331651b60a0383b3ff51db4f24c4495ac53)

[Kobzol]

Based on the attributes, it looks like they were used for some incremental tests.

[JonathanBrouwer]

@rust-timer queue

[JonathanBrouwer]

@rust-timer build a656fa4

[JonathanBrouwer]

r=me if:

• perf is not negative
• wait a few days for people to comment, in case I'm missing anything here

View changes since this review

[JonathanBrouwer]

In fact attr.name() is not implemented for parsed attributes, so this check is always false.
Since other than cfg_trace all the attributes are only used for incremental tests, and the incremental tests still pass, I don't think this check is useful

[JonathanBrouwer]

(attr.name() will be removed in the near future, as a follow-up of #154808)

[rust-timer]

Finished benchmarking commit (a656fa4): comparison URL.

Overall result: ❌ regressions - please read the text below

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.

Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @rustbot label: +perf-regression-triaged. If not, please fix the regressions and do another perf run. If its results are neutral or positive, the label will be automatically removed.

@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.1%, 1.1%]	41
Regressions ❌ (secondary)	0.5%	[0.1%, 1.5%]	17
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.4%	[0.1%, 1.1%]	41

Max RSS (memory usage)

Results (primary 1.9%, secondary 2.3%)

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)	2.3%	[2.0%, 3.3%]	5
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.9%	[1.9%, 1.9%]	1

Cycles

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

Binary size

Results (primary -0.0%, secondary -0.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)	-	-	0
Improvements ✅ (primary)	-0.0%	[-0.0%, -0.0%]	4
Improvements ✅ (secondary)	-0.0%	[-0.0%, -0.0%]	1
All ❌✅ (primary)	-0.0%	[-0.0%, -0.0%]	4

Bootstrap: 486.041s -> 492.508s (1.33%)
Artifact size: 395.29 MiB -> 395.20 MiB (-0.02%)

[nnethercote]

Perf regressions, boo. The code in question:

        let filtered: SmallVec<[&hir::Attribute; 8]> = self
            .iter()
            .filter(|attr| {
                attr.is_doc_comment().is_none()
                    // FIXME(jdonszelmann) have a better way to handle ignored attrs
                    && !attr.name().is_some_and(|ident| is_ignored_attr(ident))
            })
            .collect();

The is_ignored_attr part has no effect and can be ignored. It's the is_doc_comment().is_none() part that is important. We skip over doc comments when hashing attributes. I did some measurements and found that this can skip a lot of attributes. It accounts for the perf regression.

Is it even valid to skip doc comments? Maybe it's ok because (a) hashing is only needed for incremental compilation, (b) doc comments only affect rustdoc builds, and (c) rustdoc doesn't use incremental? Hmm, still feels dubious.

[bjorn3]

Doc comments do end up in the crate metadata, so just skipping them for incr comp is probably wrong.

[nnethercote]

There is also a query attrs_for_def that returns a &'tcx [hir::Attribute], so hashing only part of that also seems dubious.

I will update the description and then I think we should take the perf hit for improved correctness.

[nnethercote]

As per the PR description and discussion in the comments, the current behaviour of skipping doc comments (and certain other comments) is very likely wrong, and it's worth the small perf hit to correct it.

@rustbot label: +perf-regression-triaged

[rustbot]

⚠️ Warning ⚠️

• There are issue links (such as #123) in the commit messages of the following commits.
  Please move them to the PR description, to avoid spamming the issues with references to the commit, and so this bot can automatically canonicalize them to avoid issues with subtree.
  • cc74de3

[nnethercote]

@JonathanBrouwer I have updated the PR description and commit message with the new information. The conclusion being that the current behaviour is probably wrong, and it's worth taking the small perf hit to fix it.

[JonathanBrouwer]

Doc comments do end up in the crate metadata, so just skipping them for incr comp is probably wrong.

I guess the alternative to the current implementation would be removing doc comments from crate metadata.
Are doc comments actually needed in there? The answer is probably yes, but I would like to know for what.
If we know what, we can then also make a failing regression test

(And could you also move the issue numbers out of the commit descriptions to make rustbot happy?)

[JonathanBrouwer]

Started a PR here to see which tests fails, and what the perf effect would be: #155001

I just remembered they don't only end up in the crate metadata, but probably also in the incremental cache, so I think not hashing them is probably wrong either way... But that PR might mitigate the perf effect which is relatively small so we can take it, but not ideal

[JonathanBrouwer]

That idea from my PR seems to be hopeless for now, will experiment with that more when I have more time.

I would really like to see an incremental test in this PR tho, I've tried for a little bit to make one but I'm not managing to find any incremental test that fails. I don't understand why this test doesn't fail:

//@ check-pass
//@ revisions: cfail1 cfail2 cfail3 cfail4 cfail5 cfail6
//@ compile-flags: -Z query-dep-graph -O
//@ [cfail1]compile-flags: -Zincremental-ignore-spans
//@ [cfail2]compile-flags: -Zincremental-ignore-spans
//@ [cfail3]compile-flags: -Zincremental-ignore-spans
//@ ignore-backends: gcc

#![allow(warnings)]
#![feature(rustc_attrs)]
#![feature(linkage)]
#![feature(thread_local)]
#![crate_type="rlib"]


// Change static visibility
#[cfg(any(cfail1,cfail4))]
pub static ITEM: u8 = 0;

#[cfg(not(any(cfail1,cfail4)))]
#[rustc_clean(cfg="cfail2")]
#[rustc_clean(cfg="cfail3")]
#[rustc_clean(cfg="cfail5")]
#[rustc_clean(cfg="cfail6")]
/// This item has a doc-comment
pub static ITEM: u8 = 0;

View changes since this review

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[GuillaumeGomez]

I'm surprised there is no change in rmeta size. Sad. :-/

[bjorn3]

Are doc comments actually needed in there? The answer is probably yes, but I would like to know for what.

Yes, for #[doc(inline)] which can take docs from an already compiled dependency and show them inline. cargo doc uses regular check builds for dependencies, so check builds always need to contain doc comments in the crate metadata.

Edit: That was already said on your PR trying this.

[nnethercote]

@JonathanBrouwer: I see you've used tests/incremental/hashes/statics.rs as a starting point. That test shows that:

• (a) changing things like type, visibility, or mutability of a static changes its hash;
• (b) adding an attribute like #[thread_local] does not change the hash.

I'm not sure why, but changes in attributes aren't detected by this incremental testing. And this lack of detection cannot be due to the HashStable impl for [hir::Attribute] because that does not skip #[thread_local].

I looked at the history of the test. The attribute-changes-do-not-affect-hash behaviour was introduced in 6b5d2de, in the very large #79519, which has this description:

The objective is to reduce incr-comp invalidations due to modified attributes. Notably, those due to modified doc comments.

I don't see any further discussion explaining why this is legitimate.

Maybe we need @cjgillot's input here!