resolve: Replace visibility table in resolver outputs with query feeding #118657

petrochenkov · 2023-12-05T21:10:02Z

Also feed missing visibilities for import stems and trait impl items, which were previously evaluated lazily.

I suspect that in general this approach should work for queries that are 1) executed for most keys and 2) have results that are cheap to hash (do not have spans, in particular).
Visibility query matches that description.

rustbot · 2023-12-05T21:10:10Z

r? @cjgillot

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

petrochenkov · 2023-12-05T21:10:17Z

@bors try @rust-timer queue

bors · 2023-12-05T21:11:27Z

⌛ Trying commit 1f539c6 with merge 526830d...

resolve: Replace visibility table in resolver outputs with query feeding I suspect that in general this approach should work for queries that are 1) executed for most keys and 2) have results that are cheap to hash (do not have spans, in particular). Visibility query matches that description, so let's check.

bors · 2023-12-05T22:38:06Z

☀️ Try build successful - checks-actions
Build commit: 526830d (526830dfc04519a6682a4ffe88879046d5aff691)

rust-timer · 2023-12-06T02:47:27Z

Finished benchmarking commit (526830d): 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.3%	[-0.5%, -0.1%]	49
Improvements ✅ (secondary)	-0.5%	[-1.3%, -0.2%]	31
All ❌✅ (primary)	-0.3%	[-0.5%, -0.1%]	49

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)	2.6%	[2.6%, 2.6%]	1
Regressions ❌ (secondary)	0.9%	[0.8%, 1.1%]	3
Improvements ✅ (primary)	-0.6%	[-0.8%, -0.4%]	4
Improvements ✅ (secondary)	-3.7%	[-4.6%, -2.0%]	5
All ❌✅ (primary)	0.0%	[-0.8%, 2.6%]	5

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)	-	-	0
Improvements ✅ (secondary)	-2.5%	[-3.1%, -2.1%]	3
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 675.495s -> 674.636s (-0.13%)
Artifact size: 314.16 MiB -> 314.17 MiB (0.00%)

petrochenkov · 2023-12-06T14:20:20Z

@rustbot ready

petrochenkov · 2023-12-06T14:22:07Z

@cjgillot
Is it expected that feeding the same query for the same key more than once quietly succeeds?
I expected some kind of panic.
What happens if two different values are fed?

cjgillot · 2023-12-09T13:56:30Z

Is it expected that feeding the same query for the same key more than once quietly succeeds?

Yes. I'm not sure this is actually useful, but I wanted to avoid ICEing too eagerly.

I expected some kind of panic.
What happens if two different values are fed?

We check that the fingerprints match. If the fingerprints do not match, we delay a bug. If the query is no-hash, we panic.
rustc_middle::query::plumbing, line 543, and rustc_query_system::query::plumbing, line 401.

cjgillot · 2023-12-09T13:57:06Z

compiler/rustc_privacy/src/lib.rs

