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

petrochenkov · 2023-11-22T21:41:58Z

Before this PR the def kind query required building HIR for no good reason, with this PR def kinds are instead assigned immediately when DefIds are created.

Some PRs previously refactored things to make all def kinds to be available early enough - #118250, #118272, #118311.

rustbot · 2023-11-22T21:42:07Z

r? @TaKO8Ki

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

petrochenkov · 2023-11-22T21:42:59Z

@bors try @rust-timer queue

bors · 2023-11-22T21:44:10Z

⌛ Trying commit 6a70c75 with merge 9c24e80...

[WIP] resolve: Feed the `opt_def_kind` query immediately on `DefId` creation ... for most `DefId`s. Right now the def kind query requires building HIR for no good reason, in this PR I'm trying to change that. In two cases the `DefKind` is not known early enough. - Choosing between `DefKind::Closure` and `DefKind::Coroutine` requires analyzing (expanded) closure body, which cannot be done in def collector. I think `DefKind::Coroutine` should be merged into `DefKind::Closure`, it's more useful to have all `DefKind`s early in resolver (perhaps even merge them with `DefPathData`s) than to make this specific distinction. - `opt_def_kind` should report `None` instead of `Some(DefKind::AnonConst)` for some specific subset of anonymous constants not know in def collector, otherwise code in other parts of the compiler panics. I'll try to get rid of this oddity in a separate PR. Submitting this in a WIP state for preliminary benchmarking.

bors · 2023-11-22T23:12:22Z

☀️ Try build successful - checks-actions
Build commit: 9c24e80 (9c24e80265eaa62b2c0e674c0288b4ab9303dabe)

rust-timer · 2023-11-23T10:30:56Z

Finished benchmarking commit (9c24e80): 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.3%	[0.3%, 0.3%]	1
Improvements ✅ (primary)	-0.3%	[-0.5%, -0.2%]	75
Improvements ✅ (secondary)	-0.6%	[-1.2%, -0.1%]	37
All ❌✅ (primary)	-0.3%	[-0.5%, -0.2%]	75

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.8%	[3.8%, 3.8%]	1
Regressions ❌ (secondary)	3.5%	[0.8%, 6.3%]	3
Improvements ✅ (primary)	-1.9%	[-2.5%, -1.4%]	3
Improvements ✅ (secondary)	-2.0%	[-3.7%, -0.9%]	3
All ❌✅ (primary)	-0.5%	[-2.5%, 3.8%]	4

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: 675.709s -> 674.594s (-0.17%)
Artifact size: 313.73 MiB -> 313.82 MiB (0.03%)

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

Blocked on #118311.
@rustbot blocked

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

And move declarative macro compilation to an earlier point in def collector, which is required for rust-lang#118188.

resolve: Avoid clones of `MacroData` And move declarative macro compilation to an earlier point in def collector, which is required for rust-lang#118188.

Rollup merge of rust-lang#118272 - petrochenkov:macrodata, r=cjgillot resolve: Avoid clones of `MacroData` And move declarative macro compilation to an earlier point in def collector, which is required for rust-lang#118188.

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

rustc: `hir().local_def_id_to_hir_id()` -> `tcx.local_def_id_to_hir_id()` cleanup Noticed this while working on rust-lang#118188. The history here is that the method was moved from HIR map to tcx in rust-lang#93373 as a part of incremental compilation work, so it's unlikely to go back.

cjgillot

r=me with a nit

cjgillot · 2023-11-28T00:13:23Z

compiler/rustc_resolve/src/def_collector.rs


+        // FIXME: The namespace is incorrect for foreign types.


Please fix it as a drive-by.

Fixed in the second commit.

petrochenkov · 2023-11-28T13:19:11Z

@bors r=cjgillot

bors · 2023-11-28T13:19:13Z

📌 Commit 84de641 has been approved by cjgillot

It is now in the queue for this repository.

bors · 2023-11-28T14:54:15Z

⌛ Testing commit 84de641 with merge 5facb42...

bors · 2023-11-28T16:54:09Z

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

rust-timer · 2023-11-28T19:18:33Z

