resolve: Querify most cstore access methods (subset) #108992

petrochenkov · 2023-03-10T16:53:29Z

A subset of #108346 that is not on a hot path in any way.

rustbot · 2023-03-10T16:53:35Z

r? @wesleywiser

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

petrochenkov · 2023-03-10T16:53:44Z

@bors try @rust-timer queue

bors · 2023-03-10T16:53:54Z

⌛ Trying commit 609146135ed161149f162055ff123870fd593b26 with merge 09e4d7a44bc9c01eb4c5317d4aa7b60877e4aa78...

bors · 2023-03-10T19:08:52Z

☀️ Try build successful - checks-actions
Build commit: 09e4d7a44bc9c01eb4c5317d4aa7b60877e4aa78 (09e4d7a44bc9c01eb4c5317d4aa7b60877e4aa78)

bors · 2023-03-10T19:08:52Z

☀️ Try build successful - checks-actions
Build commit: 09e4d7a44bc9c01eb4c5317d4aa7b60877e4aa78 (09e4d7a44bc9c01eb4c5317d4aa7b60877e4aa78)

rust-timer · 2023-03-10T21:33:27Z

Finished benchmarking commit (09e4d7a44bc9c01eb4c5317d4aa7b60877e4aa78): 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.5%	[0.5%, 0.5%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.3%	[-0.6%, -0.2%]	6
Improvements ✅ (secondary)	-1.8%	[-1.8%, -1.8%]	1
All ❌✅ (primary)	-0.2%	[-0.6%, 0.5%]	7

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)	4.2%	[4.2%, 4.2%]	1
Improvements ✅ (primary)	-0.7%	[-0.8%, -0.7%]	2
Improvements ✅ (secondary)	-3.8%	[-6.0%, -2.0%]	4
All ❌✅ (primary)	-0.7%	[-0.8%, -0.7%]	2

Cycles

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

petrochenkov · 2023-03-11T10:49:11Z

r? @cjgillot
@rustbot ready

cjgillot

What a cleanup!

cjgillot · 2023-03-11T13:39:46Z

compiler/rustc_resolve/src/lib.rs