+        // Unique types created for closures participate in type privacy checking.
+        // They have visibilities inherited from the module they are defined in.
+        Node::Expr(hir::Expr { kind: hir::ExprKind::Closure { .. }, .. }) => {
+            ty::Visibility::Restricted(tcx.parent_module_from_def_id(def_id).to_def_id())


Is there a way to get rid of this completely?

Yes, but I wanted to do it in a separate PR because it may have a negative performance impact.
(Visibilities for item-like nodes are always queried anyway because they are encoded into metadata, but visibilities for closures are rarely queried so it makes sense to do it on demand.)

cjgillot · 2023-12-09T14:29:29Z

@bors r+

bors · 2023-12-09T14:29:31Z

📌 Commit 18d2bd2 has been approved by cjgillot

It is now in the queue for this repository.

bjorn3 · 2023-12-09T18:54:39Z

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

@@ -1133,8 +1131,6 @@ pub(super) fn crate_hash(tcx: TyCtxt<'_>, _: LocalCrate) -> Svh {
        }
        tcx.sess.opts.dep_tracking_hash(true).hash_stable(&mut hcx, &mut stable_hasher);
        tcx.stable_crate_id(LOCAL_CRATE).hash_stable(&mut hcx, &mut stable_hasher);
-        // Hash visibility information since it does not appear in HIR.
-        resolutions.visibilities.hash_stable(&mut hcx, &mut stable_hasher);


This is now hashed into tcx.hir_crate(()).opt_hir_hash? Or how do we ensure that changing visibilities still affect the crate hash?

I have restored the previous visibility hashing logic in a slightly simplified form.

bors · 2023-12-09T21:00:53Z

⌛ Testing commit 18d2bd2 with merge 8f2d30a...

resolve: Replace visibility table in resolver outputs with query feeding Also feed missing visibilities for import stems and trait impl items, which were previously evaluated lazily. I suspect that in general this approach should work for queries that are 1) executed for most keys and 2) have results that are cheap to hash (do not have spans, in particular). Visibility query matches that description.

cjgillot · 2023-12-09T21:07:51Z

@bors r-
For the crate_hash situation.

bors · 2023-12-09T23:00:59Z

☀️ Try build successful - checks-actions
Build commit: 8f2d30a (8f2d30a38c30fbf96c0ec05dc28be5d33ce0ad59)

petrochenkov · 2023-12-12T22:20:14Z

#118657 (comment)
@rustbot ready

cjgillot · 2023-12-17T21:04:30Z

r=me after rebase

Also feed missing visibilities for import stems and trait impl items, which were previously evaluated lazily.

petrochenkov · 2023-12-18T00:41:46Z

@bors r=cjgillot

bors · 2023-12-18T00:41:49Z

📌 Commit be321aa has been approved by cjgillot

It is now in the queue for this repository.

bors · 2023-12-18T02:06:04Z

⌛ Testing commit be321aa with merge 321b656...

bors · 2023-12-18T04:03:50Z

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

rust-timer · 2023-12-18T05:24:17Z

Finished benchmarking commit (321b656): 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.1%]	70
Improvements ✅ (secondary)	-0.6%	[-1.2%, -0.1%]	42
All ❌✅ (primary)	-0.3%	[-0.5%, -0.1%]	70

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)	1.0%	[1.0%, 1.0%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.6%	[-2.0%, -1.1%]	3
Improvements ✅ (secondary)	-2.0%	[-3.6%, -0.4%]	10
All ❌✅ (primary)	-1.0%	[-2.0%, 1.0%]	4

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)	-2.9%	[-2.9%, -2.8%]	2
All ❌✅ (primary)	-1.1%	[-1.1%, -1.1%]	1

Binary size

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

Bootstrap: 670.368s -> 675.09s (0.70%)
Artifact size: 312.44 MiB -> 312.50 MiB (0.02%)

resolve: Eagerly feed closure visibilities Addresses rust-lang#118657 (comment)

resolve: Eagerly feed closure visibilities Also factor out all tcx-dependent operations performed for every created definition into `TyCtxt::create_def`. Addresses rust-lang#118657 (comment)

danielhjacobs · 2023-12-20T16:02:40Z

This PR seems to be causing an ICE in #119126

rustbot assigned cjgillot Dec 5, 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 5, 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 Dec 5, 2023

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

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 Dec 6, 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 Dec 6, 2023

petrochenkov force-pushed the feedvis branch 2 times, most recently from 66f2439 to 18d2bd2 Compare December 6, 2023 14:18

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 6, 2023

cjgillot reviewed Dec 9, 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 Dec 9, 2023

bjorn3 reviewed Dec 9, 2023

View reviewed changes

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 9, 2023

petrochenkov force-pushed the feedvis branch from 18d2bd2 to b893623 Compare December 12, 2023 22:17

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 12, 2023

This comment was marked as resolved.

Sign in to view

