Always inline check in `assert_unsafe_precondition` with cfg(debug_assertions) #121196

Noratrieb · 2024-02-16T18:53:57Z

The current complexities in assert_unsafe_precondition are delicately balancing several concerns, among them compile times for the cases where there are no debug assertions. This comes at a large runtime cost when the assertions are enabled, making the debug assertion compiler a lot slower, which is very annoying.

To avoid this, we always inline the check when building with debug assertions.

Numbers (compiling stage1 library after touching core):

master: 80s
just adding #[inline(always)] to the cfg(bootstrap) debug_assertions (equivalent to a bootstrap bump (uhh, i just realized that i was on a slightly outdated master so this bump might have happened already), (Inline cfg(bootstrap) version of debug_assertions intrinsic #121112)): 67s
this: 54s

So this seems like a good solution. I think we can still get the same run-time perf improvements for other users too by massaging this code further (see my other PR about adding #[rustc_no_mir_inline] #121114) but this is a simpler step that solves the imminent problem of "holy shit my rustc is sooo slow".

Funny consequence: This now means compiling the standard library with dbeug assertions makes it faster (than without, when using debug assertions downstream)!

r? @saethlin (or anyone else if someone wants to review this)

fixes #121110, supposedly

saethlin · 2024-02-16T19:16:51Z

Can we just use cfg_attr to swap between inline never and always then put inline always on the fn debug_assertions?

Noratrieb · 2024-02-18T22:13:42Z

I did that and it took 54s, sounds good.

library/core/src/intrinsics.rs

…sertions) The current complexities in `assert_unsafe_precondition` are delicately balancing several concerns, among them compile times for the cases where there are no debug assertions. This comes at a large runtime cost when the assertions are enabled, making the debug assertion compiler a lot slower, which is very annoying. To avoid this, we always inline the check when building with debug assertions. Numbers (compiling stage1 library after touching core): - master: 80s - just adding `#[inline(always)]` to the `cfg(bootstrap)` `debug_assertions`: 67s - this: 54s So this seems like a good solution. I think we can still get the same run-time perf improvements for other users too by massaging this code further (see my other PR about adding `#[rustc_no_mir_inline]`) but this is a simpler step that solves the imminent problem of "holy shit my rustc is sooo slow". Funny consequence: This now means compiling the standard library with dbeug assertions makes it faster (than without, when using debug assertions downstream)!

saethlin · 2024-02-19T17:04:40Z

@bors r+

bors · 2024-02-19T17:04:43Z

📌 Commit 03d03c6 has been approved by saethlin

It is now in the queue for this repository.

