Remove some eval_always

[cjgillot]

Some eval_always marker may not be useful and trigger unnecessary query recomputations. This PR attempts to measure the amount of such recomputations.

Blocked on #90581

[cjgillot]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

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

[bors]

⌛ Trying commit eaafd8ead4d98f3d149f5cd0c09571b683150c98 with merge 8d3ac8e1689d1b87817fc925bd2897fa410c6079...

[bors]

☀️ Try build successful - checks-actions
Build commit: 8d3ac8e1689d1b87817fc925bd2897fa410c6079 (8d3ac8e1689d1b87817fc925bd2897fa410c6079)

[rust-timer]

Queued 8d3ac8e1689d1b87817fc925bd2897fa410c6079 with parent 3740ba2, future comparison URL.

[rust-timer]

Finished benchmarking try commit (8d3ac8e1689d1b87817fc925bd2897fa410c6079): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

[jyn514]

@cjgillot the biggest slowdown I saw was from privacy_access_levels - I wonder if that map is being hashed now when it wasn't before? If you add no_hash, does it speed things up?

[jyn514]

Also, I'd be curious to see the perf impact of just the first commit, using that for rust-lang/compiler-team#437 would make things a lot easier I think.

[cjgillot]

The first commit is #85941

[cjgillot]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

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

[bors]

⌛ Trying commit 52cae872b76456e98d0f279ce81d2dde50ee95e9 with merge 0b514f420336f0083b2363744ce7bdcdced7283f...

[bors]

☀️ Try build successful - checks-actions
Build commit: 0b514f420336f0083b2363744ce7bdcdced7283f (0b514f420336f0083b2363744ce7bdcdced7283f)

[rust-timer]

Queued 0b514f420336f0083b2363744ce7bdcdced7283f with parent 308fc23, future comparison URL.

[rust-timer]

Finished benchmarking try commit (0b514f420336f0083b2363744ce7bdcdced7283f): comparison url.

Summary: This change led to significant mixed results 🤷 in compiler performance.

• Large improvement in instruction counts (up to -5.4% on incr-unchanged builds of ripgrep-check)
• Moderate regression in instruction counts (up to 3.7% on incr-patched: dummy fn builds of unused-warnings-check)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

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 led to changes in compiler perf.

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 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

[cjgillot]

r? @Aaron1011
But without any hurry: we maybe should wait for the unstable fingerprint situation to be settled before exposing those queries to it.

[cjgillot]

@Aaron1011, I split this PR into several others, to measure the perf impact of individual commits.

• Remove eval_always for inherent_impls. #90579
• Remove eval_always from upvars. #90580
• Remove eval_always for privacy. #90581

If their impact is reasonable, I suggest you review them independently.
(Having the commits separated would also help in case of a bug.)

[jackh726]

Marking as blocked on the above PRs - but I could also imagine it might be better to just land these in individual PRs.

[apiraino]

@cjgillot are also these related/split from this PR?

• Remove eval_always for stability_index. #91331
• Remove eval_always for privacy_access_levels. #91295
• Remove eval_always for check_private_in_public. #91296

[Zoxc]

eval_always is quite useful as a performance optimization on queries with a lot of dependencies (basically the entire crate) since we don't have to load, check, record and save those dependencies to disk. Pretty much any change to the crate would cause these to be executed again anyway, so there's not much to gain by removing it.

Also take care to not remove eval_always from queries which read untracked state or have side effects.