resolve: Replace visibility table in resolver outputs with query feeding

be321aa

Also feed missing visibilities for import stems and trait impl items, which were previously evaluated lazily.

petrochenkov force-pushed the feedvis branch from b893623 to be321aa Compare December 18, 2023 00:40

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

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

bors merged commit 321b656 into rust-lang:master Dec 18, 2023
12 checks passed

rustbot added this to the 1.76.0 milestone Dec 18, 2023

bors mentioned this pull request Dec 18, 2023

Delegation implementation: step 1 #118947

Merged

matthiaskrgr mentioned this pull request Dec 18, 2023

ICE: privacy: assertion failed: AssocFn == Closure #119073

Closed

petrochenkov mentioned this pull request Dec 19, 2023

resolve: Eagerly feed closure visibilities #119136

Merged

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

Auto merge of rust-lang#119136 - petrochenkov:feedvis3, r=<try>

5b028d4

resolve: Eagerly feed closure visibilities Addresses rust-lang#118657 (comment)

sjwang05 mentioned this pull request Dec 19, 2023

Nightly compiler error compiling wayland protocols on linux #119126

Closed

Jules-Bertholet mentioned this pull request Dec 31, 2023

Compiler crash in 1.76.0-beta.1 #119463

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resolve: Replace visibility table in resolver outputs with query feeding #118657

resolve: Replace visibility table in resolver outputs with query feeding #118657

petrochenkov commented Dec 5, 2023 •

edited

Loading

rustbot commented Dec 5, 2023

petrochenkov commented Dec 5, 2023

This comment has been minimized.

bors commented Dec 5, 2023

bors commented Dec 5, 2023

This comment has been minimized.

rust-timer commented Dec 6, 2023

petrochenkov commented Dec 6, 2023

petrochenkov commented Dec 6, 2023 •

edited

Loading

cjgillot commented Dec 9, 2023

cjgillot Dec 9, 2023

petrochenkov Dec 9, 2023

cjgillot commented Dec 9, 2023

bors commented Dec 9, 2023

bjorn3 Dec 9, 2023

petrochenkov Dec 12, 2023

bors commented Dec 9, 2023

cjgillot commented Dec 9, 2023

bors commented Dec 9, 2023

petrochenkov commented Dec 12, 2023

This comment was marked as resolved.

cjgillot commented Dec 17, 2023

petrochenkov commented Dec 18, 2023

bors commented Dec 18, 2023

bors commented Dec 18, 2023

bors commented Dec 18, 2023

rust-timer commented Dec 18, 2023

danielhjacobs commented Dec 20, 2023

resolve: Replace visibility table in resolver outputs with query feeding #118657

resolve: Replace visibility table in resolver outputs with query feeding #118657

Conversation

petrochenkov commented Dec 5, 2023 • edited Loading

rustbot commented Dec 5, 2023

petrochenkov commented Dec 5, 2023

This comment has been minimized.

bors commented Dec 5, 2023

bors commented Dec 5, 2023

This comment has been minimized.

rust-timer commented Dec 6, 2023

Overall result: ✅ improvements - no action needed

petrochenkov commented Dec 6, 2023

petrochenkov commented Dec 6, 2023 • edited Loading

cjgillot commented Dec 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjgillot commented Dec 9, 2023

bors commented Dec 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Dec 9, 2023

cjgillot commented Dec 9, 2023

bors commented Dec 9, 2023

petrochenkov commented Dec 12, 2023

This comment was marked as resolved.

cjgillot commented Dec 17, 2023

petrochenkov commented Dec 18, 2023

bors commented Dec 18, 2023

bors commented Dec 18, 2023

bors commented Dec 18, 2023

rust-timer commented Dec 18, 2023

Overall result: ✅ improvements - no action needed

danielhjacobs commented Dec 20, 2023

petrochenkov commented Dec 5, 2023 •

edited

Loading

petrochenkov commented Dec 6, 2023 •

edited

Loading