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.

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

Stop walking the bodies of statics for reachability, and evaluate them instead #122371

Merged
merged 3 commits into from Mar 16, 2024

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Mar 12, 2024

cc @saethlin @RalfJung

cc #119214

This reuses the DefIdVisitor from rustc_privacy, because they basically try to do the same thing.

This PR's changes can probably be extended to constants, too, but let's tackle that separately, it's likely more involved.

@rustbot
Copy link
Collaborator

rustbot commented Mar 12, 2024

r? @fee1-dead

rustbot has assigned @fee1-dead.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 12, 2024

@bors try @rust-timer queue

@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 Mar 12, 2024
@rustbot
Copy link
Collaborator

rustbot commented Mar 12, 2024

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

@rust-timer

This comment has been minimized.

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

bors commented Mar 12, 2024

⌛ Trying commit 7b7b6b5 with merge 1a609b9...

bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 12, 2024
Stop walking the bodies of statics for reachability, and evaluate them instead

cc `@saethlin` `@RalfJung`

cc rust-lang#119214

This reuses the `DefIdVisitor` from `rustc_privacy`, because they basically try to do the same thing. It can probably be extended to constants, too, but let's tackle that separately, it's likely more involved.
@fee1-dead
Copy link
Member

Not familiar with this code, sorry.

r? compiler

@rustbot rustbot assigned fmease and unassigned fee1-dead Mar 12, 2024
@RalfJung
Copy link
Member

I can't really comment on this PR, as I know neither the intended semantics of this "reachable" pass nor the way it is used.

It would be good to have some place that documents the intended semantics -- does that exist currently? Does it need updating with this PR?

@bors
Copy link
Contributor

bors commented Mar 12, 2024

☀️ Try build successful - checks-actions
Build commit: 1a609b9 (1a609b9da1e44fc0d4e3b1a85563596147865afb)

@rust-timer

This comment has been minimized.

@saethlin
Copy link
Member

I'm pretty sure fmease doesn't know this part of the compiler either. I'll take a careful look over this later, but I think @tmiasko is a good reviewer for this based on past conversations.

r? tmiasko

@rustbot rustbot assigned tmiasko and unassigned fmease Mar 12, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (1a609b9): 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
Improvements ✅
(secondary)
-1.3% [-1.4%, -1.2%] 2
All ❌✅ (primary) - - 0

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)
7.0% [7.0%, 7.0%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-1.0% [-1.0%, -1.0%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 3.0% [-1.0%, 7.0%] 2

Cycles

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

Binary size

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.0% [0.0%, 0.0%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.1% [-0.1%, -0.1%] 6
Improvements ✅
(secondary)
-0.7% [-3.7%, -0.0%] 6
All ❌✅ (primary) -0.1% [-0.1%, 0.0%] 7

Bootstrap: 670.966s -> 671.851s (0.13%)
Artifact size: 310.09 MiB -> 310.02 MiB (-0.02%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 13, 2024
@tmiasko
Copy link
Contributor

tmiasko commented Mar 13, 2024

Could you also add a negative test case demonstrating that calling a function from an initializer of a reachable static no longer makes the function reachable?

@tmiasko tmiasko 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 14, 2024
@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 14, 2024

Added a test, and showed how it changes in the second commit

@oli-obk oli-obk removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Mar 14, 2024
@oli-obk oli-obk added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 14, 2024
@tmiasko
Copy link
Contributor

tmiasko commented Mar 14, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Mar 14, 2024

📌 Commit 746e4ef has been approved by tmiasko

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 Mar 14, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 14, 2024
Don't walk the bodies of free constants for reachability.

follow-up to rust-lang#122371

we don't have generic constants yet, so we don't need to handle failure to eval a free constant.

Associated consts and generic consts will be a different topic, that I will look into in a follow-up

r? `@tmiasko`
@bors
Copy link
Contributor

bors commented Mar 16, 2024

⌛ Testing commit 746e4ef with merge c563f2e...

@bors
Copy link
Contributor

bors commented Mar 16, 2024

☀️ Test successful - checks-actions
Approved by: tmiasko
Pushing c563f2e to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 16, 2024
@bors bors merged commit c563f2e into rust-lang:master Mar 16, 2024
12 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Mar 16, 2024
@oli-obk oli-obk deleted the visit_nested_body branch March 16, 2024 07:16
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (c563f2e): 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
Improvements ✅
(secondary)
-1.0% [-1.4%, -0.2%] 3
All ❌✅ (primary) - - 0

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)
2.6% [2.6%, 2.6%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

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

Binary size

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.0% [-0.1%, -0.0%] 6
Improvements ✅
(secondary)
-0.7% [-3.7%, -0.0%] 6
All ❌✅ (primary) -0.0% [-0.1%, -0.0%] 6

Bootstrap: 670.952s -> 670.527s (-0.06%)
Artifact size: 311.63 MiB -> 311.59 MiB (-0.01%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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
Development

Successfully merging this pull request may close these issues.

None yet

9 participants