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

extend simplify_type #86986

Merged
merged 5 commits into from
Dec 16, 2021
Merged

extend simplify_type #86986

merged 5 commits into from
Dec 16, 2021

Conversation

lcnr
Copy link
Contributor

@lcnr lcnr commented Jul 8, 2021

might cause a slight perf inprovement and imo more accurately represents what types there are.

considering that I was going to use this in #85048 it seems like we might need this in the future anyways 🤷

@rust-highfive
Copy link
Collaborator

r? @LeSeulArtichaut

(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 Jul 8, 2021
@lcnr
Copy link
Contributor Author

lcnr commented Jul 8, 2021

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

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

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 8, 2021
@bors
Copy link
Contributor

bors commented Jul 8, 2021

⌛ Trying commit 45273f8187cea4bdd13b5381f00b5fbce354f22b with merge 42b0078198bdfc32d8c25b4bac110afe8785e596...

@LeSeulArtichaut
Copy link
Contributor

LeSeulArtichaut commented Jul 8, 2021

I'm not the right reviewer. Looks like you wanted to assign #85048 to @nikomatsakis, should I r? him?

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jul 8, 2021

☀️ Try build successful - checks-actions
Build commit: 42b0078198bdfc32d8c25b4bac110afe8785e596 (42b0078198bdfc32d8c25b4bac110afe8785e596)

@rust-timer
Copy link
Collaborator

Queued 42b0078198bdfc32d8c25b4bac110afe8785e596 with parent aa65b08, future comparison URL.

@lcnr
Copy link
Contributor Author

lcnr commented Jul 8, 2021

r? @nikomatsakis

@rust-log-analyzer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (42b0078198bdfc32d8c25b4bac110afe8785e596): comparison url.

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

  • Large regression in instruction counts (up to 9.2% on incr-patched: sparse set builds of regex-check)
  • Moderate improvement in instruction counts (up to -2.4% on full builds of futures-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

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jul 9, 2021
@lcnr lcnr force-pushed the simplify_type branch from 898f2b6 to de2d60b Compare July 9, 2021 12:53
@rust-log-analyzer

This comment has been minimized.

@lcnr
Copy link
Contributor Author

lcnr commented Jul 9, 2021

while CI is failing here, I am not able to reproduce this locally 🤔 has someone an idea why that might be?

@nikomatsakis
Copy link
Contributor

@lcnr I'm rather surprised by the 9% regression! It seems like these changes make the "fast reject" more precise without introducing significantly more work, and that seems like it can only be helpful. Do you have any idea what I'm missing here?

@lcnr lcnr force-pushed the simplify_type branch 2 times, most recently from a40cabd to aa1c088 Compare July 12, 2021 19:03
@lcnr
Copy link
Contributor Author

lcnr commented Jul 12, 2021

@bors try @rust-timer queue

@oli-obk found one case where we end up doing more work. Before that I wasn't sure why we had to rerun more queries with this PR, but tbh i didn't bother as I considered the impact to be negligible. Let's see if this fixes that

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

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

Comment on lines +80 to +85
/// ² FIXME(@lcnr): this seems like it can actually end up being unsound with the way it's used during
/// candidate selection. We do not consider non blanket impls for `<_ as Trait>::Assoc` even
/// though `_` can be inferred to a concrete type later at which point a concrete impl
/// could actually apply. After experimenting for about an hour I wasn't able to cause any issues
/// this way so I am not going to change this until we actually find an issue as I am really
/// interesting in getting an actual test for this.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nikomatsakis does this comment make sense to you? I hoped to trigger it by using something like the following, but wasn't able to make much progress. We either normalize the projection because it is already concrete enough or fail for some different reason. Not sure if this is actually exploitable if it even is an issue.

trait Id {
    type Assoc;
}

impl<T> Id for T {
    type Assoc = Self;
}

trait Bar<T> {
    type Assoc;
    fn bar(self) -> Self::Assoc; 
}

impl Bar<u32> for i32 {
    type Assoc = i32;
    fn bar(self) -> Self::Assoc {
        1
    }
}

fn main() {
    let mut x = <<_ as Id>::Assoc>::default();
    x = <_ as Bar<u32>>::bar(x);
}

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Dec 2, 2021

☔ The latest upstream changes (presumably #91354) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Dec 13, 2021

☔ The latest upstream changes (presumably #91549) made this pull request unmergeable. Please resolve the merge conflicts.

@rust-log-analyzer

This comment has been minimized.

@oli-obk
Copy link
Contributor

oli-obk commented Dec 15, 2021

@bors r=nikomatsakis,oli-obk

@bors
Copy link
Contributor

bors commented Dec 15, 2021

📌 Commit 00cbacb has been approved by nikomatsakis,oli-obk

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 15, 2021
@bors
Copy link
Contributor

bors commented Dec 15, 2021

⌛ Testing commit 00cbacb with merge 69ac533...

@bors
Copy link
Contributor

bors commented Dec 16, 2021

☀️ Test successful - checks-actions
Approved by: nikomatsakis,oli-obk
Pushing 69ac533 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 16, 2021
@bors bors merged commit 69ac533 into rust-lang:master Dec 16, 2021
@rustbot rustbot added this to the 1.59.0 milestone Dec 16, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (69ac533): comparison url.

Summary: This change led to very large relevant mixed results 🤷 in compiler performance.

  • Large improvement in instruction counts (up to -4.7% on full builds of hyper-2)
  • Very large regression in instruction counts (up to 8.4% on incr-patched: sparse set builds of regex)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-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 open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

@lcnr
Copy link
Contributor Author

lcnr commented Dec 16, 2021

the only regressions are small changes to some incr-patched tests, so this does seem like a strict perf improvement to me

@lcnr lcnr deleted the simplify_type branch December 16, 2021 10:20
@oli-obk
Copy link
Contributor

oli-obk commented Dec 16, 2021

@rustbot label: +perf-regression-triaged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. 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.