Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

resolve: More detailed effective visibility tracking for imports #103965

Merged
merged 4 commits into from
Nov 8, 2022
Merged

resolve: More detailed effective visibility tracking for imports #103965

merged 4 commits into from
Nov 8, 2022

Conversation

petrochenkov
Copy link
Contributor

@petrochenkov petrochenkov commented Nov 4, 2022 •
edited
Loading

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 #103965 (comment).

This is a bugfix and continuation of #102026.

@rustbot
Copy link
Collaborator

rustbot commented Nov 4, 2022

r? @eholk

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

@rustbot 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 4, 2022
@petrochenkov petrochenkov mentioned this pull request Nov 4, 2022
Closed
@petrochenkov
Copy link
Contributor Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 4, 2022
@bors
Copy link
Contributor

bors commented Nov 4, 2022

⌛ Trying commit 07d11859ca3422305170e8d0a8dcf2ca5fc0611c with merge 84523e4ff143a1157bc045ff4ed6fc8db7a7b613...

@petrochenkov
Copy link
Contributor Author

cc @Bryanskiy

@bors
Copy link
Contributor

bors commented Nov 4, 2022

☀️ Try build successful - checks-actions
Build commit: 84523e4ff143a1157bc045ff4ed6fc8db7a7b613 (84523e4ff143a1157bc045ff4ed6fc8db7a7b613)

@rust-timer
Copy link
Collaborator

Queued 84523e4ff143a1157bc045ff4ed6fc8db7a7b613 with parent 2efb0cd, future comparison URL.

@petrochenkov petrochenkov removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 4, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (84523e4ff143a1157bc045ff4ed6fc8db7a7b613): 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-review -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.9% [0.2%, 1.6%] 26
Regressions ❌
(secondary)		 0.6% [0.3%, 1.0%] 15
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -0.5% [-0.6%, -0.4%] 8
All ❌✅ (primary) 0.9% [0.2%, 1.6%] 26

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.5% [0.5%, 0.5%] 1
Regressions ❌
(secondary)		 4.2% [4.2%, 4.2%] 1
Improvements ✅
(primary)		 -0.8% [-0.9%, -0.7%] 2
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) -0.4% [-0.9%, 0.5%] 3

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.5% [2.5%, 2.5%] 1
Regressions ❌
(secondary)		 2.7% [2.7%, 2.7%] 1
Improvements ✅
(primary)		 -0.4% [-0.4%, -0.4%] 2
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 0.6% [-0.4%, 2.5%] 3

@rustbot rustbot added perf-regression Performance regression. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Nov 4, 2022
@petrochenkov 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 Nov 4, 2022
@petrochenkov
resolve: More detailed effective visibility tracking for imports
24093fc 
Also drop `extern` blocks from the effective visibility table, they are nominally private and it doesn't make sense to keep them there.
@petrochenkov
privacy: Print effective visibilities of constructors
bb401bd
@petrochenkov
privacy: Check effective visibility invariants
448261a
@petrochenkov
Copy link
Contributor Author

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 5, 2022
@bors
Copy link
Contributor

bors commented Nov 5, 2022

⌛ Trying commit 5aa6713a19b977eac837e38df05b5b6d45947e2c with merge e2d6dc9c235b9caee330ac7d87f1db59c17a2a92...

@petrochenkov
resolve: Fill effective visibilities for import def ids in a separate…
43bea6c 
… pass

This should result in less update calls than doing it repeatedly during the fix point iteration.
@rustbot rustbot added perf-regression Performance regression. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Nov 5, 2022
@petrochenkov
Copy link
Contributor Author

Ping @oli-obk @cjgillot - you recently reviewed #102109 / #103249 and #103760, maybe you could review this PR as well?

@oli-obk
Copy link
Contributor

oli-obk commented Nov 7, 2022

Logic lgtm and makes sense. Last time it did, too, so let's ask crater at least

@craterbot check

r=me with crater not finding anything

@craterbot
Copy link
Collaborator

👌 Experiment pr-103965 created and queued.
🤖 Automatically detected try build d5fea8dd9e615740edb0e87b666e3656caa96a77
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 7, 2022
@craterbot
Copy link
Collaborator