+    fn associated_item_def_ids(&self, def_id: DefId) -> Option<&'tcx [DefId]> {
+        match def_id.as_local() {
+            Some(def_id) => self.associated_item_def_ids.get(&def_id).copied(),
+            None => Some(self.tcx.associated_item_def_ids(def_id)),


The naming here is off.

The associated_item_def_ids query is only supposed to work on traits and impl blocks, to return the def-ids of the contained items. Using it to access an ADT's fields is a quirk of metadata decoding, which stores both information in the same children table and does not validate the parent before answering.

The "proper" way to access the information would use the tcx.adt_def query.

In the same idea, this method and the field in the first branch would be better named field_def_ids, as that's what they are.

The associated_item_def_ids query is only supposed to work on traits and impl blocks, to return the def-ids of the contained items.

I'd say that fields are close enough to bless this use.
Previously associated_item_def_ids was certainly used for fields in rustdoc (until #94857 nuked all the relevant code) and maybe in rustc too.

The "proper" way to access the information would use the tcx.adt_def query.

adt_def query cannot be used in resolve because it create cycles (it needs lang items).
Also it decodes much more information than necessary in our case, but it doesn't matter much because all its uses would be in diagnostics (but in general associated_item_def_ids relates to the adt_def query in the same way as to associated_item, i.e. minimizing decoding and dependencies).

In the same idea, this method and the field in the first branch would be better named field_def_ids, as that's what they are.

Yeah, I'll rename.
I used the current naming to maybe extend it to other associated items in the future, but now I agree that it only brings confusion.

cjgillot · 2023-03-11T13:40:07Z

tests/ui/empty/empty-struct-tuple-pat.stderr

-LL |         XE::XEmpty5(/* fields */) => (),
-   |         ~~~~~~~~~~~~~~~~~~~~~~~~~
+LL |         XE::XEmpty5() => (),
+   |         ~~~~~~~~~~~~~


Where does the diagnostic change come from?

Previosly insert_field_names_extern wasn't called for enum variant fields, but now fields for extern variants are successfully retrieved, this is a correct fix.

petrochenkov · 2023-03-13T13:53:44Z

@rustbot ready

cjgillot · 2023-03-13T19:24:11Z

@bors r+

bors · 2023-03-13T19:24:13Z

📌 Commit 4a61922 has been approved by cjgillot

It is now in the queue for this repository.

bors · 2023-03-14T03:38:45Z

⌛ Testing commit 4a61922 with merge bd43458...

bors · 2023-03-14T06:42:08Z

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

rust-timer · 2023-03-14T08:00:17Z

Finished benchmarking commit (bd43458): 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.5%	[0.5%, 0.5%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.5%	[-1.2%, -0.2%]	15
Improvements ✅ (secondary)	-1.6%	[-2.1%, -1.1%]	2
All ❌✅ (primary)	-0.4%	[-1.2%, 0.5%]	16

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.6%	[3.1%, 4.3%]	3
Improvements ✅ (primary)	-3.1%	[-3.1%, -3.1%]	1
Improvements ✅ (secondary)	-5.8%	[-8.4%, -3.7%]	3
All ❌✅ (primary)	-3.1%	[-3.1%, -3.1%]	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)	2.3%	[2.3%, 2.3%]	1
Improvements ✅ (primary)	-1.5%	[-1.5%, -1.5%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-1.5%	[-1.5%, -1.5%]	1

rustbot assigned wesleywiser Mar 10, 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 Mar 10, 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 Mar 10, 2023

This comment has been minimized.

Sign in to view

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

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Mar 10, 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 Mar 10, 2023

petrochenkov mentioned this pull request Mar 10, 2023

resolve: Querify most cstore access methods #108346

Closed

petrochenkov force-pushed the qcstore2 branch from 6091461 to 119edcb Compare March 11, 2023 10:48

rustbot assigned cjgillot and unassigned wesleywiser Mar 11, 2023

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 Mar 11, 2023

cjgillot reviewed Mar 11, 2023

View reviewed changes

cjgillot 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-review Status: Awaiting review from the assignee but also interested parties. labels Mar 11, 2023

petrochenkov added 3 commits March 13, 2023 17:31

resolve: Remove visibility_untracked

17127f3

resolve: Partially remove item_attrs_untracked

901f1c9

resolve: Remove struct_field_visibilities_untracked

c05b7bd

petrochenkov added 5 commits March 13, 2023 17:31

resolve: Remove fn_has_self_parameter_untracked

c7f424b

resolve: Centralize retrieval of items span and item name

2a716f3

resolve: Remove struct_field_names_untracked

b3ee735

metadata: Remove some more untracked CStore methods

98cce81

metadata/resolve: Minor refactoring to "tcx -> cstore" conversions

4a61922

petrochenkov force-pushed the qcstore2 branch from 119edcb to 4a61922 Compare March 13, 2023 13:53

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 Mar 13, 2023

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 Mar 13, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 14, 2023

bors merged commit bd43458 into rust-lang:master Mar 14, 2023

rustbot added this to the 1.70.0 milestone Mar 14, 2023

This was referenced Mar 14, 2023

error-msg: impl better suggestion for E0532 #108971

Merged

Remember names of cfg-ed out items to mention them in diagnostics #109005

Merged

rustbot removed the perf-regression Performance regression. label Mar 14, 2023

matthiaskrgr mentioned this pull request Nov 7, 2023

ice: const_trait_impl: async functions cannot be const fn #117629

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resolve: Querify most cstore access methods (subset) #108992

resolve: Querify most cstore access methods (subset) #108992

petrochenkov commented Mar 10, 2023

rustbot commented Mar 10, 2023

petrochenkov commented Mar 10, 2023

This comment has been minimized.

bors commented Mar 10, 2023

bors commented Mar 10, 2023

bors commented Mar 10, 2023

This comment has been minimized.

rust-timer commented Mar 10, 2023

petrochenkov commented Mar 11, 2023

cjgillot left a comment

cjgillot Mar 11, 2023

petrochenkov Mar 13, 2023

cjgillot Mar 11, 2023

petrochenkov Mar 13, 2023

petrochenkov commented Mar 13, 2023

cjgillot commented Mar 13, 2023

bors commented Mar 13, 2023

bors commented Mar 14, 2023

bors commented Mar 14, 2023

rust-timer commented Mar 14, 2023

resolve: Querify most cstore access methods (subset) #108992

resolve: Querify most cstore access methods (subset) #108992

Conversation

petrochenkov commented Mar 10, 2023

rustbot commented Mar 10, 2023

petrochenkov commented Mar 10, 2023

This comment has been minimized.

bors commented Mar 10, 2023

bors commented Mar 10, 2023

bors commented Mar 10, 2023

This comment has been minimized.

rust-timer commented Mar 10, 2023

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

petrochenkov commented Mar 11, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrochenkov commented Mar 13, 2023

cjgillot commented Mar 13, 2023

bors commented Mar 13, 2023

bors commented Mar 14, 2023

bors commented Mar 14, 2023

rust-timer commented Mar 14, 2023

Overall result: ✅ improvements - no action needed