Finished benchmarking commit (5facb42): 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
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.3%	[-0.5%, -0.2%]	58
Improvements ✅ (secondary)	-0.5%	[-1.0%, -0.1%]	34
All ❌✅ (primary)	-0.3%	[-0.5%, -0.2%]	58

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.9%	[0.9%, 0.9%]	1
Regressions ❌ (secondary)	3.0%	[1.7%, 5.2%]	9
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.2%	[-1.2%, -1.2%]	2
All ❌✅ (primary)	0.9%	[0.9%, 0.9%]	1

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.2%	[-1.2%, -1.2%]	1
Improvements ✅ (secondary)	-2.3%	[-2.4%, -2.2%]	2
All ❌✅ (primary)	-1.2%	[-1.2%, -1.2%]	1

Binary size

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

Bootstrap: 672.731s -> 673.885s (0.17%)
Artifact size: 313.33 MiB -> 313.32 MiB (-0.00%)

…e_3, r=<try> revert for benchmark This PR reverts rust-lang#118311 and rust-lang#118188, with the intent to assess the performance impact brought about by rust-lang#118311

bvanjoi · 2023-11-29T15:21:01Z

compiler/rustc_interface/src/queries.rs

@@ -171,6 +172,9 @@ impl<'tcx> Queries<'tcx> {
                )));
                feed.crate_for_resolver(tcx.arena.alloc(Steal::new((krate, pre_configured_attrs))));
                feed.output_filenames(Arc::new(outputs));
+
+                let feed = tcx.feed_local_def_id(CRATE_DEF_ID);
+                feed.def_kind(DefKind::Mod);


I have a question regarding incremental builds: when referring to queries such as the results of tcx.def_kind(LocalDefId) or tcx.def_kind(DefId), does this mean the results are retrieved directly here?

Here the result is put directly into the query cache.
So when tcx.def_kind(def_id) is called it will look into the query cache first, and will always find the result there, and will never call the query implementation (which now would panic because providers.def_kind is not set).

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

rustc: Harmonize `DefKind` and `DefPathData` Follow up to rust-lang#118188. `DefPathData::(ClosureExpr,ImplTrait)` are renamed to match `DefKind::(Closure,OpaqueTy)`. `DefPathData::ImplTraitAssocTy` is replaced with `DefPathData::TypeNS(kw::Empty)` because both correspond to `DefKind::AssocTy`. It's possible that introducing `(DefKind,DefPathData)::AssocOpaqueTy` instead could be a better solution, but that would be a much more invasive change. Const generic parameters introduced for effects are moved from `DefPathData::TypeNS` to `DefPathData::ValueNS`, because constants are values. `DefPathData` is no longer passed to `create_def` functions to avoid redundancy.

Rollup merge of rust-lang#118573 - petrochenkov:pathdatakind, r=TaKO8Ki rustc: Harmonize `DefKind` and `DefPathData` Follow up to rust-lang#118188. `DefPathData::(ClosureExpr,ImplTrait)` are renamed to match `DefKind::(Closure,OpaqueTy)`. `DefPathData::ImplTraitAssocTy` is replaced with `DefPathData::TypeNS(kw::Empty)` because both correspond to `DefKind::AssocTy`. It's possible that introducing `(DefKind,DefPathData)::AssocOpaqueTy` instead could be a better solution, but that would be a much more invasive change. Const generic parameters introduced for effects are moved from `DefPathData::TypeNS` to `DefPathData::ValueNS`, because constants are values. `DefPathData` is no longer passed to `create_def` functions to avoid redundancy.

[WIP] resolve: Use def_kind query to cleanup some code Follow up to rust-lang#118188. Similar attempts to use queries in resolver resulted in perf regressions in the past, so this needs a perf run first.

rustc: Harmonize `DefKind` and `DefPathData` Follow up to rust-lang/rust#118188. `DefPathData::(ClosureExpr,ImplTrait)` are renamed to match `DefKind::(Closure,OpaqueTy)`. `DefPathData::ImplTraitAssocTy` is replaced with `DefPathData::TypeNS(kw::Empty)` because both correspond to `DefKind::AssocTy`. It's possible that introducing `(DefKind,DefPathData)::AssocOpaqueTy` instead could be a better solution, but that would be a much more invasive change. Const generic parameters introduced for effects are moved from `DefPathData::TypeNS` to `DefPathData::ValueNS`, because constants are values. `DefPathData` is no longer passed to `create_def` functions to avoid redundancy.

…rrors resolve: Use `def_kind` query to cleanup some code Follow up to rust-lang#118188. Similar attempts to use queries in resolver resulted in perf regressions in the past, so this needs a perf run first.

