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

interpret: make some large types not Copy #99309

Merged
merged 2 commits into from
Jul 19, 2022

Conversation

RalfJung
Copy link
Member

Also remove some unused trait impls (mostly HashStable).

This didn't find any unnecessary copies that I managed to avoid, but it might still be better to require explicit clone for these types? Not sure.

r? @oli-obk

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

rustbot commented Jul 16, 2022

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

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 Jul 16, 2022
@oli-obk
Copy link
Contributor

oli-obk commented Jul 16, 2022

@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 16, 2022
@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

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

@oli-obk
Copy link
Contributor

oli-obk commented Jul 16, 2022

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

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

@bors
Copy link
Contributor

bors commented Jul 16, 2022

⌛ Trying commit d90185827ab6d18956cf668fed302f01ee721eed with merge e43644679e78da80484751bc69eb266a1e5ba8e3...

@bors
Copy link
Contributor

bors commented Jul 16, 2022

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

@rust-timer
Copy link
Collaborator

Queued e43644679e78da80484751bc69eb266a1e5ba8e3 with parent e6c43cf, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (e43644679e78da80484751bc69eb266a1e5ba8e3): comparison url.

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results
  • Primary benchmarks: 🎉 relevant improvement found
  • Secondary benchmarks: 😿 relevant regressions found
mean1 max count2
Regressions 😿
(primary)
N/A N/A 0
Regressions 😿
(secondary)
4.0% 5.3% 2
Improvements 🎉
(primary)
-2.3% -2.3% 1
Improvements 🎉
(secondary)
N/A N/A 0
All 😿🎉 (primary) -2.3% -2.3% 1

Cycles

This benchmark run did not return any relevant results for this metric.

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 may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

Footnotes

  1. the arithmetic mean of the percent change

  2. number of relevant changes

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 16, 2022
let value =
if i == index { *elem } else { self.mplace_index(&input, i)?.into() };
let value = if i == index {
elem.clone()
Copy link
Member

Choose a reason for hiding this comment

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

it's not going to matter much in the grand scheme of things by saving one (1) clone per call to this intrinsic, but this one should be easy to remove by calling copy_op in the branches ^^

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I considered this and decided, for the reasons you mentioned, that reducing code duplication is more important.

@oli-obk
Copy link
Contributor

oli-obk commented Jul 18, 2022

r=me after a rebase

@RalfJung
Copy link
Member Author

Oh, bors didn't mention the conflict, strange.

@bors r=oli-obk

@bors
Copy link
Contributor

bors commented Jul 18, 2022

📌 Commit 213a25d has been approved by oli-obk

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 Jul 18, 2022
@bors
Copy link
Contributor

bors commented Jul 19, 2022

⌛ Testing commit 213a25d with merge 29c5a02...

@bors
Copy link
Contributor

bors commented Jul 19, 2022

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 29c5a02 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 19, 2022
@bors bors merged commit 29c5a02 into rust-lang:master Jul 19, 2022
@rustbot rustbot added this to the 1.64.0 milestone Jul 19, 2022
@rust-highfive
Copy link
Collaborator

📣 Toolstate changed by #99309!

Tested on commit 29c5a02.
Direct link to PR: #99309

💔 miri on windows: test-fail → build-fail (cc @RalfJung @oli-obk).
💔 miri on linux: test-fail → build-fail (cc @RalfJung @oli-obk).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Jul 19, 2022
Tested on commit rust-lang/rust@29c5a02.
Direct link to PR: <rust-lang/rust#99309>

💔 miri on windows: test-fail → build-fail (cc @RalfJung @oli-obk).
💔 miri on linux: test-fail → build-fail (cc @RalfJung @oli-obk).
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (29c5a02): comparison url.

Instruction count

  • Primary benchmarks: 🎉 relevant improvement found
  • Secondary benchmarks: 😿 relevant regressions found
mean1 max count2
Regressions 😿
(primary)
N/A N/A 0
Regressions 😿
(secondary)
1.5% 1.7% 3
Improvements 🎉
(primary)
-0.8% -0.8% 1
Improvements 🎉
(secondary)
N/A N/A 0
All 😿🎉 (primary) -0.8% -0.8% 1

Max RSS (memory usage)

Results
  • Primary benchmarks: 🎉 relevant improvement found
  • Secondary benchmarks: mixed results
mean1 max count2
Regressions 😿
(primary)
N/A N/A 0
Regressions 😿
(secondary)
2.0% 2.0% 1
Improvements 🎉
(primary)
-3.1% -3.1% 1
Improvements 🎉
(secondary)
-3.0% -4.0% 4
All 😿🎉 (primary) -3.1% -3.1% 1

Cycles

This benchmark run did not return any relevant results for this metric.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

  1. the arithmetic mean of the percent change 2

  2. number of relevant changes 2

@oli-obk
Copy link
Contributor

oli-obk commented Jul 20, 2022

weird, the previous perf run was clean. Inlining threshold noise I guess?

@oli-obk
Copy link
Contributor

oli-obk commented Jul 20, 2022

268,061,669  ???:<rustc_const_eval::interpret::eval_context::InterpCx<rustc_const_eval::const_eval::machine::CompileTimeInterpreter>>::run
 -7,078,188  ???:<rustc_middle::ty::subst::SubstFolder as rustc_middle::ty::fold::TypeFolder>::fold_const
 -5,636,093  ???:<rustc_const_eval::interpret::eval_context::InterpCx<rustc_const_eval::const_eval::machine::CompileTimeInterpreter>>::eval_place
  5,453,140  ???:<rustc_middle::ty::instance::Instance>::resolve_opt_const_arg
 -3,829,325  ???:<rustc_const_eval::interpret::eval_context::InterpCx<rustc_const_eval::const_eval::machine::CompileTimeInterpreter>>::eval_fn_call
  3,704,373  ???:<rustc_const_eval::interpret::eval_context::InterpCx<rustc_const_eval::const_eval::machine::CompileTimeInterpreter>>::push_stack_frame
 -3,407,626  ???:<rustc_middle::ty::normalize_erasing_regions::TryNormalizeAfterErasingRegionsFolder as rustc_middle::ty::fold::FallibleTypeFolder>::try_fold_mir_const
 -2,175,874  ???:<rustc_const_eval::interpret::eval_context::InterpCx<rustc_const_eval::const_eval::machine::CompileTimeInterpreter>>::eval_operand
   -367,016  ???:<rustc_const_eval::interpret::eval_context::InterpCx<rustc_const_eval::const_eval::machine::CompileTimeInterpreter>>::mir_const_to_op
    277,583  ???:rustc_trait_selection::traits::project::normalize::<rustc_middle::ty::sty::TraitRef>
   -277,443  ???:llvm::FPPassManager::runOnFunction(llvm::Function&)
   -262,321  ???:<rustc_target::abi::TyAndLayout<rustc_middle::ty::Ty>>::field::<rustc_const_eval::interpret::eval_context::InterpCx<rustc_const_eval::const_eval::machine::CompileTimeInterpreter>>

@RalfJung RalfJung deleted the no-large-copies branch July 22, 2022 13:08
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. 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