Populate effective visibilities in 'rustc_resolve' by Bryanskiy · Pull Request #102026 · rust-lang/rust

Bryanskiy · 2022-09-19T17:27:15Z

Next part of RFC #48054.
previous: #101713

@rustbot author
r? @petrochenkov

petrochenkov · 2022-09-19T20:16:59Z

@rustbot author

This doesn't seem to work correctly if posted in the PR description.
@rustbot author

Bryanskiy · 2022-09-20T15:34:09Z

@rustbot ready

compiler/rustc_middle/src/middle/privacy.rs

compiler/rustc_span/src/def_id.rs

src/test/ui/privacy/access_levels.rs

compiler/rustc_middle/src/middle/privacy.rs

compiler/rustc_resolve/src/access_levels.rs

petrochenkov · 2022-09-21T15:13:59Z

Could you also refactor fn set_bindings_access_level into something like this - petrochenkov@155cb7e?
I already suggested it in the initial PR - #100147 (comment).

Bryanskiy · 2022-09-26T12:50:59Z

@rustbot ready

src/test/ui/privacy/access_levels.rs

compiler/rustc_privacy/src/lib.rs

compiler/rustc_middle/src/middle/privacy.rs

petrochenkov · 2022-10-05T20:19:29Z

Also #102026 (comment) wasn't addressed yet.

compiler/rustc_resolve/src/access_levels.rs

compiler/rustc_middle/src/middle/privacy.rs

compiler/rustc_resolve/src/access_levels.rs

Bryanskiy · 2022-10-09T16:19:34Z

@rustbot ready

Bryanskiy · 2022-10-09T17:12:15Z

@rustbot author

bors · 2022-10-16T20:34:42Z

⌛ Testing commit 496ccd9 with merge b8b5cae...

bors · 2022-10-16T23:15:14Z

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing b8b5cae to master...

rust-timer · 2022-10-17T00:33:12Z

Finished benchmarking commit (b8b5cae): comparison URL.

