resolve: Do not build expensive suggestions if they are not actually used by petrochenkov · Pull Request #95255 · rust-lang/rust

petrochenkov · 2022-03-24T00:14:24Z

And remove a bunch of (conditionally) unused parameters from path resolution functions.

This helps with performance issues in #94857, and should be helpful in general even without that.

rust-highfive · 2022-03-24T00:14:27Z

r? @michaelwoerister

(rust-highfive has picked a reviewer for you, use r? to override)

petrochenkov · 2022-03-24T00:14:50Z

@bors try @rust-timer queue

rust-timer · 2022-03-24T00:14:51Z

Awaiting bors try build completion.

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

bors · 2022-03-24T00:15:00Z

⌛ Trying commit 7e878bb75e2a91159cb6a3f2e4cbabf09fdbb9b1 with merge 5ef703f1b77179a2f29e24ea3e1775a23765725c...

klensy · 2022-03-24T00:30:10Z

This looks like unintended change

This is a drive-by fix for #94832 (comment) without which Rust doesn't build on my machine.
UPD: submitted as a separate PR in #95266.

bors · 2022-03-24T01:31:55Z

☀️ Try build successful - checks-actions
Build commit: 5ef703f1b77179a2f29e24ea3e1775a23765725c (5ef703f1b77179a2f29e24ea3e1775a23765725c)

rust-timer · 2022-03-24T01:31:57Z

Queued 5ef703f1b77179a2f29e24ea3e1775a23765725c with parent 9f4dc0b, future comparison URL.

rust-timer · 2022-03-24T03:00:37Z

Finished benchmarking commit (5ef703f1b77179a2f29e24ea3e1775a23765725c): comparison url.

Summary: This benchmark run shows 35 relevant improvements 🎉 to instruction counts.

Arithmetic mean of relevant improvements: -0.6%
Largest improvement in instruction counts: -2.2% on full builds of projection-caching doc

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.

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

petrochenkov · 2022-03-24T09:02:04Z

Maybe this enum should have some different name.
It should be short because it's used often.
There are two path resolution modes:
Speculative mode - we only return the result, but do not report errors or lints, and do not do recovery
Non-speculative mode - we report errors, do recovery when possible, also report lints, and mark path targets as used (the record_used naming comes from this). That's why need some extra data like a node id for linting or a span in this mode.
Maybe call it enum Finalize?
It's two letters shorter, not as specific as the old "record used" naming, and we use "finalize" elsewhere in resolve for similar functionality, like fn finalize_(imports,macro_resolutions).

Maybe something like
enumMode{Speculative,Finalize}
?
enum Finalize looks OK to me though.

I've renamed everything to Finalize/finalize, but kept it a single flat enum (with Finalize::No variant), otherwise it becomes too annoying to both construct and unwrap it.

michaelwoerister · 2022-03-24T12:35:14Z

Let me know when this is ready for review 👍

petrochenkov · 2022-03-24T13:17:52Z

@michaelwoerister
It is ready.

michaelwoerister · 2022-03-24T14:11:02Z

This looks like some great cleanup work. Lots of duplicate error messages being removed too.
I don't really know the code involved here -- but since this still passes all tests and the code diff looks reasonable to me, you can r=me. If you'd like to have more in-depth feedback, it's better to ask for a review from someone else.

…used Also remove a redundant parameter from `fn resolve_path(_with_ribs)`, `crate_lint: CrateLint` is a more detailed version of `record_used: bool` with `CrateLint::No` meaning `false` and anything else meaning `true`.

Do not construct or pass unused data

…nctions

And `crate_lint`/`record_used` to `finalize`

petrochenkov · 2022-03-24T23:23:16Z

If you'd like to have more in-depth feedback, it's better to ask for a review from someone else.

No, nothing too deep happens here, I mostly wanted some second opinion on naming.
@rustbot ready

michaelwoerister · 2022-03-25T09:33:07Z

OK, looks good to me.
@bors r+

bors · 2022-03-25T09:33:09Z

📌 Commit 1ad64a2 has been approved by michaelwoerister

bors · 2022-03-25T11:35:22Z

⌛ Testing commit 1ad64a2 with merge 903427b...

