Merge CrateDisambiguator into StableCrateId #85804

bjorn3 · 2021-05-29T10:12:00Z

This simplifies the code and potentially improves performance by reducing the amount of hashed data.

Fixes #85795

rust-highfive · 2021-05-29T10:12:03Z

r? @matthewjasper

(rust-highfive has picked a reviewer for you, use r? to override)

bjorn3 · 2021-05-29T12:17:07Z

@bors try @rust-timer queue

rust-timer · 2021-05-29T12:17:08Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2021-05-29T12:17:16Z

⌛ Trying commit 195e4a24be951480d9d27bd44a0f30771e374ae3 with merge 621a481ceedc7341caa1c2cdb9f083e0a2d47de4...

bors · 2021-05-29T13:06:54Z

☀️ Try build successful - checks-actions
Build commit: 621a481ceedc7341caa1c2cdb9f083e0a2d47de4 (621a481ceedc7341caa1c2cdb9f083e0a2d47de4)

rust-timer · 2021-05-29T13:06:55Z

Queued 621a481ceedc7341caa1c2cdb9f083e0a2d47de4 with parent f77b1a5, future comparison URL.

rust-timer · 2021-05-29T15:08:58Z

Finished benchmarking try commit (621a481ceedc7341caa1c2cdb9f083e0a2d47de4): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

bjorn3 · 2021-05-29T15:40:01Z

Overall this seems to be a slight perf improvement. I would guess that the regressions are caused by the fact that the StableCrateId of crates has changed and not by anything else.

bors · 2021-05-29T22:27:07Z

☔ The latest upstream changes (presumably #85698) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2021-05-30T14:48:31Z

⌛ Testing commit e0e0cfa with merge 5957990...

bors · 2021-05-30T17:12:05Z

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 5957990 to master...

ehuss · 2021-05-30T18:43:10Z

Would you mind updating the rustc-dev-guide to accommodate this change? https://rustc-dev-guide.rust-lang.org/backend/libs-and-metadata.html#crate-disambiguator

Also, this seems to drop the hash size from 128 bits to 64 bits, but I don't see any discussion here if that might cause any problems?

bjorn3 · 2021-05-30T18:55:59Z

Would you mind updating the rustc-dev-guide to accommodate this change? https://rustc-dev-guide.rust-lang.org/backend/libs-and-metadata.html#crate-disambiguator

Sure

Edit: rust-lang/rustc-dev-guide#1135

Also, this seems to drop the hash size from 128 bits to 64 bits, but I don't see any discussion here if that might cause any problems?

It was already impossible to have two crates with different disambiguators but identical StableCrateId in the same crate graph. If anything this reduces the likelyhood of a hash conflict as now only one hash can conflict while previously two hashes could conflict. As for the possibility of two crates with identical name and StableCrateId, but previously distinct disambiguators silently getting collapsed, the SVH would be different in that case thus causing an error.

ehuss · 2021-05-30T22:33:26Z

Thanks I appreciate it!

Mark-Simulacrum · 2021-06-01T14:11:25Z

@bjorn3 It seems like this was a regression on landing - https://perf.rust-lang.org/compare.html?start=2023cc3aa1ea98530f3124ed07713e6f95fd26ab&end=59579907ab52ad2369735622185a26f158bf0f0f&stat=instructions%3Au . This seems held up by further perf runs over the course of the week, too, so likely not spurious.

I think we should measure a revert PR, at the very least; would you be up for posting that?

bjorn3 · 2021-06-01T14:42:13Z

It seems like this was a regression on landing

Strange. The previous perf run showed that it was an improvement.

I think we should measure a revert PR, at the very least; would you be up for posting that?

Done: #85891

bjorn3 · 2021-06-01T14:45:36Z

compiler/rustc_span/src/def_id.rs

 pub struct StableCrateId(u64);

 impl StableCrateId {
+    pub fn to_u64(self) -> u64 {


I forgot to mark this as #[inline]. Maybe that is responsible for the perf regression?

bjorn3 · 2021-06-01T14:47:01Z

compiler/rustc_session/src/session.rs

-    pub fn generate_proc_macro_decls_symbol(&self, disambiguator: CrateDisambiguator) -> String {
-        format!("__rustc_proc_macro_decls_{}__", disambiguator.to_fingerprint().to_hex())
+    pub fn generate_proc_macro_decls_symbol(&self, stable_crate_id: StableCrateId) -> String {
+        format!("__rustc_proc_macro_decls_{:08x}__", stable_crate_id.to_u64())


Using {:08x} instead of {:x} like to_hex() seems to have used may also be part of the problem.

bjorn3 · 2021-06-01T14:52:31Z

compiler/rustc_middle/src/ty/query/on_disk_cache.rs

-                    let crate_name = tcx.original_crate_name(cnum).to_string();
-                    let crate_disambiguator = tcx.crate_disambiguator(cnum);
-                    ((crate_name, crate_disambiguator), cnum)
+                    let stable_crate_id = tcx.def_path_hash(cnum.as_def_id()).stable_crate_id();


Avoiding the def_path_hash call here in favor of directly looking up the StableCrateId by CrateNum from the CStore may also help.

…or, r=Mark-Simulacrum Revert "Merge CrateDisambiguator into StableCrateId" This reverts rust-lang#85804

…mbiguator, r=michaelwoerister Reland "Merge CrateDisambiguator into StableCrateId" Reverts rust-lang#85891 as this revert of rust-lang#85804 made perf even worse. r? `@Mark-Simulacrum`

bjorn3 added C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 29, 2021

rust-highfive assigned matthewjasper May 29, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 29, 2021