🚧 Experiment pr-103965 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-103965 is completed!
📊 41 regressed and 12 fixed (247541 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Nov 8, 2022
@oli-obk
Copy link
Contributor

oli-obk commented Nov 8, 2022

None of the "regressions" seem remotely related and I don't know how this PR could cause

error: expected expression, found <eof>

@bors r+

@bors
Copy link
Contributor

bors commented Nov 8, 2022

📌 Commit 43bea6c has been approved by oli-obk

It is now in the queue for this repository.

@bors 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 8, 2022
@bors
Copy link
Contributor

bors commented Nov 8, 2022

⌛ Testing commit 43bea6c with merge c5842b0...

@bors
Copy link
Contributor

bors commented Nov 8, 2022

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing c5842b0 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 8, 2022
@bors bors merged commit c5842b0 into rust-lang:master Nov 8, 2022
@rustbot rustbot added this to the 1.67.0 milestone Nov 8, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (c5842b0): 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.3% [0.2%, 0.4%] 4
Regressions ❌
(secondary)		 0.3% [0.2%, 0.4%] 5
Improvements ✅
(primary)		 -0.2% [-0.2%, -0.2%] 3
Improvements ✅
(secondary)		 -0.5% [-1.1%, -0.4%] 9
All ❌✅ (primary) 0.1% [-0.2%, 0.4%] 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% [2.1%, 7.1%] 3
Improvements ✅
(primary)		 -2.7% [-2.7%, -2.7%] 1
Improvements ✅
(secondary)		 -2.3% [-2.3%, -2.3%] 1
All ❌✅ (primary) -2.7% [-2.7%, -2.7%] 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)		 - - 0
Improvements ✅
(secondary)		 -11.9% [-14.2%, -10.7%] 3
All ❌✅ (primary) - - 0

@jruderman jruderman mentioned this pull request Nov 10, 2022
Closed
@pnkfelix
Copy link
Member

This is an internal refactoring that we need to get more precise tracking of imports in presence of globs. So I am going to treat the regressions as acceptable and mark it as triaged.

@rustbot label: +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Nov 15, 2022
@petrochenkov petrochenkov mentioned this pull request Nov 23, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 25, 2022
@bors
Auto merge of rust-lang#104602 - petrochenkov:effvisperf5, r=oli-obk
41e0363 
privacy: Fix more (potential) issues with effective visibilities

Continuation of rust-lang#103965.
See individual commits for more detailed description of the changes.

The shortcuts removed in rust-lang@4eb63f6 and rust-lang@c7c7d16 could actually be correct (or correct after some tweaks), but they used global reasoning like "we can skip this update because if the code compiles then some other update should do the same thing eventually".
I have some expertise in this area, but I still have doubt whether such global reasoning was correct or not, especially in presence of all possible exotic cases with imports.
After this PR all table changes should be "locally correct" after every update, even if it may be overcautious.
If similar optimizations are introduced again they will need detailed comments explaining why it's legal to do what they do and providing proofs.

Fixes rust-lang#104249.
Fixes rust-lang#104539.
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this pull request Apr 20, 2024
@bors
Auto merge of #104602 - petrochenkov:effvisperf5, r=oli-obk
3616288 
privacy: Fix more (potential) issues with effective visibilities

Continuation of rust-lang/rust#103965.
See individual commits for more detailed description of the changes.

The shortcuts removed in rust-lang/rust@4eb63f6 and rust-lang/rust@c7c7d16 could actually be correct (or correct after some tweaks), but they used global reasoning like "we can skip this update because if the code compiles then some other update should do the same thing eventually".
I have some expertise in this area, but I still have doubt whether such global reasoning was correct or not, especially in presence of all possible exotic cases with imports.
After this PR all table changes should be "locally correct" after every update, even if it may be overcautious.
If similar optimizations are introduced again they will need detailed comments explaining why it's legal to do what they do and providing proofs.

Fixes rust-lang/rust#104249.
Fixes rust-lang/rust#104539.
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this pull request Apr 27, 2024
@bors
Auto merge of #104602 - petrochenkov:effvisperf5, r=oli-obk
fe08d3c 
privacy: Fix more (potential) issues with effective visibilities

Continuation of rust-lang/rust#103965.
See individual commits for more detailed description of the changes.

The shortcuts removed in rust-lang/rust@4eb63f6 and rust-lang/rust@c7c7d16 could actually be correct (or correct after some tweaks), but they used global reasoning like "we can skip this update because if the code compiles then some other update should do the same thing eventually".
I have some expertise in this area, but I still have doubt whether such global reasoning was correct or not, especially in presence of all possible exotic cases with imports.
After this PR all table changes should be "locally correct" after every update, even if it may be overcautious.
If similar optimizations are introduced again they will need detailed comments explaining why it's legal to do what they do and providing proofs.

Fixes rust-lang/rust#104249.
Fixes rust-lang/rust#104539.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@eholk eholk

Labels
merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.67.0
Development

Successfully merging this pull request may close these issues.
8 participants
@petrochenkov @rustbot @rust-timer @bors @oli-obk @craterbot @pnkfelix @eholk