Computing crate_hash from metadata encoding instead of HIR (implements #94878) by Daniel-B-Smith · Pull Request #154724 · rust-lang/rust

Daniel-B-Smith · 2026-04-02T18:15:01Z

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.

This PR currently forks FileEncoder instead of changing the API of a widely used type. It would not be hard to add a FnMut strategy on flush instead of the fork. Since that would be an unpleasant merge conflict risk, I would request help to get it in quickly. If we stick with the fork, I'll make sure to port the tests. The first commit is solely the file copy, so that the diff is clean in the second commit.

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

nnethercote · 2026-04-09T23:26:03Z

@bors try @rust-timer queue

mejrs · 2026-05-06T21:20:44Z

@bors try @rust-timer queue

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

rust-bors · 2026-05-06T23:29:50Z

☀️ Try build successful (CI)
Build commit: d83f021 (d83f0213d3b2172cfc04784f39a42736048ff7fd, parent: 365c0e1d7a614ca94cb48431dcd2bc6d3b645db1)

rust-timer · 2026-05-07T00:10:27Z

Finished benchmarking commit (d83f021): 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.3%	[0.2%, 0.6%]	7
Regressions ❌ (secondary)	0.1%	[0.1%, 0.1%]	6
Improvements ✅ (primary)	-0.5%	[-5.5%, -0.1%]	82
Improvements ✅ (secondary)	-1.4%	[-5.1%, -0.1%]	47
All ❌✅ (primary)	-0.4%	[-5.5%, 0.6%]	89

Max RSS (memory usage)

Results (primary -0.5%, 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)	1.8%	[1.7%, 1.8%]	2
Regressions ❌ (secondary)	0.9%	[0.4%, 2.1%]	6
Improvements ✅ (primary)	-2.0%	[-3.6%, -0.7%]	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.5%	[-3.6%, 1.8%]	5

Cycles

Results (primary -4.1%, secondary -2.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)	0.8%	[0.5%, 1.1%]	3
Improvements ✅ (primary)	-4.1%	[-7.8%, -2.1%]	3
Improvements ✅ (secondary)	-3.3%	[-5.3%, -0.7%]	17
All ❌✅ (primary)	-4.1%	[-7.8%, -2.1%]	3

Binary size

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

Bootstrap: 501.88s -> 497.481s (-0.88%)
Artifact size: 395.21 MiB -> 395.12 MiB (-0.02%)

rustbot · 2026-05-12T00:26:08Z

r? @mu001999

rustbot has assigned @mu001999.
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
compiler expanded to 73 candidates
Random selection from 16 candidates

mu001999 · 2026-05-12T00:38:08Z

r? @cjgillot

cjgillot · 2026-05-23T16:02:54Z

                extra_filename: tcx.sess.opts.cg.extra_filename.clone(),
                stable_crate_id: tcx.stable_crate_id(LOCAL_CRATE),
                required_panic_strategy: tcx.required_panic_strategy(LOCAL_CRATE),
                panic_in_drop_strategy: tcx.sess.opts.unstable_opts.panic_in_drop,
                edition: tcx.sess.edition(),
                has_global_allocator: tcx.has_global_allocator(LOCAL_CRATE),
                has_alloc_error_handler: tcx.has_alloc_error_handler(LOCAL_CRATE),
                has_panic_handler: tcx.has_panic_handler(LOCAL_CRATE),
                has_default_lib_allocator: find_attr!(attrs, DefaultLibAllocator),
                externally_implementable_items,
                proc_macro_data,
                debugger_visualizers,
                compiler_builtins: find_attr!(attrs, CompilerBuiltins),
                needs_allocator: find_attr!(attrs, NeedsAllocator),
                needs_panic_runtime: find_attr!(attrs, NeedsPanicRuntime),
                no_builtins: find_attr!(attrs, NoBuiltins),
                panic_runtime: find_attr!(attrs, PanicRuntime),
                profiler_runtime: find_attr!(attrs, ProfilerRuntime),
                symbol_mangling_version: tcx.sess.opts.get_symbol_mangling_version(),

                crate_deps,
                dylib_dependency_formats,
                lib_features,
                stability_implications,
                lang_items,
                diagnostic_items,
                lang_items_missing,
                stripped_cfg_items,
                native_libraries,
                foreign_modules,
                source_map,
                target_modifiers,
                denied_partial_mitigations,
                traits,
                impls,
                incoherent_impls,
                exportable_items,
                stable_order_of_exportable_impls,
                exported_non_generic_symbols,
                exported_generic_symbols,
                interpret_alloc_index,
                tables,
                syntax_contexts,
                expn_data,
                expn_hashes,
                def_path_hash_map,
                specialization_enabled_in: tcx.specialization_enabled_in(LOCAL_CRATE),


How do we include all these in the hash?

View changes since the review

I believe almost all of these fields are covered either directly (all of the Lazy* fields) or indirectly (via the HIR hash or the dep tracking hash). is_stub isn't covered, but that's not related to the contents of the crate.

It would be annoyingly verbose, but it's completely straightforward to just add hashes for all of these fields. That might allow for removing the dependency on the HIR hash and/or the dep tracking hash. The change will probably be a small performance hit either way.

cjgillot · 2026-05-23T16:10:23Z

            .encode_enabled_denied_partial_mitigations());