Rollup merge of rust-lang#118620 - petrochenkov:defeed2, r=compiler-errors resolve: Use `def_kind` query to cleanup some code Follow up to rust-lang#118188. Similar attempts to use queries in resolver resulted in perf regressions in the past, so this needs a perf run first.

rustbot assigned TaKO8Ki Nov 22, 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 Nov 22, 2023

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 22, 2023

petrochenkov removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 22, 2023

petrochenkov mentioned this pull request Nov 22, 2023

vis note for no pub reexports glob import #115993

Merged

This comment has been minimized.

Sign in to view

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

petrochenkov added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Nov 23, 2023

petrochenkov mentioned this pull request Nov 24, 2023

rustc: Make def_kind mandatory for all DefIds #118250

Merged

rustbot added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels 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.

petrochenkov mentioned this pull request Nov 24, 2023

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

Merged

petrochenkov added a commit to petrochenkov/rust that referenced this pull request Nov 24, 2023

resolve: Avoid clones of MacroData

ad0770e

And move declarative macro compilation to an earlier point in def collector, which is required for rust-lang#118188.

petrochenkov mentioned this pull request Nov 24, 2023

resolve: Avoid clones of MacroData #118272

Merged

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

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

5c97719

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

cjgillot reviewed Nov 28, 2023

View reviewed changes

petrochenkov added 2 commits November 28, 2023 15:39

resolve: Feed the def_kind query immediately on DefId creation

f0dc906

def collector: Set correct namespace in DefPathData for foreign types

84de641

petrochenkov force-pushed the defeed branch from 2b85556 to 84de641 Compare November 28, 2023 13:18

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 28, 2023

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

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

rustbot added this to the 1.76.0 milestone Nov 28, 2023

bvanjoi mentioned this pull request Nov 29, 2023

revert for benchmark #118436

Closed

pnkfelix mentioned this pull request Nov 29, 2023

Rollup of 4 pull requests #118319

Merged

bvanjoi reviewed Nov 29, 2023

View reviewed changes

bvanjoi mentioned this pull request Nov 29, 2023

aligns the behavior to that prior to #118311 #118411

Closed

flip1995 pushed a commit to flip1995/rust-clippy that referenced this pull request Dec 1, 2023

Auto merge of #118250 - petrochenkov:optdefkind, r=compiler-errors

a79a7d6

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

This was referenced Dec 2, 2023

Investigate feeding def_span in the resolver #118552

Open

Collect lang items from AST, get rid of GenericBound::LangItemTrait #118396

Merged

petrochenkov mentioned this pull request Dec 3, 2023

rustc: Harmonize DefKind and DefPathData #118573

Merged

petrochenkov mentioned this pull request Dec 4, 2023

resolve: Use def_kind query to cleanup some code #118620

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

petrochenkov commented Nov 22, 2023 •

edited

Loading

rustbot commented Nov 22, 2023

petrochenkov commented Nov 22, 2023

This comment has been minimized.

bors commented Nov 22, 2023

bors commented Nov 22, 2023

This comment has been minimized.

rust-timer commented Nov 23, 2023

petrochenkov commented Nov 24, 2023 •

edited

Loading

cjgillot left a comment

cjgillot Nov 28, 2023

petrochenkov Nov 28, 2023

petrochenkov commented Nov 28, 2023

bors commented Nov 28, 2023

bors commented Nov 28, 2023

bors commented Nov 28, 2023

rust-timer commented Nov 28, 2023

bvanjoi Nov 29, 2023

petrochenkov Nov 29, 2023

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

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

Conversation

petrochenkov commented Nov 22, 2023 • edited Loading

rustbot commented Nov 22, 2023

petrochenkov commented Nov 22, 2023

This comment has been minimized.

bors commented Nov 22, 2023

bors commented Nov 22, 2023

This comment has been minimized.

rust-timer commented Nov 23, 2023

Overall result: ✅ improvements - no action needed

petrochenkov commented Nov 24, 2023 • edited Loading

cjgillot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrochenkov commented Nov 28, 2023

bors commented Nov 28, 2023

bors commented Nov 28, 2023

bors commented Nov 28, 2023

rust-timer commented Nov 28, 2023

Overall result: ✅ improvements - no action needed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

petrochenkov commented Nov 22, 2023 •

edited

Loading

petrochenkov commented Nov 24, 2023 •

edited

Loading