…aethlin Always inline check in `assert_unsafe_precondition` with cfg(debug_assertions) The current complexities in `assert_unsafe_precondition` are delicately balancing several concerns, among them compile times for the cases where there are no debug assertions. This comes at a large runtime cost when the assertions are enabled, making the debug assertion compiler a lot slower, which is very annoying. To avoid this, we always inline the check when building with debug assertions. Numbers (compiling stage1 library after touching core): - master: 80s - just adding `#[inline(always)]` to the `cfg(bootstrap)` `debug_assertions` (equivalent to a bootstrap bump (uhh, i just realized that i was on a slightly outdated master so this bump might have happened already), (rust-lang#121112)): 67s - this: 54s So this seems like a good solution. I think we can still get the same run-time perf improvements for other users too by massaging this code further (see my other PR about adding `#[rustc_no_mir_inline]` rust-lang#121114) but this is a simpler step that solves the imminent problem of "holy shit my rustc is sooo slow". Funny consequence: This now means compiling the standard library with dbeug assertions makes it faster (than without, when using debug assertions downstream)! r? `@saethlin` (or anyone else if someone wants to review this) fixes rust-lang#121110, supposedly

Rollup of 8 pull requests Successful merges: - rust-lang#121167 (resolve: Scale back unloading of speculatively loaded crates) - rust-lang#121196 (Always inline check in `assert_unsafe_precondition` with cfg(debug_assertions)) - rust-lang#121206 (Top level error handling) - rust-lang#121223 (intrinsics::simd: add missing functions) - rust-lang#121241 (Implement `NonZero` traits generically.) - rust-lang#121242 (Generate `getelementptr` instead of `inttoptr` for `ptr::invalid`) - rust-lang#121278 (Remove the "codegen" profile from bootstrap) - rust-lang#121286 (Rename `ConstPropLint` to `KnownPanicsLint`) r? `@ghost` `@rustbot` modify labels: rollup

…aethlin Always inline check in `assert_unsafe_precondition` with cfg(debug_assertions) The current complexities in `assert_unsafe_precondition` are delicately balancing several concerns, among them compile times for the cases where there are no debug assertions. This comes at a large runtime cost when the assertions are enabled, making the debug assertion compiler a lot slower, which is very annoying. To avoid this, we always inline the check when building with debug assertions. Numbers (compiling stage1 library after touching core): - master: 80s - just adding `#[inline(always)]` to the `cfg(bootstrap)` `debug_assertions` (equivalent to a bootstrap bump (uhh, i just realized that i was on a slightly outdated master so this bump might have happened already), (rust-lang#121112)): 67s - this: 54s So this seems like a good solution. I think we can still get the same run-time perf improvements for other users too by massaging this code further (see my other PR about adding `#[rustc_no_mir_inline]` rust-lang#121114) but this is a simpler step that solves the imminent problem of "holy shit my rustc is sooo slow". Funny consequence: This now means compiling the standard library with dbeug assertions makes it faster (than without, when using debug assertions downstream)! r? ``@saethlin`` (or anyone else if someone wants to review this) fixes rust-lang#121110, supposedly

Rollup of 8 pull requests Successful merges: - rust-lang#121167 (resolve: Scale back unloading of speculatively loaded crates) - rust-lang#121196 (Always inline check in `assert_unsafe_precondition` with cfg(debug_assertions)) - rust-lang#121241 (Implement `NonZero` traits generically.) - rust-lang#121278 (Remove the "codegen" profile from bootstrap) - rust-lang#121286 (Rename `ConstPropLint` to `KnownPanicsLint`) - rust-lang#121291 (target: Revert default to the medium code model on LoongArch targets) - rust-lang#121302 (Remove `RefMutL` hack in `proc_macro::bridge`) - rust-lang#121318 (Trigger `unsafe_code` lint on invocations of `global_asm`) Failed merges: - rust-lang#121206 (Top level error handling) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#121196 - Nilstrieb:the-clever-solution, r=saethlin Always inline check in `assert_unsafe_precondition` with cfg(debug_assertions) The current complexities in `assert_unsafe_precondition` are delicately balancing several concerns, among them compile times for the cases where there are no debug assertions. This comes at a large runtime cost when the assertions are enabled, making the debug assertion compiler a lot slower, which is very annoying. To avoid this, we always inline the check when building with debug assertions. Numbers (compiling stage1 library after touching core): - master: 80s - just adding `#[inline(always)]` to the `cfg(bootstrap)` `debug_assertions` (equivalent to a bootstrap bump (uhh, i just realized that i was on a slightly outdated master so this bump might have happened already), (rust-lang#121112)): 67s - this: 54s So this seems like a good solution. I think we can still get the same run-time perf improvements for other users too by massaging this code further (see my other PR about adding `#[rustc_no_mir_inline]` rust-lang#121114) but this is a simpler step that solves the imminent problem of "holy shit my rustc is sooo slow". Funny consequence: This now means compiling the standard library with dbeug assertions makes it faster (than without, when using debug assertions downstream)! r? ```@saethlin``` (or anyone else if someone wants to review this) fixes rust-lang#121110, supposedly

RalfJung · 2024-02-22T17:24:55Z

library/core/src/intrinsics.rs

+            // time impact when debug assertions are disabled.
+            // It is not clear whether that is the best solution, see #120848.
+            #[cfg_attr(debug_assertions, inline(always))]
+            #[cfg_attr(not(debug_assertions), inline(never))]


This checks cfg(debug_assertions) which is different from intrinsics::debug_assertions which is used to determine whether the function is even called. Shouldn't the comment explain why that mismatch is okay? It is certainly not obvious to me.

Why wouldn't the difference be okay? Sure, the behavior isn't exactly the same, but users who enable that will get more checks in case they don't enable debug_assertions downstream but that seems okay. We should be able to remove this again after #121114 anyways, I mostly PRed this to avoid thinking in #121114, but I have done the thinking now.

Why wouldn't the difference be okay?

There's a funny case to consider where debug assertions are disabled but intrinsics::debug_assertions returns true. (There's theoretically also the dual case.) I would have expected the comment to acknowledge this and explain why what happens in that case is what we want.

We should be able to remove this again after #121114 anyways

Ah, if this is temporary then never mind. :)

There's a funny case to consider where debug assertions are disabled but intrinsics::debug_assertions returns true.

when building core, that cannot happen. When building user code, yes, that's actually the expected and normal path, core without debug assertions, but debug assertions in the user case.

rustbot assigned saethlin Feb 16, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Feb 16, 2024

This comment has been minimized.

Sign in to view

Noratrieb force-pushed the the-clever-solution branch 2 times, most recently from 95b1f45 to 0f4db16 Compare February 18, 2024 22:15

Noratrieb changed the title ~~Avoid complexity in assert_unsafe_precondition with cfg(debug_assertions)~~ Always inline check in assert_unsafe_precondition with cfg(debug_assertions) Feb 18, 2024

saethlin reviewed Feb 18, 2024

View reviewed changes

library/core/src/intrinsics.rs Outdated Show resolved Hide resolved

Veykril added a commit to ferrocene/ferrocene that referenced this pull request Feb 19, 2024

Pull in rust-lang/rust#121196

610753e

Veykril mentioned this pull request Feb 19, 2024

Automated pull from upstream master ferrocene/ferrocene#285

Merged

Noratrieb force-pushed the the-clever-solution branch from 0f4db16 to 03d03c6 Compare February 19, 2024 16:28

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 Feb 19, 2024

Noratrieb mentioned this pull request Feb 20, 2024

Rollup of 8 pull requests #121332

Closed

This was referenced Feb 20, 2024

Rollup of 8 pull requests #121339

Closed

Rollup of 8 pull requests #121345

Merged

saethlin mentioned this pull request Feb 20, 2024

Totally-not-a-tracking-issue for UB-detecting debug assertions in the standard library #120848

Open

9 tasks

bors merged commit 4ff6bb5 into rust-lang:master Feb 20, 2024
11 checks passed

rustbot added this to the 1.78.0 milestone Feb 20, 2024

Noratrieb deleted the the-clever-solution branch February 20, 2024 19:12

RalfJung reviewed Feb 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Always inline check in `assert_unsafe_precondition` with cfg(debug_assertions) #121196

Always inline check in `assert_unsafe_precondition` with cfg(debug_assertions) #121196

Noratrieb commented Feb 16, 2024 •

edited

Loading

This comment has been minimized.

saethlin commented Feb 16, 2024

Noratrieb commented Feb 18, 2024

saethlin commented Feb 19, 2024

bors commented Feb 19, 2024

RalfJung Feb 22, 2024 •

edited

Loading

Noratrieb Feb 22, 2024 •

edited

Loading

RalfJung Feb 22, 2024 •

edited

Loading

Noratrieb Feb 22, 2024

Always inline check in assert_unsafe_precondition with cfg(debug_assertions) #121196

Always inline check in assert_unsafe_precondition with cfg(debug_assertions) #121196

Conversation

Noratrieb commented Feb 16, 2024 • edited Loading

This comment has been minimized.

saethlin commented Feb 16, 2024

Noratrieb commented Feb 18, 2024

saethlin commented Feb 19, 2024

bors commented Feb 19, 2024

RalfJung Feb 22, 2024 • edited Loading

Choose a reason for hiding this comment

Noratrieb Feb 22, 2024 • edited Loading

Choose a reason for hiding this comment

RalfJung Feb 22, 2024 • edited Loading

Choose a reason for hiding this comment

Noratrieb Feb 22, 2024

Choose a reason for hiding this comment

Always inline check in `assert_unsafe_precondition` with cfg(debug_assertions) #121196

Always inline check in `assert_unsafe_precondition` with cfg(debug_assertions) #121196

Noratrieb commented Feb 16, 2024 •

edited

Loading

RalfJung Feb 22, 2024 •

edited

Loading

Noratrieb Feb 22, 2024 •

edited

Loading

RalfJung Feb 22, 2024 •

edited

Loading