Overall result: ❌ regressions - 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)	1.1%	[0.2%, 5.6%]	65
Regressions ❌ (secondary)	0.9%	[0.2%, 3.7%]	32
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.1%	[0.2%, 5.6%]	65

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)	-	-	0
Improvements ✅ (primary)	-1.6%	[-1.6%, -1.6%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-1.6%	[-1.6%, -1.6%]	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)	2.8%	[2.1%, 3.9%]	9
Regressions ❌ (secondary)	2.9%	[2.2%, 3.9%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.8%	[2.1%, 3.9%]	9

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

nnethercote · 2022-10-17T05:53:22Z

Ouch, there are some significant performance regressions caused by this. They are bad enough that I think we should consider reverting this PR.

@Bryanskiy: did you consider compile times while working on this? Are there possibilities for optimizing the new code?

petrochenkov · 2022-10-17T06:27:06Z

This PR implements new functionality, i.e. the compiler has to do more work, so a slight regression would be expected, but probably not this much. We certainly need to check what exactly is a hotspot here.

Perhaps fully private items are visited too much and that can be avoided by pruning them early, or perhaps it's something else.

Perf improvements for effective visibility calculating related to rust-lang#102026 r? `@petrochenkov`

petrochenkov · 2022-10-29T09:07:48Z

#103158, #103496 and #103688 recover some of the performance here, and I will do some more work on this pass in the next few days which may help with performance too.

nnethercote · 2022-10-29T09:12:57Z

Great news!

Perf improvements for effective visibility calculating related to rust-lang/rust#102026 r? `@petrochenkov`

…omez rustdoc: Simplify modifications of effective visibility table It is now obvious that rustdoc only calls `set_access_level` with foreign def ids and `AccessLevel::Public`. The second commit makes one more step and separates effective visibilities coming from rustc from similar data collected by rustdoc for extern `DefId`s. The original table is no longer modified and now only contains local def ids as populated by rustc. cc rust-lang#102026 `@Bryanskiy`

rustc_metadata: Encode even less doc comments The fact that `def_id` is in the `tcx.privacy_access_levels(())` table is not very meaningful, especially after rust-lang#102026, `is_exported` (or `is_reachable` in the worst case) is what you need. Follow up to rust-lang#98450. r? `@GuillaumeGomez` `@lqd`

rustdoc: Simplify modifications of effective visibility table It is now obvious that rustdoc only calls `set_access_level` with foreign def ids and `AccessLevel::Public`. The second commit makes one more step and separates effective visibilities coming from rustc from similar data collected by rustdoc for extern `DefId`s. The original table is no longer modified and now only contains local def ids as populated by rustc. cc rust-lang/rust#102026 `@Bryanskiy`

rustc_metadata: Encode even less doc comments The fact that `def_id` is in the `tcx.privacy_access_levels(())` table is not very meaningful, especially after rust-lang/rust#102026, `is_exported` (or `is_reachable` in the worst case) is what you need. Follow up to rust-lang/rust#98450. r? `@GuillaumeGomez` `@lqd`

resolve: More detailed effective visibility tracking for imports Per-`DefId` tracking is not enough, due to glob imports in particular, which have a single `DefId` for the whole glob import item. We need to track this stuff per every introduced name (`NameBinding`). Also drop `extern` blocks from the effective visibility table, they are nominally private and it doesn't make sense to keep them there. Later commits add some debug-only invariant checking and optimiaztions to mitigate regressions in rust-lang#103965 (comment). This is a bugfix and continuation of rust-lang#102026.

…nkov Populate effective visibilities in 'rustc_resolve' Next part of RFC rust-lang#48054. previous: rust-lang#101713 `@rustbot` author r? `@petrochenkov`

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Sep 19, 2022

rust-highfive assigned petrochenkov Sep 19, 2022

rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Sep 19, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 19, 2022

petrochenkov removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 19, 2022

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 Sep 20, 2022

cjgillot reviewed Sep 20, 2022

View reviewed changes

compiler/rustc_middle/src/middle/privacy.rs Outdated Show resolved Hide resolved

compiler/rustc_span/src/def_id.rs Outdated Show resolved Hide resolved

petrochenkov reviewed Sep 20, 2022

View reviewed changes

petrochenkov 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 Sep 20, 2022

This comment was marked as resolved.

Sign in to view

Bryanskiy force-pushed the resolve_update branch from 150e9ef to 26a1d3d Compare September 26, 2022 12:08

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 Sep 26, 2022

petrochenkov reviewed Oct 5, 2022

View reviewed changes

petrochenkov reviewed Oct 6, 2022

View reviewed changes

petrochenkov 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 Oct 6, 2022

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 Oct 9, 2022

rustbot 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 Oct 9, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 16, 2022

bors merged commit b8b5cae into rust-lang:master Oct 16, 2022

rustbot added this to the 1.66.0 milestone Oct 16, 2022

rustbot added the perf-regression Performance regression. label Oct 17, 2022

Bryanskiy mentioned this pull request Oct 17, 2022

Perf improvements for effective visibility calculating #103158

Merged

petrochenkov mentioned this pull request Oct 24, 2022

rustc_metadata: Encode even less doc comments #103496

Merged

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

Auto merge of rust-lang#103158 - Bryanskiy:resolve_perf, r=petrochenkov

a5406fe

Perf improvements for effective visibility calculating related to rust-lang#102026 r? `@petrochenkov`

RalfJung pushed a commit to RalfJung/miri that referenced this pull request Oct 30, 2022

Auto merge of #103158 - Bryanskiy:resolve_perf, r=petrochenkov

8c19823

Perf improvements for effective visibility calculating related to rust-lang/rust#102026 r? `@petrochenkov`

petrochenkov mentioned this pull request Nov 5, 2022

resolve: More detailed effective visibility tracking for imports #103965

Merged

petrochenkov mentioned this pull request May 5, 2023

Populate effective visibilities in 'rustc_privacy' #110907

Merged

Bryanskiy deleted the resolve_update branch May 17, 2023 13:36

Uh oh!

Conversation

Bryanskiy commented Sep 19, 2022

Uh oh!

petrochenkov commented Sep 19, 2022

Uh oh!

Bryanskiy commented Sep 20, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

petrochenkov commented Sep 21, 2022

Uh oh!

This comment was marked as resolved.

Bryanskiy commented Sep 26, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

petrochenkov commented Oct 5, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Bryanskiy commented Oct 9, 2022

Uh oh!

Bryanskiy commented Oct 9, 2022

Uh oh!

bors commented Oct 16, 2022

Uh oh!

bors commented Oct 16, 2022

Uh oh!

rust-timer commented Oct 17, 2022

Overall result: ❌ regressions - ACTION NEEDED

Instruction count

Max RSS (memory usage)

Cycles

Footnotes

Uh oh!

nnethercote commented Oct 17, 2022

Uh oh!

petrochenkov commented Oct 17, 2022

Uh oh!

petrochenkov commented Oct 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nnethercote commented Oct 29, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

petrochenkov commented Oct 29, 2022 •

edited

Loading