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

Remove TypeckResults from InferCtxt #101632

Merged
merged 4 commits into from
Oct 7, 2022

Conversation

camsteffen
Copy link
Contributor

@camsteffen camsteffen commented Sep 9, 2022

InferCtxt currently has in_progress_typeck_results which is only used for some diagnostics during typeck. It adds a lifetime which propagates through a lot of code. This PR moves that field into a new helper struct TypeErrCtxt.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Sep 9, 2022
@rust-highfive
Copy link
Collaborator

r? @oli-obk

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

@rustbot
Copy link
Collaborator

rustbot commented Sep 9, 2022

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred in const_evaluatable.rs

cc @lcnr

Some changes occurred in need_type_info.rs

cc @lcnr

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 9, 2022
@bors
Copy link
Contributor

bors commented Sep 11, 2022

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

Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

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

really happy about this PR as it is something i've wanted to do myself for quite a while ❤️

some nits and further suggestions, but this looks pretty good already

compiler/rustc_infer/src/infer/mod.rs Outdated Show resolved Hide resolved
compiler/rustc_infer/src/infer/mod.rs Outdated Show resolved Hide resolved
compiler/rustc_infer/src/infer/combine.rs Show resolved Hide resolved
@lcnr
Copy link
Contributor

lcnr commented Sep 12, 2022

r? @lcnr

@rust-highfive rust-highfive assigned lcnr and unassigned oli-obk Sep 12, 2022
@lcnr lcnr 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 Sep 12, 2022
@camsteffen camsteffen force-pushed the refactor-infer-err branch 2 times, most recently from 4c43c71 to ce7f171 Compare September 20, 2022 13:42
@camsteffen
Copy link
Contributor Author

I don't know where to start with the failing rustdoc tests 😤

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Sep 20, 2022

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

@lcnr
Copy link
Contributor

lcnr commented Sep 21, 2022

🤔 i also can't tell why rustdoc is failing 😅 maybe look at the actually generated docs for the smallest failing test to check whether there are any user-visible changes? 🤔

@camsteffen
Copy link
Contributor Author

camsteffen commented Sep 29, 2022

Looks like a bit of waiting and doing nothing made the problem go away! nope

@camsteffen
Copy link
Contributor Author

@rustbot ready

@rustbot 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 Sep 29, 2022
@rust-log-analyzer

This comment has been minimized.

compiler/rustc_hir_analysis/src/check/fn_ctxt/mod.rs Outdated Show resolved Hide resolved
@@ -63,6 +66,37 @@ pub struct ImplCandidate<'tcx> {
}

pub trait InferCtxtExt<'tcx> {
Copy link
Contributor

Choose a reason for hiding this comment

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

should these be on TypeErrCtxt instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe? I was being conservative, only moving stuff to TypeErrCtxt if it needs TypeckResults.

src/tools/clippy/clippy_lints/src/future_not_send.rs Outdated Show resolved Hide resolved
@lcnr
Copy link
Contributor

lcnr commented Oct 3, 2022

small nits which we can fix in a followup pr

prone to merge conflicts:
@bors r+ p=5 rollup=never

@bors
Copy link
Contributor

bors commented Oct 3, 2022

📌 Commit d68bfb9012f16c29c52dea118439dc1c8d4354a5 has been approved by lcnr

It is now in the queue for this repository.

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Oct 7, 2022
@bors
Copy link
Contributor

bors commented Oct 7, 2022

⌛ Testing commit 283abbf with merge 43c22af...

@bors
Copy link
Contributor

bors commented Oct 7, 2022

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing 43c22af to master...

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (43c22af): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

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
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌
(primary)
0.7% [0.5%, 1.2%] 13
Regressions ❌
(secondary)
4.3% [3.2%, 5.7%] 6
Improvements ✅
(primary)
-0.3% [-0.6%, -0.2%] 19
Improvements ✅
(secondary)
-0.6% [-1.6%, -0.2%] 52
All ❌✅ (primary) 0.1% [-0.6%, 1.2%] 32

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
3.5% [2.1%, 4.5%] 4
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.6% [-2.9%, -2.3%] 2
All ❌✅ (primary) - - 0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌
(primary)
2.4% [2.4%, 2.4%] 1
Regressions ❌
(secondary)
3.4% [3.4%, 3.4%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-3.4% [-3.4%, -3.4%] 1
All ❌✅ (primary) 2.4% [2.4%, 2.4%] 1

Footnotes

  1. the arithmetic mean of the percent change 2 3

  2. number of relevant changes 2 3

@rustbot rustbot added the perf-regression Performance regression. label Oct 7, 2022
@lcnr
Copy link
Contributor

lcnr commented Oct 10, 2022

oh wow, should have definitely run perf before approving this 😅

i guess this resulted in a lot of small inlining/optimization changes, might be worth to check out the flamegraph comparison of a few regressions to check if we can maybe fix them by putting #[inline] somewhere.

@rylev
Copy link
Member

rylev commented Oct 11, 2022

@lcnr @camsteffen I ran callgrind diff on one of the benchmarks. Looks specialization_graph::Children::insert is getting called way more.

--------------------------------------------------------------------------------
Ir           file:function
--------------------------------------------------------------------------------
 94,311,966  ???:rustc_trait_selection::traits::coherence::overlapping_impls::<<rustc_middle::traits::specialization_graph::Children as rustc_trait_selection::traits::specialize::specialization_graph::ChildrenExt>::insert::{closure
-66,019,056  ???:<rustc_middle::traits::specialization_graph::Children as rustc_trait_selection::traits::specialize::specialization_graph::ChildrenExt>::insert
  7,957,920  ???:__rust_probestack
 -7,904,085  ???:<rustc_infer::infer::InferCtxtBuilder>::enter::<core::result::Result<alloc::vec::Vec<rustc_middle::ty::Predicate>, rustc_errors::ErrorGuaranteed>, rustc_trait_selection::traits::do_normalize_predicates::{closure
  6,887,397  ???:rustc_trait_selection::traits::do_normalize_predicates
  4,716,702  ???:<rustc_infer::infer::InferCtxt>::probe::<(), <rustc_trait_selection::traits::select::SelectionContext>::assemble_candidates_from_impls::{closure
 -4,586,815  ???:<rustc_trait_selection::traits::select::SelectionContext>::match_impl
  4,052,492  ???:<rustc_infer::infer::InferCtxtBuilder>::build
 -3,625,123  /build/glibc-eX1tMB/glibc-2.31/string/../sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S:__memcpy_avx_unaligned_erms

@camsteffen
Copy link
Contributor Author

Thanks @rylev. I think it might be traits::overlapping_impls that needs inline? I see that function in the callgrind diff, and the changes to that function might have made it not inlined anymore.

@rylev
Copy link
Member

rylev commented Oct 11, 2022

@camsteffen the symbol is a bit hard to parse. It seems like it's not the function but possibly a closure inside of that function? Might require some experimentation.

@camsteffen
Copy link
Contributor Author

Probably this closure, and if overlapping_impls is inlined, then the closure would probably be inlined as well?

@camsteffen
Copy link
Contributor Author

bitmaps perf addressed in #102931. Looks like deeply-nested-multi could use another fix.

@nnethercote
Copy link
Contributor

deeply-nested-multi is less important, plus it has been noisy lately, so don't spend too much time on it.

@camsteffen
Copy link
Contributor Author

Thanks @nnethercote. I'll gladly ignore that.

bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 16, 2022
… r=cjgillot

Make `overlapping_impls` not generic

Trying to win back perf from rust-lang#101632.
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. 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.