rust icon indicating copy to clipboard operation
rust copied to clipboard

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

Open oli-obk opened this issue 1 year ago • 11 comments

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.

oli-obk avatar Mar 12 '24 08:03 oli-obk

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

rustbot avatar Mar 12 '24 08:03 rustbot

@bors try @rust-timer queue

oli-obk avatar Mar 12 '24 08:03 oli-obk

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.

rustbot avatar Mar 12 '24 08:03 rustbot

Awaiting bors try build completion.

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

rust-timer avatar Mar 12 '24 08:03 rust-timer

:hourglass: Trying commit 7b7b6b53ee704996ee9038944991855353da73eb with merge 1a609b9da1e44fc0d4e3b1a85563596147865afb...

bors avatar Mar 12 '24 08:03 bors

Not familiar with this code, sorry.

r? compiler

fee1-dead avatar Mar 12 '24 09:03 fee1-dead

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?

RalfJung avatar Mar 12 '24 10:03 RalfJung

:sunny: Try build successful - checks-actions Build commit: 1a609b9da1e44fc0d4e3b1a85563596147865afb (1a609b9da1e44fc0d4e3b1a85563596147865afb)

bors avatar Mar 12 '24 10:03 bors

Queued 1a609b9da1e44fc0d4e3b1a85563596147865afb with parent b0170b693ef91460c97ff9ea5e360888466611dd, future comparison URL. There are currently 2 preceding artifacts in the queue. It will probably take at least ~3.0 hours until the benchmark run finishes.

rust-timer avatar Mar 12 '24 10:03 rust-timer

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

saethlin avatar Mar 12 '24 17:03 saethlin

Finished benchmarking commit (1a609b9da1e44fc0d4e3b1a85563596147865afb): 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%)

rust-timer avatar Mar 13 '24 00:03 rust-timer

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 avatar Mar 13 '24 21:03 tmiasko

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

oli-obk avatar Mar 14 '24 14:03 oli-obk

@bors r+

tmiasko avatar Mar 14 '24 16:03 tmiasko

:pushpin: Commit 746e4eff263527ba8960552ea62db11ab9821644 has been approved by tmiasko

It is now in the queue for this repository.

bors avatar Mar 14 '24 16:03 bors

:hourglass: Testing commit 746e4eff263527ba8960552ea62db11ab9821644 with merge c563f2ee799b285067124b516ce99f26063f8351...

bors avatar Mar 16 '24 04:03 bors

:sunny: Test successful - checks-actions Approved by: tmiasko Pushing c563f2ee799b285067124b516ce99f26063f8351 to master...

bors avatar Mar 16 '24 06:03 bors

Finished benchmarking commit (c563f2ee799b285067124b516ce99f26063f8351): 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%)

rust-timer avatar Mar 16 '24 07:03 rust-timer