bors · 2022-03-25T14:16:07Z

☀️ Test successful - checks-actions
Approved by: michaelwoerister
Pushing 903427b to master...

rust-timer · 2022-03-25T19:13:43Z

Finished benchmarking commit (903427b): comparison url.

Summary: This benchmark run shows 33 relevant improvements 🎉 to instruction counts.

Arithmetic mean of relevant improvements: -0.6%
Largest improvement in instruction counts: -2.1% on full builds of projection-caching doc

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

@rustbot label: -perf-regression

resolve: Cleanup path resolution finalization Some cleanup after rust-lang#95255 and rust-lang#95405. r? `@cjgillot`

rust-highfive assigned michaelwoerister Mar 24, 2022

rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Mar 24, 2022

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

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 24, 2022

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

klensy reviewed Mar 24, 2022
View reviewed changes

rustbot added 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 Mar 24, 2022

petrochenkov commented Mar 24, 2022
View reviewed changes

petrochenkov mentioned this pull request Mar 24, 2022
Resolve documentation links in rustc and store the results in metadata #94857
Merged

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 Mar 24, 2022

petrochenkov added 4 commits March 25, 2022 02:03

resolve: Optimize path resolution for rustdoc
15a8b98
Do not construct or pass unused data

resolve: Stop passing unused spans and node ids to path resolution fu…
74d079d
…nctions

resolve: Rename CrateLint to Finalize
1ad64a2
And `crate_lint`/`record_used` to `finalize`

petrochenkov force-pushed the suggresolve branch from 7e878bb to 1ad64a2Compare March 24, 2022 23:19

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 Mar 24, 2022

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 25, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 25, 2022

bors merged commit 903427b into rust-lang:masterMar 25, 2022

rustbot added this to the 1.61.0 milestone Mar 25, 2022

petrochenkov mentioned this pull request Mar 25, 2022
rustbuild: Fix LLVM build with LLD on windows-gnu targets #95266
Closed

matthiaskrgr mentioned this pull request Mar 26, 2022
ice: "indeterminate path result in resolve_qpath" #95327
Closed

petrochenkov mentioned this pull request Apr 30, 2022
resolve: Cleanup path resolution finalization #96582
Merged

bors added a commit to rust-lang-ci/rust that referenced this pull request May 1, 2022
Auto merge ofrust-lang#96582- petrochenkov:finalize, r=cjgillot
a933de8
resolve: Cleanup path resolution finalization Some cleanup after rust-lang#95255 and rust-lang#95405. r? `@cjgillot`

lqd mentioned this pull request May 4, 2022
internal compiler error: min_const_generics resolve check didn't stop compilation #96654
Closed

petrochenkov deleted the suggresolve branch February 22, 2025 18:40

Uh oh!

Uh oh!

Conversation

petrochenkov commented Mar 24, 2022

Uh oh!

rust-highfive commented Mar 24, 2022

Uh oh!

petrochenkov commented Mar 24, 2022

Uh oh!

rust-timer commented Mar 24, 2022

Uh oh!

bors commented Mar 24, 2022

Uh oh!

klensyMar 24, 2022

Choose a reason for hiding this comment

Uh oh!

petrochenkovMar 24, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bors commented Mar 24, 2022

Uh oh!

rust-timer commented Mar 24, 2022

Uh oh!

rust-timer commented Mar 24, 2022

Uh oh!

petrochenkovMar 24, 2022

Choose a reason for hiding this comment

Uh oh!

michaelwoeristerMar 24, 2022

Choose a reason for hiding this comment

Uh oh!

petrochenkovMar 24, 2022

Choose a reason for hiding this comment

Uh oh!

michaelwoerister commented Mar 24, 2022

Uh oh!

petrochenkov commented Mar 24, 2022

Uh oh!

michaelwoerister commented Mar 24, 2022

Uh oh!

petrochenkov commented Mar 24, 2022

Uh oh!

michaelwoerister commented Mar 25, 2022

Uh oh!

bors commented Mar 25, 2022

Uh oh!

bors commented Mar 25, 2022

Uh oh!

bors commented Mar 25, 2022

Uh oh!

rust-timer commented Mar 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

petrochenkovMar 24, 2022•
edited
Loading