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

derive various impls instead of hand-rolling them #101858

Merged
merged 6 commits into from Sep 15, 2022

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Sep 15, 2022

r? @lcnr

This may not have been what you asked for in 964b97e#r84051418 but I got carried away while following the compiler team meeting today.

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

rustbot commented Sep 15, 2022

This PR changes MIR

cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @celinval, @vakaras

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 15, 2022
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.

it's better than what i've asked for 😁

one more idea, with or without it

r=me

compiler/rustc_middle/src/mir/type_visitable.rs Outdated Show resolved Hide resolved
@oli-obk
Copy link
Contributor Author

oli-obk commented Sep 15, 2022

@bors r=lcnr p=1 bitrotty

@bors
Copy link
Contributor

bors commented Sep 15, 2022

📌 Commit c6fcb1c has been approved by lcnr

It is now in the queue for this repository.

@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 Sep 15, 2022
@bors
Copy link
Contributor

bors commented Sep 15, 2022

⌛ Testing commit c6fcb1c with merge df34db9...

@bors
Copy link
Contributor

bors commented Sep 15, 2022

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 15, 2022
@bors bors merged commit df34db9 into rust-lang:master Sep 15, 2022
@rustbot rustbot added this to the 1.65.0 milestone Sep 15, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (df34db9): 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)
1.2% [0.2%, 2.2%] 11
Regressions ❌
(secondary)
0.9% [0.3%, 2.1%] 18
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.7% [-0.9%, -0.5%] 6
All ❌✅ (primary) 1.2% [0.2%, 2.2%] 11

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)
3.4% [3.4%, 3.4%] 1
Regressions ❌
(secondary)
4.3% [3.5%, 5.1%] 2
Improvements ✅
(primary)
-3.6% [-3.6%, -3.6%] 1
Improvements ✅
(secondary)
-4.5% [-4.5%, -4.5%] 1
All ❌✅ (primary) -0.1% [-3.6%, 3.4%] 2

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)
- - 0
Regressions ❌
(secondary)
3.9% [2.7%, 5.1%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.6% [-2.6%, -2.6%] 1
All ❌✅ (primary) - - 0

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 Sep 15, 2022
@nnethercote
Copy link
Contributor

Looks like some genuine regressions.

@oli-obk: how do the derived version differ from the hand-written ones? Type folding is typically quite hot, so small differences could easily have perf effects.

@oli-obk oli-obk deleted the lift_derive branch September 16, 2022 06:27
@oli-obk
Copy link
Contributor Author

oli-obk commented Sep 16, 2022

I'll look into them. Most should be equivalent, I did check beforehand

@oli-obk
Copy link
Contributor Author

oli-obk commented Sep 16, 2022

From the following diff I'm guessing that this PR caused some additional obligations to be generated or just processed.

139,443,901  ???:<rustc_trait_selection::traits::fulfill::FulfillProcessor as rustc_data_structures::obligation_forest::ObligationProcessor>::process_obligation
 61,282,965  ???:<rustc_middle::ty::subst::SubstFolder as rustc_middle::ty::fold::TypeFolder>::fold_ty
-59,795,752  ???:<alloc::rc::Rc<rustc_middle::traits::ObligationCauseCode> as core::ops::drop::Drop>::drop
 49,456,068  ???:<rustc_middle::ty::subst::SubstFolder as rustc_middle::ty::fold::FallibleTypeFolder>::try_fold_ty
-47,967,367  ???:<rustc_middle::ty::Ty as rustc_middle::ty::fold::TypeSuperFoldable>::super_fold_with::<rustc_middle::ty::subst::SubstFolder>
 46,111,073  ???:<rustc_trait_selection::traits::select::SelectionContext>::evaluate_predicates_recursively::<alloc::vec::into_iter::IntoIter<rustc_infer::traits::Obligation<rustc_middle::ty::Predicate>>>
 44,568,113  ???:<rustc_infer::infer::combine::CombineFields>::instantiate
 33,973,909  ???:<rustc_trait_selection::traits::fulfill::FulfillmentContext as rustc_infer::traits::engine::TraitEngine>::register_predicate_obligation
-31,618,494  ???:<rustc_infer::infer::equate::Equate as rustc_middle::ty::relate::TypeRelation>::relate::<rustc_middle::ty::Ty>
 30,370,709  ???:<rustc_trait_selection::traits::wf::WfPredicates>::nominal_obligations_inner
 29,798,110  ???:rustc_trait_selection::traits::wf::predicate_obligations
 26,729,122  ???:<rustc_trait_selection::traits::select::SelectionContext>::assemble_candidates
 25,095,611  ???:<rustc_infer::infer::InferCtxt>::commit_if_ok::<rustc_infer::infer::InferOk<(alloc::vec::Vec<rustc_middle::ty::adjustment::Adjustment>, rustc_middle::ty::Ty)>, rustc_middle::ty::error::TypeError, <rustc_typeck::check::fn_ctxt::FnCtxt>::try_coerce::{closure
-24,951,977  ???:<rustc_trait_selection::traits::select::SelectionContext>::evaluate_predicate_recursively
-23,810,814  ???:<rustc_typeck::check::coercion::Coerce>::coerce
 23,372,325  ???:<rustc_middle::ty::ParamEnvAnd<rustc_middle::traits::query::type_op::ProvePredicate> as rustc_trait_selection::traits::query::type_op::TypeOp>::fully_perform
 20,236,839  ???:<alloc::vec::Vec<rustc_infer::traits::Obligation<rustc_middle::ty::Predicate>> as alloc::vec::spec_from_iter::SpecFromIter<rustc_infer::traits::Obligation<rustc_middle::ty::Predicate>, core::iter::adapters::filter::Filter<core::iter::adapters::map::Map<core::iter::adapters::zip::Zip<core::iter::adapters::zip::Zip<alloc::vec::into_iter::IntoIter<rustc_middle::ty::Predicate>, alloc::vec::into_iter::IntoIter<rustc_span::span_encoding::Span>>, core::iter::adapters::rev::Rev<alloc::vec::into_iter::IntoIter<rustc_span::def_id::DefId>>>, <rustc_trait_selection::traits::wf::WfPredicates>::nominal_obligations_inner::{closure
 17,899,560  ???:<rustc_trait_selection::traits::select::SelectionContext>::select

@bjorn3
Copy link
Member

bjorn3 commented Sep 20, 2022

@oli-obk This PR seems to have regressed the lift impl for Instance. cg_clif depends on it for it's jit mode.

thread 'rustc' panicked at 'called `Option::unwrap()` on a `None` value', src/driver/jit.rs:239:59
stack backtrace:
   0: rust_begin_unwind
             at /rustc/2019147c5642c08cdb9ad4cacd97dd1fa4ffa701/library/std/src/panicking.rs:584:5
   1: core::panicking::panic_fmt
             at /rustc/2019147c5642c08cdb9ad4cacd97dd1fa4ffa701/library/core/src/panicking.rs:142:14
   2: core::panicking::panic
             at /rustc/2019147c5642c08cdb9ad4cacd97dd1fa4ffa701/library/core/src/panicking.rs:48:5
   3: rustc_codegen_cranelift::driver::jit::jit_fn
   4: rustc_codegen_cranelift::driver::jit::run_jit

The Instance for which it fails is Instance { def: Item(WithOptConstParam { did: DefId(0:61 ~ mini_core_hello_world[7ee4]::main), const_param_did: None }), substs: [] }.

Comment on lines -264 to +275
match self {
Some(x) => tcx.lift(x).map(Some),
None => Some(None),
}
tcx.lift(self?).map(Some)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh... this is wrong now 🤦

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 23, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Sep 24, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 24, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 29, 2022
Fix perf regression from TypeVisitor changes

Regression occurred in rust-lang#101858 (comment)

Instead of just reverting, we only fixed part of the regression. The main regression was due to actually correctly visiting a type that contains types and consts and should therefor be visited. This is not actually observable (yet?), but we should still do it correctly instead of risking major bugs in the future.
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this pull request Apr 20, 2024
Fix perf regression from TypeVisitor changes

Regression occurred in rust-lang/rust#101858 (comment)

Instead of just reverting, we only fixed part of the regression. The main regression was due to actually correctly visiting a type that contains types and consts and should therefor be visited. This is not actually observable (yet?), but we should still do it correctly instead of risking major bugs in the future.
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this pull request Apr 27, 2024
Fix perf regression from TypeVisitor changes

Regression occurred in rust-lang/rust#101858 (comment)

Instead of just reverting, we only fixed part of the regression. The main regression was due to actually correctly visiting a type that contains types and consts and should therefor be visited. This is not actually observable (yet?), but we should still do it correctly instead of risking major bugs in the future.
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.

None yet

8 participants