Skip to content

Conversation

b-naber
Copy link
Contributor

@b-naber b-naber commented May 13, 2022

Compartmentalising #96591 as much as possible.

r? @oli-obk

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

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

@rust-highfive
Copy link
Contributor

⚠️ Warning ⚠️

  • These commits modify submodules.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 13, 2022
@rust-log-analyzer

This comment has been minimized.

@b-naber b-naber force-pushed the transition-to-valtrees-pt1 branch from 2800a34 to 2a1f40a Compare May 13, 2022 15:41
@b-naber
Copy link
Contributor Author

b-naber commented May 13, 2022

I'm not sure why I get the changes in the submodules. Something must have gone wrong during a rebase, I had some conflicts in submodules and used get reset master submodule_path to resolve those, which might have been the wrong way to fix those. Does anybody know how to fix this?

@rust-log-analyzer

This comment has been minimized.

@RalfJung
Copy link
Member

Figure out in which commit the submodule change is, and then use rebase --interactive to edit that commit and get rid of the submodule change.

@b-naber b-naber force-pushed the transition-to-valtrees-pt1 branch from 2a1f40a to 1ae577e Compare May 13, 2022 16:58
@b-naber
Copy link
Contributor Author

b-naber commented May 13, 2022

Updated the submodule and squashed with the commit that previously had the submodule change. Still have a change in the submodule's Cargo.lock file according to the diff though.

Copy link
Contributor

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

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

I went afk mid review and forgot to send it. Will continue on Monday.

@rust-log-analyzer

This comment has been minimized.

@b-naber b-naber force-pushed the transition-to-valtrees-pt1 branch from 1ae577e to 26db3d7 Compare May 13, 2022 17:56
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Collaborator

bors commented May 13, 2022

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

@oli-obk
Copy link
Contributor

oli-obk commented May 16, 2022

needs a rebase.

Also you got a submodule update somewhere in there, please make sure to remove it during the rebase

@oli-obk
Copy link
Contributor

oli-obk commented May 16, 2022

r=me after that

@b-naber b-naber force-pushed the transition-to-valtrees-pt1 branch from 26db3d7 to 0d1ab44 Compare May 16, 2022 13:28
@b-naber b-naber force-pushed the transition-to-valtrees-pt1 branch from 0d1ab44 to 2b603fe Compare May 16, 2022 13:46
@oli-obk
Copy link
Contributor

oli-obk commented May 16, 2022

Please squash the commits so there's no back and forth with the submodule

@b-naber b-naber force-pushed the transition-to-valtrees-pt1 branch from 2b603fe to 96b36d6 Compare May 16, 2022 13:58
@b-naber
Copy link
Contributor Author

b-naber commented May 17, 2022

@oli-obk Can you take another look, please?

@oli-obk
Copy link
Contributor

oli-obk commented May 18, 2022

@bors r+

@bors
Copy link
Collaborator

bors commented May 18, 2022

📌 Commit 96b36d6 has been approved by oli-obk

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 18, 2022
@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 May 18, 2022
@bors
Copy link
Collaborator

bors commented May 18, 2022

⌛ Testing commit 96b36d6 with merge cd282d7...

@bors
Copy link
Collaborator

bors commented May 18, 2022

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 18, 2022
@bors bors merged commit cd282d7 into rust-lang:master May 18, 2022
@rustbot rustbot added this to the 1.63.0 milestone May 18, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (cd282d7): comparison url.

Summary:

  • Primary benchmarks: 😿 relevant regressions found
  • Secondary benchmarks: 😿 relevant regressions found
Regressions 😿
(primary)
Regressions 😿
(secondary)
Improvements 🎉
(primary)
Improvements 🎉
(secondary)
All 😿 🎉
(primary)
count1 4 5 0 0 4
mean2 1.0% 0.1% N/A N/A 1.0%
max 1.1% 0.1% N/A N/A 1.1%

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

Footnotes

  1. number of relevant changes

  2. the arithmetic mean of the percent change

@rustbot rustbot added the perf-regression Performance regression. label May 19, 2022
@oli-obk
Copy link
Contributor

oli-obk commented May 19, 2022

cachegrind on the unicode-normalization regression:

-97,176,447  ???:rustc_mir_build::thir::pattern::compare_const_vals
 72,480,660  ???:<rustc_middle::mir::interpret::value::ConstValue>::try_to_bits
 54,411,647  ???:<rustc_mir_build::build::Builder>::match_simplified_candidates
-37,703,135  ???:<rustc_mir_build::build::Builder>::test_candidates_with_or
 20,818,165  ???:<rustc_middle::mir::ConstantKind>::ty
 18,643,459  ???:<rustc_mir_build::build::Builder>::values_not_contained_in_range
  9,696,591  ???:<rustc_mir_build::build::Builder>::simplify_candidate
 -4,265,882  ???:rustc_mir_build::build::expr::as_constant::lit_to_mir_constant
  2,721,211  ???:<rustc_mir_build::build::Builder>::as_constant

could probably benefit from some targeted #[inline(always)] instructions (try_to_bits and ConstantKind::ty maybe?)

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.

9 participants