+        let hash = Svh::new(self.opaque.hash());
+        tcx.untracked().local_crate_hash.set(hash).expect("local_crate_hash set twice");


Can we make crate_hash feedable instead?

View changes since the review

We can make it feedable if we get rid of the crate_hash dependency in the !tcx.needs_metadata() case. See my response on map.rs for more detailed discussion of that.

cjgillot · 2026-05-23T16:12:50Z

                    name: tcx.crate_name(LOCAL_CRATE),
                    triple: tcx.sess.opts.target_triple.clone(),
-                    hash: tcx.crate_hash(LOCAL_CRATE),
+                    hash,


Is this the best position to put the hash? Should we put it close to the header, after the start position? This would let us finish encoding everything and make partial decoding easier.

View changes since the review

I don't have a strong opinion. Everything encoded is already in the hash as finish() is called immediately after this function:

rust/compiler/rustc_metadata/src/rmeta/encoder.rs

Line 2552 in 8c1d720

if let Err((path, err)) = ecx.opaque.finish() {

rust/compiler/rustc_metadata/src/rmeta/encoder.rs

Line 2490 in 8c1d720

let root = ecx.encode_crate_root();

The partial decoding win would be nice, but my preference would be to punt the metadata format change to a follow up PR regardless.

cjgillot · 2026-05-23T16:13:44Z

This probably deserves a better file name.

View changes since the review

How about file_encoder.rs?

I also want to flag that we could alternatively change the shared FileEncoder interface instead of forking.

cjgillot · 2026-05-23T16:14:04Z

@@ -1,5 +1,6 @@
 // tidy-alphabetical-start
 #![allow(internal_features)]
+#![feature(core_intrinsics)]


Where?

View changes since the review

opaque.rs line 164

cjgillot · 2026-05-23T16:14:51Z

+            .local_crate_hash
+            .get()
+            .expect("crate_hash(LOCAL_CRATE) called before metadata encoding")
+    } else {


When does this happen?

View changes since the review

It's needed in the case where incremental compilation is enabled but metadata is not (

rust/compiler/rustc_middle/src/ty/context.rs

Line 1117 in b5e038d

CrateType::Executable

). It's used in naming the session directory name when finalizing the directory (

rust/compiler/rustc_incremental/src/persist/fs.rs

Line 307 in b5e038d

pub fn finalize_session_directory(sess: &Session, svh: Option<Svh>) {

set here:

rust/compiler/rustc_interface/src/queries.rs

Line 39 in 8c1d720

crate_hash: if tcx.sess.opts.incremental.is_some() {

). Incremental compilation still uses it to identify the local crate cache even though metadata isn't generated.

I tried forcing metadata generation whenever incremental compilation is enabled. It was both a large performance regression and exposed some edge cases where the library could be emitted but metadata encoding was not able to succeed.

cjgillot · 2026-05-23T16:15:21Z

@@ -1122,78 +1122,30 @@ impl<'tcx> pprust_hir::PpAnn for TyCtxt<'tcx> {
 }

 pub(super) fn crate_hash(tcx: TyCtxt<'_>, _: LocalCrate) -> Svh {


When is this implementation called? Could we get rid of it?

View changes since the review

This is the implementation of the crate_hash query which is used for the stub metadata header and for finalizing a session (

rust/compiler/rustc_interface/src/queries.rs

Line 39 in 8c1d720

crate_hash: if tcx.sess.opts.incremental.is_some() {

).

petrochenkov · 2026-05-26T10:08:36Z

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 · 2026-05-26T10:11:54Z

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

susitsm · 2026-05-26T11:41:10Z

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

rustbot · 2026-05-28T15:43:37Z

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.

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 2, 2026