rustc: Make `def_kind` mandatory for all `DefId`s #118250

petrochenkov · 2023-11-24T16:20:53Z

Prerequisite for #118188.

rustbot · 2023-11-24T16:21:01Z

r? @TaKO8Ki

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

rustbot · 2023-11-24T16:21:04Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

petrochenkov · 2023-11-24T16:21:53Z

@bors try @rust-timer queue

bors · 2023-11-24T16:23:02Z

⌛ Trying commit 331df84 with merge f86037d...

rustc: Make `def_kind` mandatory for all `DefId`s Prerequisite for rust-lang#118188.

bors · 2023-11-24T17:49:15Z

☀️ Try build successful - checks-actions
Build commit: f86037d (f86037df69d1452502b1846b6754a21a1e184d23)

compiler-errors

👍 like this a lot. a few comments (and i know perf is running)

compiler/rustc_metadata/src/rmeta/mod.rs

compiler-errors · 2023-11-24T19:19:48Z

compiler/rustc_middle/src/hir/map/mod.rs

@@ -895,7 +891,7 @@ impl<'hir> Map<'hir> {

    #[inline]
    fn opt_ident(self, id: HirId) -> Option<Ident> {
-        match self.get(id) {
+        match self.find(id)? {


What codepath triggers this specifically? Just curious.

def_span for anonymous constants without HIR.

Should we keep a FIXME here too?

HirIds do not always have HIR nodes or Idents, so the new behavior looks reasonable to me, we just didn't hit this case before.

compiler/rustc_metadata/src/rmeta/encoder.rs

rust-timer · 2023-11-25T00:28:37Z

Finished benchmarking commit (f86037d): comparison URL.

Overall result: ✅ improvements - no 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.

@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
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.2%	[-0.2%, -0.1%]	4
Improvements ✅ (secondary)	-0.3%	[-0.6%, -0.2%]	9
All ❌✅ (primary)	-0.2%	[-0.2%, -0.1%]	4

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)	3.2%	[3.2%, 3.2%]	1
Regressions ❌ (secondary)	1.5%	[1.5%, 1.5%]	1
Improvements ✅ (primary)	-3.4%	[-3.4%, -3.4%]	1
Improvements ✅ (secondary)	-0.9%	[-0.9%, -0.9%]	1
All ❌✅ (primary)	-0.1%	[-3.4%, 3.2%]	2

Cycles

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

Binary size

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

Bootstrap: 674.644s -> 675.592s (0.14%)
Artifact size: 313.53 MiB -> 313.54 MiB (0.01%)

cjgillot · 2023-11-25T02:01:38Z

compiler/rustc_metadata/src/rmeta/encoder.rs

@@ -1393,7 +1392,13 @@ impl<'a, 'tcx> EncodeContext<'a, 'tcx> {
            if should_encode_fn_sig(def_kind) {
                record!(self.tables.fn_sig[def_id] <- tcx.fn_sig(def_id));
            }
-            if should_encode_generics(def_kind) {
+            // FIXME: Some anonymous constants produced by `#[rustc_legacy_const_generics]`


Should we give them a dummy hir node to avoid those ices?

Maybe, I just didn't want to get into that in this PR, #118188 is not blocked on it.

I tried to look what exactly happens with rustc_legacy_const_generics, but it's still not clear to me.
There seems to be some mismatch between what def collector and AST lowering do.

cjgillot · 2023-11-25T02:02:14Z

compiler/rustc_middle/src/hir/map/mod.rs

@@ -895,7 +891,7 @@ impl<'hir> Map<'hir> {

    #[inline]
    fn opt_ident(self, id: HirId) -> Option<Ident> {
-        match self.get(id) {
+        match self.find(id)? {


Should we keep a FIXME here too?

compiler-errors

r=me unless you want another round of review from cjgillot

petrochenkov · 2023-11-25T20:59:00Z

@bors r=compiler-errors
I'd want to see #118250 (comment) addressed somehow, just don't have time to investigate right now.

bors · 2023-11-25T20:59:02Z

📌 Commit 2c23386 has been approved by compiler-errors

It is now in the queue for this repository.

bors · 2023-11-26T04:44:23Z

⌛ Testing commit 2c23386 with merge 5c97719...

bors · 2023-11-26T06:42:56Z

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 5c97719 to master...

rust-timer · 2023-11-26T08:06:06Z

Finished benchmarking commit (5c97719): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

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
Regressions ❌ (secondary)	0.5%	[0.5%, 0.5%]	2
Improvements ✅ (primary)	-0.1%	[-0.1%, -0.1%]	5
Improvements ✅ (secondary)	-0.3%	[-0.5%, -0.2%]	9
All ❌✅ (primary)	-0.1%	[-0.1%, -0.1%]	5

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.7%	[2.8%, 4.5%]	2
Improvements ✅ (primary)	-2.0%	[-2.4%, -1.6%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.0%	[-2.4%, -1.6%]	2

Cycles

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

Binary size

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

Bootstrap: 674.797s -> 672.47s (-0.34%)
Artifact size: 313.34 MiB -> 313.36 MiB (0.01%)

nnethercote · 2023-11-26T22:51:17Z

If anything, this looks like a tiny performance win.

@rustbot label: +perf-regression-triaged

resolve: Feed the `def_kind` query immediately on `DefId` creation Before this PR the def kind query required building HIR for no good reason, with this PR def kinds are instead assigned immediately when `DefId`s are created. Some PRs previously refactored things to make all def kinds to be available early enough - rust-lang#118250, rust-lang#118272, rust-lang#118311.

…errors rustc: Make `def_kind` mandatory for all `DefId`s Prerequisite for rust-lang#118188.

rustbot assigned TaKO8Ki Nov 24, 2023

petrochenkov mentioned this pull request Nov 24, 2023

resolve: Feed the def_kind query immediately on DefId creation #118188

Merged

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 Nov 24, 2023

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

Auto merge of rust-lang#118250 - petrochenkov:optdefkind, r=<try>

f86037d

rustc: Make `def_kind` mandatory for all `DefId`s Prerequisite for rust-lang#118188.

This comment has been minimized.

Sign in to view

compiler-errors reviewed Nov 24, 2023

View reviewed changes

compiler-errors assigned compiler-errors and unassigned TaKO8Ki Nov 24, 2023

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 25, 2023

cjgillot reviewed Nov 25, 2023

View reviewed changes

rustc: Make def_kind mandatory for all DefIds

2c23386

petrochenkov force-pushed the optdefkind branch from 331df84 to 2c23386 Compare November 25, 2023 12:21

compiler-errors approved these changes Nov 25, 2023

View reviewed changes

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 Nov 25, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 26, 2023

bors merged commit 5c97719 into rust-lang:master Nov 26, 2023
12 checks passed

rustbot added this to the 1.76.0 milestone Nov 26, 2023

bors mentioned this pull request Nov 26, 2023

rustc: hir().local_def_id_to_hir_id() -> tcx.local_def_id_to_hir_id() cleanup #118256

Merged

rustbot added the perf-regression Performance regression. label Nov 26, 2023

xFrednet mentioned this pull request Nov 26, 2023

Chore: Bump nightly -> 2023-12-28 rust-marker/marker#315

Open

rustbot added the perf-regression-triaged The performance regression has been triaged. label Nov 26, 2023

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

Auto merge of rust-lang#118250 - petrochenkov:optdefkind, r=compiler-…

a79a7d6

…errors rustc: Make `def_kind` mandatory for all `DefId`s Prerequisite for rust-lang#118188.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustc: Make `def_kind` mandatory for all `DefId`s #118250

rustc: Make `def_kind` mandatory for all `DefId`s #118250

petrochenkov commented Nov 24, 2023

rustbot commented Nov 24, 2023

rustbot commented Nov 24, 2023

petrochenkov commented Nov 24, 2023

This comment has been minimized.

bors commented Nov 24, 2023

bors commented Nov 24, 2023

This comment has been minimized.

compiler-errors left a comment

compiler-errors Nov 24, 2023

petrochenkov Nov 24, 2023

cjgillot Nov 25, 2023

petrochenkov Nov 25, 2023 •

edited

Loading

rust-timer commented Nov 25, 2023

cjgillot Nov 25, 2023

petrochenkov Nov 25, 2023

cjgillot Nov 25, 2023

compiler-errors left a comment

petrochenkov commented Nov 25, 2023

bors commented Nov 25, 2023

bors commented Nov 26, 2023

bors commented Nov 26, 2023

rust-timer commented Nov 26, 2023

nnethercote commented Nov 26, 2023

rustc: Make def_kind mandatory for all DefIds #118250

rustc: Make def_kind mandatory for all DefIds #118250

Conversation

petrochenkov commented Nov 24, 2023

rustbot commented Nov 24, 2023

rustbot commented Nov 24, 2023

petrochenkov commented Nov 24, 2023

This comment has been minimized.

bors commented Nov 24, 2023

bors commented Nov 24, 2023

This comment has been minimized.

compiler-errors left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrochenkov Nov 25, 2023 • edited Loading

Choose a reason for hiding this comment

rust-timer commented Nov 25, 2023

Overall result: ✅ improvements - no action needed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compiler-errors left a comment

Choose a reason for hiding this comment

petrochenkov commented Nov 25, 2023

bors commented Nov 25, 2023

bors commented Nov 26, 2023

bors commented Nov 26, 2023

rust-timer commented Nov 26, 2023

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

nnethercote commented Nov 26, 2023

rustc: Make `def_kind` mandatory for all `DefId`s #118250

rustc: Make `def_kind` mandatory for all `DefId`s #118250

petrochenkov Nov 25, 2023 •

edited

Loading