Computing crate_hash from metadata encoding instead of HIR (implements #94878)

[Daniel-B-Smith]

View all comments

This PR converts the crate_hash/SVH to depend on metadata instead of HIR whenever metadata is needed. A trimmed down crate_hash is kept for dylib/binary cases where metadata is not present. It is believed that the metadata will be a more sound way to track dependency changes.

Related to #94878

The metadata hash is calculated on the raw bytes before flush because hashing the elements incrementally turned out to be too expensive.

The change to the HIR hash is potentially safe even without the metadata crate_hash change. Without that change, this PR is a performance regression. I am bundling them because I believe the HIR hash change is more likely to be safe in light of the HIR hash being less load bearing on the SVH.

The dylib/binary crate_hash removes components that I believe are not relevant to those cases. Analysis:

• resolutions.visibilities_for_hashing:

The comment says: "Hash visibility information since it does not appear in HIR." Visibilities only matter to external users of the crate (i.e. consumers of metadata). Without metadata, visibility differences are not observable, so this contributes nothing for incremental correctness.

• debugger_visualizers:

The comment says: "that content is exported into crate metadata, so any changes to it need to be reflected in the crate hash." Without metadata, there is nothing to export, so this is purely metadata-motivated. The visualizer file path is already covered by HIR (the attribute) and the content is loaded later only if metadata is being written.

• source_file_names:

These exist solely so that the crate-hash reflects remapped source paths that get embedded into metadata (the comment explicitly says "If we included the full mapping in the SVH, we could only have reproducible builds…"). The remapping itself is already captured via dep_tracking_hash, so for incremental this is redundant.

The crate hash calculation can be reverted via -Z metadata-crate-hash=no. I manually confirmed that setting that flag generates the same hash as the commit prior to my change. The test for that flag only checks that it changes the value of the hash and does not check the specific value of the hash. I could add a check for the latter, but I'm nervous that a hash golden test would be a maintenance headache.

DO NOT SUBMIT: Incremental compilation testing is ongoing and a way to revert the hash is necessary.

[nnethercote]

@bors try @rust-timer queue

[petrochenkov]

If this needs an extra pair of eyes, feel free to assign me after cjgillot reviews.
I'm generally interested in this topic and RDR, just don't have time for doing full initial reviews at the moment.

[petrochenkov]

Is there any ordering dependency between this PR and #155871?
Should this PR be merged first, or it doesn't matter?

[susitsm]

These are independent. The one getting in later will have to do a bit of merge conflict resolution, but nothing too bad.

[Kobzol]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: f0dc837 (f0dc837ab0fd17f0782df57d842c84dc7f57a4a0, parent: e7815e522ecc746592fee32f50478f521333b503)

[rust-timer]

Finished benchmarking commit (f0dc837): 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.4%, 0.4%]	1
Regressions ❌ (secondary)	0.1%	[0.1%, 0.1%]	5
Improvements ✅ (primary)	-0.4%	[-0.9%, -0.1%]	92
Improvements ✅ (secondary)	-1.3%	[-5.4%, -0.1%]	53
All ❌✅ (primary)	-0.4%	[-0.9%, 0.4%]	93

Max RSS (memory usage)

Results (primary 1.9%, secondary 0.3%)

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

	mean	range	count
Regressions ❌ (primary)	3.1%	[2.7%, 3.6%]	2
Regressions ❌ (secondary)	2.2%	[1.4%, 3.0%]	2
Improvements ✅ (primary)	-0.7%	[-0.7%, -0.7%]	1
Improvements ✅ (secondary)	-1.6%	[-1.7%, -1.4%]	2
All ❌✅ (primary)	1.9%	[-0.7%, 3.6%]	3

Cycles

Results (secondary -2.1%)

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.6%	[5.6%, 5.6%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.4%	[-3.9%, -2.7%]	6
All ❌✅ (primary)	-	-	0

Binary size

Results (primary 0.0%, secondary 0.1%)

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

	mean	range	count
Regressions ❌ (primary)	0.0%	[0.0%, 0.1%]	8
Regressions ❌ (secondary)	0.1%	[0.0%, 0.1%]	35
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.0%	[0.0%, 0.1%]	8

Bootstrap: 516.831s -> 540.55s (4.59%)
Artifact size: 400.72 MiB -> 398.64 MiB (-0.52%)

[Daniel-B-Smith]

The remaining big comment from the review is to decide on extending the existing FileEncoder interface to support hashing before flush or keeping the fork. After that, I can resolve the existing comments, add the disable flag and create a gist with the testing strategy I've been using with other crates.

[rustbot]

Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter
gets adapted for the changes, if necessary.

cc @rust-lang/miri, @RalfJung, @oli-obk, @lcnr

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

cc @fmease

HIR ty lowering was modified

cc @fmease

library/core/src/unicode/unicode_data.rs is generated by the src/tools/unicode-table-generator tool.

If you want to modify unicode_data.rs, please modify the tool then regenerate the library source file via ./x run src/tools/unicode-table-generator instead of editing unicode_data.rs manually.

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in src/tools/compiletest

cc @jieyouxu

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

cc @rust-lang/clippy

The reflection data structures are tied exactly to the implementation
in the compiler. Make sure to also adjust rustc_const_eval/src/const_eval/type_info.rs

cc @oli-obk

Some changes occurred in match checking

cc @Nadrieril

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[Daniel-B-Smith]

Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter gets adapted for the changes, if necessary.

cc @rust-lang/miri, @RalfJung, @oli-obk, @lcnr

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

cc @fmease

HIR ty lowering was modified

cc @fmease

library/core/src/unicode/unicode_data.rs is generated by the src/tools/unicode-table-generator tool.

If you want to modify unicode_data.rs, please modify the tool then regenerate the library source file via ./x run src/tools/unicode-table-generator instead of editing unicode_data.rs manually.

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in src/tools/compiletest

cc @jieyouxu

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

cc @rust-lang/clippy

The reflection data structures are tied exactly to the implementation in the compiler. Make sure to also adjust rustc_const_eval/src/const_eval/type_info.rs

cc @oli-obk

Some changes occurred in match checking

cc @Nadrieril

Apologies for the noise. There was a rebase snafu, so I accidentally a version that pushed included a bunch of changes from upstream. None of those files are in this PR.

[Daniel-B-Smith]

@cjgillot this should be ready for another round of review. I want to spend a little bit more time doing iterative incremental compilation on a couple of crates, but the revert flag should make the risk relatively low (/it might need more than 12 weeks of nightly before being merged into stable).