Skip to content

Clause::kind use unreachable_unchecked - #163639

Closed
lcnr wants to merge 1 commit into
rust-lang:mainfrom
lcnr:clause-kind-unreachable_unchecked
Closed

Clause::kind use unreachable_unchecked#163639
lcnr wants to merge 1 commit into
rust-lang:mainfrom
lcnr:clause-kind-unreachable_unchecked

Conversation

@lcnr

@lcnr lcnr commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

it's very hot. Let's see if it positively impacts perf

r? ghost

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 2, 2026
@lcnr

lcnr commented Oct 2, 2026

Copy link
Copy Markdown
Contributor Author

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 2, 2026
@rust-bors

This comment has been minimized.

Sign in to view
rust-bors Bot pushed a commit that referenced this pull request Oct 2, 2026
@bors
Auto merge of #163639 - lcnr:clause-kind-unreachable_unchecked, r=<try>
b7e7c71 
`Clause::kind` use `unreachable_unchecked`
lcnr
lcnr commented Oct 2, 2026
View reviewed changes
Comment thread compiler/rustc_middle/src/ty/predicate.rs
Comment on lines +176 to +177
// SAFETY: This function is incredibly hot. Let's make it optimize better.
_ => unsafe { std::hint::unreachable_unchecked() },

@lcnr lcnr Oct 2, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't understand safety comments apparently. This is not a safety comment

View changes since the review

@lqd lqd Oct 2, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

// HOTNESS: This function is incredibly hot.

@lcnr lcnr Oct 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Now the big problem, is that it's non-trivial to actually make this safe. THis field is public and the best commit is "while pub(super), nobody constructs a clause from something that's not a PredicateKind::Clause"

@rust-bors

rust-bors Bot commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

☀️ Try build successful (CI)
Build commit: b7e7c71 (b7e7c712bff8701692837010635d9f0bdae2d44f)
Base parent: dba8825 (dba8825fe50879b22129271fb865944e384f7cce)

@rust-timer

This comment has been minimized.

Sign in to view
@rust-timer

rust-timer commented Oct 2, 2026

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (b7e7c71): comparison URL.

Overall result: ❌✅ regressions and improvements - please read:

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

Next, please: If you can, justify the regressions found in this try perf run in writing along with @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

@bors rollup=never rustc-perf
@rustbot label: -S-waiting-on-perf +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.2% [0.2%, 0.3%] 5
Improvements ✅
(primary)		 -0.5% [-1.0%, -0.1%] 25
Improvements ✅
(secondary)		 -0.4% [-0.7%, -0.1%] 48
All ❌✅ (primary) -0.5% [-1.0%, -0.1%] 25

Max RSS (memory usage)

Results (primary 2.0%, secondary 2.4%)

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

mean range count
Regressions ❌
(primary)		 2.0% [2.0%, 2.0%] 1
Regressions ❌
(secondary)		 2.4% [1.9%, 2.9%] 2
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 2.0% [2.0%, 2.0%] 1

Cycles

Results (primary 1.9%, secondary -5.1%)

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

mean range count
Regressions ❌
(primary)		 1.9% [1.6%, 2.2%] 2
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -5.1% [-5.1%, -5.1%] 1
All ❌✅ (primary) 1.9% [1.6%, 2.2%] 2

Binary size

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

Bootstrap: 490.969s -> 489.986s (-0.20%)
Artifact size: 408.65 MiB -> 408.69 MiB (0.01%)

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Oct 2, 2026
@lcnr

lcnr commented Oct 2, 2026

Copy link
Copy Markdown
Contributor Author

don't have the capacity to properly make sure this is safe https://rust-lang.zulipchat.com/#narrow/channel/247081-t-compiler.2Fperformance/topic/needs.20help.3A.20small.20perf.20improvement.20with.20non-local.20reasoning/with/628660586

closing this PR

@lcnr lcnr closed this Oct 2, 2026
@rustbot rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 2, 2026
@panstromek panstromek mentioned this pull request Oct 2, 2026
Merged
rust-bors Bot pushed a commit that referenced this pull request Oct 4, 2026
@bors
Auto merge of #163657 - panstromek:next-solver-unreachable, r=lcnr
a639ea0 
outline unreachable! from Clause::kind



Safe alternative to #163639. `unreachable!()` expands to `panic("internal error: entered unreachable code")`, which we've seen before to be worse than plain argument-less call in other hot places.

see https://rust-lang.zulipchat.com/#narrow/channel/247081-t-compiler.2Fperformance/topic/needs.20help.3A.20small.20perf.20improvement.20with.20non-local.20reasoning/with/628660586
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lqd lqd lqd left review comments

Assignees

No one assigned

Labels

perf-regression Performance regression. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@lcnr @rust-timer @lqd @rustbot