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

Cleanup: Eliminate ConstnessAnd #91354

Merged
merged 18 commits into from
Dec 2, 2021
Merged

Conversation

fee1-dead
Copy link
Member

This is almost a behaviour-free change and purely a refactoring. "almost" because we appear to be using the wrong ParamEnv somewhere already, and this is now exposed by failing a test using the unstable ~const feature.

We most definitely need to review all without_const and at some point should probably get rid of many of them by using TraitPredicate instead of TraitRef.

This is a continuation of #90274.

r? @oli-obk

cc @spastorino @ecstatic-morse

oli-obk and others added 14 commits November 29, 2021 21:19
This now causes a lot of queries to be executed twice, as reveal_all forces NotConst
…ension for it

This breaks a ~const test that will be fixed in a follow up commit of this PR
Nothing else makes sense, and there is no "danger" in doing so, as it only does something if there are const bounds, which are unstable. This used to happen implicitly via the inferctxt before, which was much more fragile.
@rust-highfive
Copy link
Collaborator

Some changes occurred in src/tools/clippy.

cc @rust-lang/clippy

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 rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 29, 2021
@fee1-dead
Copy link
Member Author

@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 Nov 29, 2021
@bors
Copy link
Contributor

bors commented Nov 29, 2021

⌛ Trying commit 8710a2e with merge 3bde14d5d8c5c093944947c2dfb90ea047842a0b...

@rust-log-analyzer

This comment has been minimized.

@oli-obk
Copy link
Contributor

oli-obk commented Nov 29, 2021

r? @spastorino

Can't review a PR made up of mostly my own commits

@rust-highfive rust-highfive assigned spastorino and unassigned oli-obk Nov 29, 2021
@oli-obk
Copy link
Contributor

oli-obk commented Nov 29, 2021

Tho... essentially @fee1-dead can review this, you only rebased it and fixed some tools. I guess r=oli-obk,fee1-dead if you are alright with my commits

@fee1-dead
Copy link
Member Author

I don't think I can do that, as this PR requires some work (fixing the assoc type ui test), also improving caching of queries that do not care about constness if the performance had gotten worse.

The ui test fix is quite simple. constness_for_typeck should return Constness::Const when the item is ImplItem::Fn | ImplItem::Const in a const impl. I had to remove the function from rustc_hir as the crate doesn't know what TyCtxt is and therefore cannot query the parent of a node easily.

@fee1-dead fee1-dead marked this pull request as draft November 29, 2021 14:41
@fee1-dead
Copy link
Member Author

@bors r=spastorino

@bors
Copy link
Contributor

bors commented Dec 2, 2021

📌 Commit 5ebc99e has been approved by spastorino

@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 Dec 2, 2021
@bors
Copy link
Contributor

bors commented Dec 2, 2021

⌛ Testing commit 5ebc99e with merge 18bb8c6...

@spastorino
Copy link
Member

I'm not sure how perf labelling works really but seems like we have perf-regression left from the first perf run, but the second shows mixed results.
Should we manually remove perf-regression label? if that's the case maybe it's nice if the perf tooling automatically does that in the second run.

cc @rust-lang/wg-compiler-performance

@fee1-dead
Copy link
Member Author

rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

perfbot always marks PRs with regressions as regressions. It automatically removes the label when there are no regressions, which is not the case for this PR.

@bors
Copy link
Contributor

bors commented Dec 2, 2021

☀️ Test successful - checks-actions
Approved by: spastorino
Pushing 18bb8c6 to master...

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (18bb8c6): comparison url.

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

  • Small improvement in instruction counts (up to -0.6% on full builds of deeply-nested)
  • Moderate regression in instruction counts (up to 1.1% on incr-patched: b9b3e592dd cherry picked builds of style-servo)

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

@oli-obk
Copy link
Contributor

oli-obk commented Dec 2, 2021

@sdroege
Copy link
Contributor

sdroege commented Dec 3, 2021

This seems to cause a regression, see #91489

@oli-obk
Copy link
Contributor

oli-obk commented Dec 3, 2021

Oof. That's quite odd. Let's revert this PR and go with a smaller subset of the changes first?

spastorino added a commit to spastorino/rust that referenced this pull request Dec 3, 2021
…rino"

This reverts commit 18bb8c6, reversing
changes made to d9baa36.
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 3, 2021
Revert "Auto merge of rust-lang#91354 - fee1-dead:const_env, r=spastorino"

This reverts commit 18bb8c6, reversing
changes made to d9baa36.

Reverts rust-lang#91354 in order to address rust-lang#91489. We would need to place this changes in a more granular way and would also be nice to address the small perf regression that was also introduced.

r? `@oli-obk`
cc `@fee1-dead`
@fee1-dead fee1-dead restored the const_env branch December 3, 2021 16:45
flip1995 pushed a commit to flip1995/rust that referenced this pull request Dec 17, 2021
Cleanup: Eliminate ConstnessAnd

This is almost a behaviour-free change and purely a refactoring. "almost" because we appear to be using the wrong ParamEnv somewhere already, and this is now exposed by failing a test using the unstable `~const` feature.

We most definitely need to review all `without_const` and at some point should probably get rid of many of them by using `TraitPredicate` instead of `TraitRef`.

This is a continuation of rust-lang#90274.

r? `@oli-obk`

cc `@spastorino` `@ecstatic-morse`
flip1995 pushed a commit to flip1995/rust that referenced this pull request Dec 17, 2021
…rino"

This reverts commit 18bb8c6, reversing
changes made to d9baa36.
@fee1-dead fee1-dead added the F-const_trait_impl `#![feature(const_trait_impl)]` label Jul 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-const_trait_impl `#![feature(const_trait_impl)]` 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.