Skip to content

Revert "trait_selection: Keep type-op region constraints in borrowck" - #162523

Merged
rust-bors[bot] merged 1 commit into
rust-lang:mainfrom
Dnreikronos:trait_selection/revert_preserve_type_op_constraints
Sep 10, 2026
Merged

Revert "trait_selection: Keep type-op region constraints in borrowck"#162523
rust-bors[bot] merged 1 commit into
rust-lang:mainfrom
Dnreikronos:trait_selection/revert_preserve_type_op_constraints

Conversation

@Dnreikronos

@Dnreikronos Dnreikronos commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

This reverts #161423.

Boxy pointed out the problem after it landed: the code I touched there runs a lot, and I made it do the new region constraint work every time, even when assumptions-on-binders is off and the constraint is just true. So in the common case you end up paying for building and carrying around something nobody ever looks at. Fair point, and honestly I should have seen it myself while writing it. I was thinking about getting the correctness right and not really about how often those functions get called.

The plan is to revert now and then re-land the same thing with the new logic behind checks for whether AoB is actually on, so we can get a perf run and look at real numbers instead of guessing.

There is nothing clever in this PR. It is just git revert -m 1 on the rollup merge, so the diff is the exact opposite of what went in. It applied with no conflicts on current main and x check compiler is fine, which I wanted to confirm because #162238 landed after this one and touches some of the same files.

r? @BoxyUwU

@rustbot

rustbot commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels Sep 9, 2026
@rustbot

rustbot commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

BoxyUwU is currently at their maximum review capacity.
They may take a while to respond.

@rustbot

This comment has been minimized.

This reverts commit 9318843, reversing
changes made to 37719fa.

The new region constraint handling runs on hot codepaths even when
assumptions-on-binders is off and the constraint is just `true`, so we
pay for it in the common case. Reverting to re-land it later with the new
logic behind checks for whether AoB is enabled, so it can get a perf run.
@Dnreikronos
Dnreikronos force-pushed the trait_selection/revert_preserve_type_op_constraints branch from 7ed7954 to 393c127 Compare September 9, 2026 11:03
@BoxyUwU

BoxyUwU commented Sep 9, 2026

Copy link
Copy Markdown
Member

@bors r+ rollup=never

thanks for posting this, sorry for not catching that it might be perf sensitive

@rust-bors

rust-bors Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

📌 Commit 393c127 has been approved by BoxyUwU

It is now in the queue for this repository.

@rust-bors rust-bors Bot 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 9, 2026
@rust-bors

This comment has been minimized.

rust-bors Bot pushed a commit that referenced this pull request Sep 9, 2026
…ype_op_constraints, r=BoxyUwU

Revert "trait_selection: Keep type-op region constraints in borrowck"

This reverts #161423.

Boxy pointed out the problem after it landed: the code I touched there runs a lot, and I made it do the new region constraint work every time, even when assumptions-on-binders is off and the constraint is just `true`. So in the common case you end up paying for building and carrying around something nobody ever looks at. Fair point, and honestly I should have seen it myself while writing it. I was thinking about getting the correctness right and not really about how often those functions get called.

The plan is to revert now and then re-land the same thing with the new logic behind checks for whether AoB is actually on, so we can get a perf run and look at real numbers instead of guessing.

There is nothing clever in this PR. It is just `git revert -m 1` on the rollup merge, so the diff is the exact opposite of what went in. It applied with no conflicts on current main and `x check compiler` is fine, which I wanted to confirm because #162238 landed after this one and touches some of the same files.

r? @BoxyUwU
@rust-bors rust-bors Bot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 9, 2026
@rust-bors

rust-bors Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

💔 Test for c8976fe failed: CI. Failed job:

@JonathanBrouwer

Copy link
Copy Markdown
Member

@bors retry

@rust-bors rust-bors Bot 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 9, 2026
@rust-log-analyzer

Copy link
Copy Markdown
Collaborator

A job failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@JonathanBrouwer

Copy link
Copy Markdown
Member

@bors retry

@rust-bors

rust-bors Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

❗ You can only retry pull requests that are approved and have a previously failed auto build.

Hint: If you wanted to retry pull request CI instead, push the latest commit again, or close and then reopen this PR.

@rust-bors

This comment has been minimized.

@rust-bors rust-bors Bot added merged-by-bors This PR was explicitly merged by bors. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 10, 2026
@rust-bors

rust-bors Bot commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

☀️ Test successful - CI
Approved by: BoxyUwU
Duration: 3h 10m 37s
Pushing 315ecf4 to main...

@rust-bors
rust-bors Bot merged commit 315ecf4 into rust-lang:main Sep 10, 2026
14 checks passed
@rustbot rustbot added this to the 1.100.0 milestone Sep 10, 2026
@github-actions

Copy link
Copy Markdown
Contributor
What is this? This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing a36d05e (parent) -> 315ecf4 (this PR)

Test differences

Show 8 test diffs

Stage 0

  • infer::solver_region_constraints::tests::true_constraint_keeps_query_response_empty: pass -> [missing] (J1)

Stage 1

  • infer::solver_region_constraints::tests::true_constraint_keeps_query_response_empty: pass -> [missing] (J0)

Additionally, 6 doctest diffs were found. These are ignored, as they are noisy.

Job group index

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard 315ecf4a939def16631c2b25c3782ad67fc22160 --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

  1. test-x86_64-gnu-miri: 47m 31s -> 1h 23m (+76.1%)
  2. test-armhf-gnu: 1h -> 1h 38m (+62.4%)
  3. test-x86_64-gnu-stdlib-semver-check: 11m 30s -> 17m 45s (+54.3%)
  4. dist-ohos-aarch64: 1h 3m -> 1h 23m (+32.2%)
  5. test-i686-gnu-2: 1h 44m -> 1h 10m (-31.9%)
  6. test-x86_64-gnu-parallel-frontend: 1h 31m -> 1h 59m (+31.4%)
  7. test-i686-gnu-1: 2h 31m -> 1h 48m (-28.5%)
  8. test-x86_64-gnu-gcc-core-tests: 12m 39s -> 16m 8s (+27.5%)
  9. dist-x86_64-mingw: 2h 8m -> 2h 42m (+26.3%)
  10. dist-arm-linux-gnueabi: 1h 36m -> 1h 11m (-25.3%)
How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (315ecf4): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-2.3% [-6.2%, -0.2%] 167
Improvements ✅
(secondary)
-2.3% [-6.3%, -0.1%] 181
All ❌✅ (primary) -2.3% [-6.2%, -0.2%] 167

Max RSS (memory usage)

Results (primary -0.8%, secondary -1.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
0.4% [0.4%, 0.4%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.9% [-1.5%, -0.5%] 11
Improvements ✅
(secondary)
-1.1% [-2.1%, -0.9%] 5
All ❌✅ (primary) -0.8% [-1.5%, 0.4%] 12

Cycles

Results (primary -2.5%, secondary -3.5%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
1.5% [0.5%, 3.4%] 3
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-2.7% [-4.3%, -0.5%] 55
Improvements ✅
(secondary)
-3.5% [-5.2%, -2.1%] 41
All ❌✅ (primary) -2.5% [-4.3%, 3.4%] 58

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 481.833s -> 478.997s (-0.59%)
Artifact size: 403.27 MiB -> 403.72 MiB (0.11%)

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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants