Only store StableCrateId once in DefPathTable. #119259

cjgillot · 2023-12-23T17:40:06Z

#119238 made me think of this.

rustbot · 2023-12-23T17:40:14Z

(rustbot has picked a reviewer for you, use r? to override)

Mark-Simulacrum · 2023-12-23T19:20:06Z

compiler/rustc_hir/src/definitions.rs

+            def_path_hash_to_index: Default::default(),
+        }
+    }
+
    fn allocate(&mut self, key: DefKey, def_path_hash: DefPathHash) -> DefIndex {


FWIW it looks like we're also hashing the parent's stable crate ID into the local part of this hash --

rust/compiler/rustc_hir/src/definitions.rs

Line 120 in 5eccfc3

parent.hash(&mut hasher);

It's probably pretty cheap (just an extra u64 hashed in) but seems like it would be correct to skip that hashing since we're not crossing crate boundaries.

This changes a lot of hash-dependent behaviour, so I'll investigate this in a follow-up PR.

Perf results don't look particularly meaningful either, so it might not really matter.

Mark-Simulacrum · 2023-12-23T19:20:13Z

r=me

cjgillot · 2023-12-24T01:08:20Z

@bors try @rust-timer queue

bors · 2023-12-24T01:09:29Z

⌛ Trying commit 62b611e with merge 53c94be...

Only store StableCrateId once in DefPathTable. rust-lang#119238 made me think of this. cc `@Mark-Simulacrum`

bors · 2023-12-24T02:35:52Z

☀️ Try build successful - checks-actions
Build commit: 53c94be (53c94be95e622cd89cc4f453f0c2d8db8349484e)

rust-timer · 2023-12-24T03:57:30Z

Finished benchmarking commit (53c94be): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.9%	[0.9%, 0.9%]	1
Regressions ❌ (secondary)	1.1%	[1.1%, 1.1%]	1
Improvements ✅ (primary)	-0.3%	[-1.1%, -0.2%]	17
Improvements ✅ (secondary)	-0.2%	[-0.4%, -0.2%]	14
All ❌✅ (primary)	-0.2%	[-1.1%, 0.9%]	18

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.4%	[3.4%, 3.4%]	1
Improvements ✅ (primary)	-1.7%	[-2.1%, -1.2%]	2
Improvements ✅ (secondary)	-2.8%	[-2.8%, -2.8%]	1
All ❌✅ (primary)	-1.7%	[-2.1%, -1.2%]	2

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.1%	[-1.1%, -1.1%]	1
Improvements ✅ (secondary)	-3.3%	[-4.5%, -2.1%]	6
All ❌✅ (primary)	-1.1%	[-1.1%, -1.1%]	1

Binary size

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

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

Bootstrap: 670.687s -> 670.155s (-0.08%)
Artifact size: 312.77 MiB -> 312.78 MiB (0.00%)

Mark-Simulacrum · 2023-12-29T12:55:18Z

@bors r+

bors · 2023-12-29T12:55:20Z

📌 Commit 821920b has been approved by Mark-Simulacrum

It is now in the queue for this repository.

bors · 2023-12-29T14:37:43Z

⌛ Testing commit 821920b with merge dc450f9...

bors · 2023-12-29T16:34:28Z

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing dc450f9 to master...

rust-timer · 2023-12-29T19:41:52Z

Finished benchmarking commit (dc450f9): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.8%	[0.8%, 0.8%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.3%	[-0.6%, -0.2%]	78
Improvements ✅ (secondary)	-0.4%	[-0.6%, -0.3%]	16
All ❌✅ (primary)	-0.3%	[-0.6%, 0.8%]	79

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.9%	[2.6%, 5.4%]	4
Improvements ✅ (primary)	-1.6%	[-2.5%, -0.5%]	18
Improvements ✅ (secondary)	-3.1%	[-6.4%, -1.0%]	8
All ❌✅ (primary)	-1.6%	[-2.5%, -0.5%]	18

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.5%	[0.5%, 0.5%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.9%	[-1.0%, -0.7%]	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.5%	[-1.0%, 0.5%]	4

Binary size

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.6%	[-5.1%, -0.1%]	114
Improvements ✅ (secondary)	-2.7%	[-6.9%, -0.0%]	76
All ❌✅ (primary)	-1.6%	[-5.1%, -0.1%]	114

Bootstrap: 670.236s -> 671.694s (0.22%)
Artifact size: 312.30 MiB -> 312.04 MiB (-0.08%)

rustbot assigned b-naber Dec 23, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 23, 2023

Mark-Simulacrum reviewed Dec 23, 2023

View reviewed changes

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 24, 2023

bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 24, 2023

Auto merge of rust-lang#119259 - cjgillot:single-crate-id, r=<try>

53c94be

Only store StableCrateId once in DefPathTable. rust-lang#119238 made me think of this. cc `@Mark-Simulacrum`

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Dec 24, 2023

Only store StableCrateId once in DefPathTable.

027fa9e

cjgillot force-pushed the single-crate-id branch from cbe64c2 to 179cf7d Compare December 24, 2023 12:54

This comment has been minimized.

Sign in to view

cjgillot added 2 commits December 24, 2023 17:22

Do not store stable_crate_id again in Definitions.

75ad81c

Do not store stable crate id in on-disk hash map.

821920b

cjgillot force-pushed the single-crate-id branch 2 times, most recently from c1fb22b to 821920b Compare December 24, 2023 17:23

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 29, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 29, 2023

bors merged commit dc450f9 into rust-lang:master Dec 29, 2023
12 checks passed

rustbot added this to the 1.77.0 milestone Dec 29, 2023

bors mentioned this pull request Dec 29, 2023

[do not merge] CI experiments #112049

Draft

cjgillot deleted the single-crate-id branch December 29, 2023 17:35

rustbot removed the perf-regression Performance regression. label Dec 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only store StableCrateId once in DefPathTable. #119259

Only store StableCrateId once in DefPathTable. #119259

cjgillot commented Dec 23, 2023

rustbot commented Dec 23, 2023

Mark-Simulacrum Dec 23, 2023

cjgillot Dec 24, 2023

Mark-Simulacrum Dec 24, 2023

Mark-Simulacrum commented Dec 23, 2023

cjgillot commented Dec 24, 2023

This comment has been minimized.

bors commented Dec 24, 2023

This comment has been minimized.

bors commented Dec 24, 2023

This comment has been minimized.

rust-timer commented Dec 24, 2023

This comment has been minimized.

This comment has been minimized.

Mark-Simulacrum commented Dec 29, 2023

bors commented Dec 29, 2023

bors commented Dec 29, 2023

bors commented Dec 29, 2023

rust-timer commented Dec 29, 2023

Only store StableCrateId once in DefPathTable. #119259

Only store StableCrateId once in DefPathTable. #119259

Conversation

cjgillot commented Dec 23, 2023

rustbot commented Dec 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mark-Simulacrum commented Dec 23, 2023

cjgillot commented Dec 24, 2023

This comment has been minimized.

bors commented Dec 24, 2023

This comment has been minimized.

bors commented Dec 24, 2023

This comment has been minimized.

rust-timer commented Dec 24, 2023

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

This comment has been minimized.

This comment has been minimized.

Mark-Simulacrum commented Dec 29, 2023

bors commented Dec 29, 2023

bors commented Dec 29, 2023

bors commented Dec 29, 2023

rust-timer commented Dec 29, 2023

Overall result: ✅ improvements - no action needed