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

Add TraitDef::find_map_relevant_impl #77754

Merged
merged 3 commits into from
Oct 10, 2020

Conversation

bugadani
Copy link
Contributor

@bugadani bugadani commented Oct 9, 2020

This PR adds a method to TraitDef. While for_each_relevant_impl covers the general use case, sometimes it's not necessary to scan through all the relevant implementations, so this PR introduces a new method, find_map_relevant_impl. I've also replaced the for_each_relevant_impl calls where possible.

I'm hoping for a tiny bit of efficiency gain here and there.

@rust-highfive
Copy link
Collaborator

Some changes occurred in intra-doc-links.

cc @jyn514

@rust-highfive
Copy link
Collaborator

r? @matthewjasper

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 9, 2020
if let Some(item) = self.associated_items(impl_did).in_definition_order().next() {
if validate(self, impl_did).is_ok() {
dtor_did = Some(item.def_id);
return Some(item.def_id);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically I'm changing the semantics here. The old code returned the last def_id, while the new one returns the first. Let me know if this is a problem.

@jyn514 jyn514 added A-traits Area: Trait system I-compiletime Issue: Problems and improvements with respect to compile times. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 9, 2020
@jyn514
Copy link
Member

jyn514 commented Oct 9, 2020

I think you could write for_each_relevant_impl in terms of this by calling f() in your closure and always returning None.

@bugadani
Copy link
Contributor Author

bugadani commented Oct 9, 2020

I'm not sure because for_each_relevant_impl takes FnMut, but correct me if there is a way.

@jyn514
Copy link
Member

jyn514 commented Oct 9, 2020

All FnMut closures also implement Fn.

@bugadani
Copy link
Contributor Author

bugadani commented Oct 9, 2020

@jyn514 Do you have something like this in mind?

    pub fn for_each_relevant_impl<F: FnMut(DefId)>(
        self,
        def_id: DefId,
        self_ty: Ty<'tcx>,
        mut f: F,
    ) {
        let _: Option<()> = self.find_map_relevant_impl(def_id, self_ty, |did| {
            f(did);
            None
        });
    }

The problem is, that the inner closure can't borrow f as mutable.

error[E0596]: cannot borrow `f` as mutable, as it is a captured variable in a `Fn` closure
   --> compiler/rustc_middle/src/ty/trait_def.rs:127:13
    |
127 |             f(did);
    |             ^ cannot borrow as mutable

@jyn514
Copy link
Member

jyn514 commented Oct 9, 2020

Hmm, could you change find_map to take FnMut instead? https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=ed6b2e4a35c4f3b3eecf170ba7140df1

@bugadani
Copy link
Contributor Author

bugadani commented Oct 9, 2020

I can, but none of the current usages need FnMut so I wanted to avoid that.

@jyn514
Copy link
Member

jyn514 commented Oct 9, 2020

I don't think there's any reason to avoid FnMut. APIs should be flexible unless there's a specific reason not to make them so. And indeed there is a usage that requires it: the for_each function that wants to reuse code.

Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but someone who works in this part of the compiler should make sure it has the same behavior

@jyn514
Copy link
Member

jyn514 commented Oct 9, 2020

@bors try @rust-timer queue

Let's see if this actually helps compile times.

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Oct 9, 2020

⌛ Trying commit 217d6f9 with merge 98ba3259700b5141f095cde6e8255e29402bf5cd...

@bors
Copy link
Contributor

bors commented Oct 9, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: 98ba3259700b5141f095cde6e8255e29402bf5cd (98ba3259700b5141f095cde6e8255e29402bf5cd)

@rust-timer
Copy link
Collaborator

Queued 98ba3259700b5141f095cde6e8255e29402bf5cd with parent 53a4c3b, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (98ba3259700b5141f095cde6e8255e29402bf5cd): 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

@jyn514
Copy link
Member

jyn514 commented Oct 9, 2020

Small improvement on deeply-nested-async, not much change elsewhere. Perf doesn't have test cases for associated items in intra-doc links so no telling what the effect was there.

@matthewjasper
Copy link
Contributor

@bors r+ rollup=maybe

@bors
Copy link
Contributor

bors commented Oct 10, 2020

📌 Commit 217d6f9 has been approved by matthewjasper

@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 Oct 10, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 10, 2020
Rollup of 10 pull requests

Successful merges:

 - rust-lang#77195 (Link to documentation-specific guidelines.)
 - rust-lang#77629 (Cleanup of `eat_while()` in lexer)
 - rust-lang#77709 (Link Vec leak doc to Box)
 - rust-lang#77738 (fix __rust_alloc_error_handler comment)
 - rust-lang#77748 (Dead code cleanup in windows-gnu std)
 - rust-lang#77754 (Add TraitDef::find_map_relevant_impl)
 - rust-lang#77766 (Clarify the debug-related values should take boolean)
 - rust-lang#77777 (doc: disambiguate stat in MetadataExt::as_raw_stat)
 - rust-lang#77782 (Fix typo in error code description)
 - rust-lang#77787 (Update `changelog-seen` in config.toml.example)

Failed merges:

r? `@ghost`
@bors bors merged commit 8752b43 into rust-lang:master Oct 10, 2020
@rustbot rustbot added this to the 1.49.0 milestone Oct 10, 2020
@bugadani bugadani deleted the find_map_relevant_impl branch October 10, 2020 21:21
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Oct 13, 2020
…-morse

Monomorphize `calculate_dtor` instead of using function pointers

Change `calculate_dtor` to avoid dynamic dispatching. This change allows the empty functions to be optimized away.

Based on the discussion in rust-lang#77754 (comment), the performance impact of this change was measured.

Perf run results: https://perf.rust-lang.org/compare.html?start=7bc5839e99411aad9061a632b62075d1346cbb3b&end=ffec759ae9bbc4d6d2235ff40ade6723a85bc7cc
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 13, 2020
…orse

Monomorphize `calculate_dtor` instead of using function pointers

Change `calculate_dtor` to avoid dynamic dispatching. This change allows the empty functions to be optimized away.

Based on the discussion in rust-lang#77754 (comment), the performance impact of this change was measured.

Perf run results: https://perf.rust-lang.org/compare.html?start=7bc5839e99411aad9061a632b62075d1346cbb3b&end=ffec759ae9bbc4d6d2235ff40ade6723a85bc7cc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-traits Area: Trait system I-compiletime Issue: Problems and improvements with respect to compile times. 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.

7 participants