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

performance: Speed up Method Completions By Taking Advantage of Orphan Rules #16555

Conversation

davidbarsky
Copy link
Contributor

(Continues #16498)

This PR speeds up method completions by doing two things without regressing analysis-stats1:

  • Filter candidate traits prior to calling iterate_path_candidates by relying on orphan rules (see below for a slightly more in-depth explanation). When generating completions on slog::Logger in oxidecomputer/omicron as a test, this PR halved my completion times—it's now 454ms cold and 281ms warm. Before this PR, it was 808ms cold and 579ms warm.
  • Inline some of the method candidate checks into is_valid_method_candidate and remove some unnecessary visibility checks. This was suggested by @Veykril in this comment.

We filter candidate traits by taking advantage of orphan rules. For additional details, I'll rely on @WaffleLapkin's explanation from Zulip:

A type A can only implements traits which

  1. Have a blanket implementation (impl<T> Trait for T {})
  2. Have implementation for A (impl Trait for A {})

Blanket implementation can only exist in Trait's crate. Implementation for A can only exist in A's or Trait's crate.

Big thanks to Waffle for its keen observation!


I think some additional improvements are possible:

  • for_trait_and_self_ty seemingly does not distinguish between &T, &mut T, or T, resulting in seemingly irrelevant traits like tokio::io::AsyncWrite being being included for, e.g., &slog::Logger. I don't know they're being considered due to the autoref/autoderef behavior, but I wonder if it'd make sense to filter by mutability earlier and not consider trait implementations that require &mut T when we only have a &T.
  • The method completions spend a lot of time in unification, and while there might be low-hanging fruit there, it might make more sense to wait for the new trait solver in rustc. I dunno.

Footnotes

  1. The filtering occurs outside of typechecking, after all.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 13, 2024
@davidbarsky davidbarsky force-pushed the david/speedup-completions-by-exploiting-orphan-rules branch from 26d794d to f3d507f Compare February 13, 2024 20:29
crates/hir-ty/src/method_resolution.rs Outdated Show resolved Hide resolved
crates/hir-ty/src/method_resolution.rs Outdated Show resolved Hide resolved
crates/hir/src/lib.rs Outdated Show resolved Hide resolved
crates/hir/src/lib.rs Outdated Show resolved Hide resolved
crates/ide-db/src/imports/import_assets.rs Outdated Show resolved Hide resolved
crates/ide-db/src/imports/import_assets.rs Outdated Show resolved Hide resolved
@Veykril Veykril 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 Feb 14, 2024
@davidbarsky davidbarsky force-pushed the david/speedup-completions-by-exploiting-orphan-rules branch 2 times, most recently from 4e8e251 to becdc6e Compare February 19, 2024 19:32
@davidbarsky
Copy link
Contributor Author

Right, okay: I addressed the review feedback, except the big one from Waffle. I'm pretty sure I've got a handle on checking fundamental types (I'll pilfer from it from the orphan implementation check), but I'm struggling to get a test with fundamental types working (see the changes in crates/ide-completion/src/tests/flyimport.rs). The expect side of things isn't the correct correct, but I can't seem to get the fixtures to be aware of a #[fundamental] Box—any tips?

@Veykril
Copy link
Member

Veykril commented Feb 19, 2024

Aren't you missing the fundamental check? The fixture seems correct to me

@davidbarsky
Copy link
Contributor Author

davidbarsky commented Feb 19, 2024

I am, yeah. I just want to write some tests that invoke fundamental trait impls and then ensure the impl works afterwards (in this PR, to be clear). i’m viewing those tests as a baseline.

@Veykril
Copy link
Member

Veykril commented Feb 20, 2024

Oh wait, your test fixture is not using the fundamental type in any impl, so how should it show anything?

@davidbarsky
Copy link
Contributor Author

Maybe a better of phrasing my question is "do I need to do anything special to introduce a new fundamental type in a fixture, or is the #[fundamental] annotation sufficient". Looking at some of the other tests, I'm inclined to say no.

@Veykril
Copy link
Member

Veykril commented Feb 20, 2024

Just using the attribute is enough yes, see the orphan check diagnostic fixtures

@davidbarsky davidbarsky force-pushed the david/speedup-completions-by-exploiting-orphan-rules branch from becdc6e to a8d0dbd Compare February 20, 2024 18:57
@davidbarsky
Copy link
Contributor Author

Alright, I updated the PR to include tests for impl ForeignTrait for Box<LocalStruct>, impl ForeignTrait for &LocalStruct, and impl Into<A> for (). The first two tests pass without issue (in that the filtering already considers fundamental types), but the third test seemingly does not work, despite .into() showing up in completions when I build a rust-analyzer binary with these exact changes.

Notably, I didn't need to make additional changes when filtering to consider fundamental types.

@davidbarsky davidbarsky force-pushed the david/speedup-completions-by-exploiting-orphan-rules branch 2 times, most recently from 9ba0c22 to b5a9609 Compare February 21, 2024 15:18
crates/ide-db/src/imports/import_assets.rs Outdated Show resolved Hide resolved
crates/ide-db/src/imports/import_assets.rs Outdated Show resolved Hide resolved
crates/hir/src/lib.rs Outdated Show resolved Hide resolved
@davidbarsky davidbarsky force-pushed the david/speedup-completions-by-exploiting-orphan-rules branch from b5a9609 to 3cb0f00 Compare February 23, 2024 16:31
crates/hir/src/lib.rs Outdated Show resolved Hide resolved
crates/hir/src/lib.rs Outdated Show resolved Hide resolved
crates/hir/src/lib.rs Outdated Show resolved Hide resolved
crates/hir/src/lib.rs Show resolved Hide resolved
@Veykril
Copy link
Member

Veykril commented Feb 23, 2024

Lgtm with the imports tidied up
@bors delegate+

@bors
Copy link
Collaborator

bors commented Feb 23, 2024

✌️ @davidbarsky, you can now approve this pull request!

If @Veykril told you to "r=me" after making some further change, please make that change, then do @bors r=@Veykril

@davidbarsky davidbarsky force-pushed the david/speedup-completions-by-exploiting-orphan-rules branch 2 times, most recently from 8613d1b to 053cdd1 Compare February 26, 2024 17:37
@davidbarsky davidbarsky force-pushed the david/speedup-completions-by-exploiting-orphan-rules branch from 053cdd1 to c246a93 Compare February 26, 2024 17:49
@davidbarsky
Copy link
Contributor Author

@bors r=@Veykril

@bors
Copy link
Collaborator

bors commented Feb 26, 2024

📌 Commit c246a93 has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Feb 26, 2024

⌛ Testing commit c246a93 with merge 5fead60...

@bors
Copy link
Collaborator

bors commented Feb 26, 2024

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing 5fead60 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants