rust icon indicating copy to clipboard operation
rust copied to clipboard

restrict promotion of `const fn` calls

Open RalfJung opened this issue 5 months ago • 50 comments

We only promote them in const/static initializers, but even that is still unfortunate -- we still cannot add promoteds to required_consts. But we should add them there to make sure it's always okay to evaluate every const we encounter in a MIR body. That effort of not promoting things that can fail to evaluate is tracked in https://github.com/rust-lang/rust/issues/80619. These const fn calls are the last missing piece.

So I propose that we do not promote const-fn calls in const when that may fail without the entire const failing, thereby completing https://github.com/rust-lang/rust/issues/80619. Unfortunately we can't just reject promoting these functions outright due to backwards compatibility. So let's see if we can find a hack that makes crater happy...

For the record, this is the crater analysis from when I tried to entirely forbid this kind of promotion. It's a tiny amount of breakage and if we had a nice alternative for code like that, we could conceivably push it through... but sadly, inline const expressions are still blocked on t-lang concerns about post-monomorphization errors and we haven't yet figured out an implementation that can resolve those concerns. So we're forced to make progress via other means, such as terrible hacks like this.

Attempt one: only promote calls on the "safe path" at the beginning of a MIR block. This is the path that starts at the start block and continues via gotos and calls, but stops at the first branch. If we had imposed this restriction before stabilizing if and match in const, this would have definitely been sufficient...

EDIT: Turns out that works. :) Here's the t-lang nomination comment.

r? @oli-obk

RalfJung avatar Feb 24 '24 16:02 RalfJung

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

rustbot avatar Feb 24 '24 16:02 rustbot

@bors try

RalfJung avatar Feb 24 '24 16:02 RalfJung

:hourglass: Trying commit 98c587a8dcd522834617bdb00369d1806ca3f7f6 with merge 80d5fe37e16a0befb4128414cb6a11d544788c0a...

bors avatar Feb 24 '24 16:02 bors

The job x86_64-gnu-llvm-16 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
#12 writing image sha256:abffa254ca3066906eb880c4c92cfa98fd869348ee7692170852fe05fe148dbf done
#12 naming to docker.io/library/rust-ci done
#12 DONE 10.1s
##[endgroup]
Setting extra environment values for docker:  --env ENABLE_GCC_CODEGEN=1 --env GCC_EXEC_PREFIX=/usr/lib/gcc/
[CI_JOB_NAME=x86_64-gnu-llvm-16]
##[group]Clock drift check
  local time: Sat Feb 24 16:21:25 UTC 2024
  network time: Sat, 24 Feb 2024 16:21:25 GMT
  network time: Sat, 24 Feb 2024 16:21:25 GMT
##[endgroup]
sccache: Starting the server...
##[group]Configure the build
configure: processing command line
configure: 
configure: build.configure-args := ['--build=x86_64-unknown-linux-gnu', '--llvm-root=/usr/lib/llvm-16', '--enable-llvm-link-shared', '--set', 'rust.thin-lto-import-instr-limit=10', '--set', 'change-id=99999999', '--enable-verbose-configure', '--enable-sccache', '--disable-manage-submodules', '--enable-locked-deps', '--enable-cargo-native-static', '--set', 'rust.codegen-units-std=1', '--set', 'dist.compression-profile=balanced', '--dist-compression-formats=xz', '--set', 'build.optimized-compiler-builtins', '--disable-dist-src', '--release-channel=nightly', '--enable-debug-assertions', '--enable-overflow-checks', '--enable-llvm-assertions', '--set', 'rust.verify-llvm-ir', '--set', 'rust.codegen-backends=llvm,cranelift,gcc', '--set', 'llvm.static-libstdcpp', '--enable-new-symbol-mangling']
configure: target.x86_64-unknown-linux-gnu.llvm-config := /usr/lib/llvm-16/bin/llvm-config
configure: llvm.link-shared     := True
configure: rust.thin-lto-import-instr-limit := 10
configure: change-id            := 99999999
---

---- [ui] tests/ui/rfcs/rfc-1623-static/rfc1623-2.rs stdout ----
diff of stderr:

35    = note: `fn(&Foo<'2>) -> &Foo<'2> {id::<&Foo<'2>>}` must implement `FnOnce<(&'a Foo<'1>,)>`, for any lifetime `'1`...
36    = note: ...but it actually implements `FnOnce<(&Foo<'2>,)>`, for some specific lifetime `'2`
- error: aborting due to 4 previous errors
+ error[E0308]: mismatched types
+   --> $DIR/rfc1623-2.rs:28:8
+    |
+    |
+ LL |     f: &id,
+    |        ^^^ one type is more general than the other
+    |
+    = note: expected trait `for<'a, 'b> Fn(&'a Foo<'b>)`
+               found trait `Fn(&Foo<'_>)`
+    = note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no`
+ error[E0308]: mismatched types
+   --> $DIR/rfc1623-2.rs:28:8
+    |
+    |
+ LL |     f: &id,
+    |        ^^^ one type is more general than the other
+    |
+    = note: expected trait `for<'a, 'b> Fn(&'a Foo<'b>)`
+               found trait `Fn(&Foo<'_>)`
+    = note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no`
+ error: implementation of `FnOnce` is not general enough
+   --> $DIR/rfc1623-2.rs:28:8
+    |
+    |
+ LL |     f: &id,
+    |        ^^^ implementation of `FnOnce` is not general enough
+    |
+    = note: `fn(&'2 Foo<'_>) -> &'2 Foo<'_> {id::<&'2 Foo<'_>>}` must implement `FnOnce<(&'1 Foo<'b>,)>`, for any lifetime `'1`...
+    = note: ...but it actually implements `FnOnce<(&'2 Foo<'_>,)>`, for some specific lifetime `'2`
+    = note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no`
+ error: implementation of `FnOnce` is not general enough
+   --> $DIR/rfc1623-2.rs:28:8
+    |
+    |
+ LL |     f: &id,
+    |        ^^^ implementation of `FnOnce` is not general enough
+    |
+    = note: `fn(&Foo<'2>) -> &Foo<'2> {id::<&Foo<'2>>}` must implement `FnOnce<(&'a Foo<'1>,)>`, for any lifetime `'1`...
+    = note: ...but it actually implements `FnOnce<(&Foo<'2>,)>`, for some specific lifetime `'2`
+    = note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no`
+ error: aborting due to 8 previous errors
39 
40 For more information about this error, try `rustc --explain E0308`.
41 
41 


The actual stderr differed from the expected stderr.
Actual stderr saved to /checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/rfcs/rfc-1623-static/rfc1623-2/rfc1623-2.stderr
To update references, rerun the tests and pass the `--bless` flag
To only update this specific test, also pass `--test-args rfcs/rfc-1623-static/rfc1623-2.rs`
error: 1 errors occurred comparing output.
status: exit status: 1
status: exit status: 1
command: RUSTC_ICE="0" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/tests/ui/rfcs/rfc-1623-static/rfc1623-2.rs" "-Zthreads=1" "-Zsimulate-remapped-rust-src-base=/rustc/FAKE_PREFIX" "-Ztranslate-remapped-path-to-local-path=no" "-Z" "ignore-directory-in-diagnostics-source-blocks=/cargo" "--sysroot" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "--json" "future-incompat" "-Ccodegen-units=1" "-Zui-testing" "-Zdeduplicate-diagnostics=no" "-Zwrite-long-types-to-disk=no" "-Cstrip=debuginfo" "--emit" "metadata" "-C" "prefer-dynamic" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/rfcs/rfc-1623-static/rfc1623-2" "-A" "unused" "-A" "internal_features" "-Crpath" "-Cdebuginfo=0" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/rfcs/rfc-1623-static/rfc1623-2/auxiliary"
--- stderr -------------------------------
error[E0308]: mismatched types
##[error]  --> /checkout/tests/ui/rfcs/rfc-1623-static/rfc1623-2.rs:28:8
   |
   |
LL |     f: &id,
   |        ^^^ one type is more general than the other
   |
   = note: expected trait `for<'a, 'b> Fn(&'a Foo<'b>)`
              found trait `Fn(&Foo<'_>)`
error[E0308]: mismatched types
##[error]  --> /checkout/tests/ui/rfcs/rfc-1623-static/rfc1623-2.rs:28:8
   |
LL |     f: &id,
LL |     f: &id,
   |        ^^^ one type is more general than the other
   |
   = note: expected trait `for<'a, 'b> Fn(&'a Foo<'b>)`
              found trait `Fn(&Foo<'_>)`
   = note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no`
error: implementation of `FnOnce` is not general enough
##[error]  --> /checkout/tests/ui/rfcs/rfc-1623-static/rfc1623-2.rs:28:8
   |
LL |     f: &id,
LL |     f: &id,
   |        ^^^ implementation of `FnOnce` is not general enough
   |
   = note: `fn(&'2 Foo<'_>) -> &'2 Foo<'_> {id::<&'2 Foo<'_>>}` must implement `FnOnce<(&'1 Foo<'b>,)>`, for any lifetime `'1`...
   = note: ...but it actually implements `FnOnce<(&'2 Foo<'_>,)>`, for some specific lifetime `'2`
error: implementation of `FnOnce` is not general enough
##[error]  --> /checkout/tests/ui/rfcs/rfc-1623-static/rfc1623-2.rs:28:8
   |
LL |     f: &id,
LL |     f: &id,
   |        ^^^ implementation of `FnOnce` is not general enough
   |
   = note: `fn(&Foo<'2>) -> &Foo<'2> {id::<&Foo<'2>>}` must implement `FnOnce<(&'a Foo<'1>,)>`, for any lifetime `'1`...
   = note: ...but it actually implements `FnOnce<(&Foo<'2>,)>`, for some specific lifetime `'2`
error[E0308]: mismatched types
##[error]  --> /checkout/tests/ui/rfcs/rfc-1623-static/rfc1623-2.rs:28:8
   |
LL |     f: &id,
LL |     f: &id,
   |        ^^^ one type is more general than the other
   |
   = note: expected trait `for<'a, 'b> Fn(&'a Foo<'b>)`
              found trait `Fn(&Foo<'_>)`
   = note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no`
error[E0308]: mismatched types
##[error]  --> /checkout/tests/ui/rfcs/rfc-1623-static/rfc1623-2.rs:28:8
   |
LL |     f: &id,
LL |     f: &id,
   |        ^^^ one type is more general than the other
   |
   = note: expected trait `for<'a, 'b> Fn(&'a Foo<'b>)`
              found trait `Fn(&Foo<'_>)`
   = note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no`
error: implementation of `FnOnce` is not general enough
##[error]  --> /checkout/tests/ui/rfcs/rfc-1623-static/rfc1623-2.rs:28:8
   |
LL |     f: &id,
LL |     f: &id,
   |        ^^^ implementation of `FnOnce` is not general enough
   |
   = note: `fn(&'2 Foo<'_>) -> &'2 Foo<'_> {id::<&'2 Foo<'_>>}` must implement `FnOnce<(&'1 Foo<'b>,)>`, for any lifetime `'1`...
   = note: ...but it actually implements `FnOnce<(&'2 Foo<'_>,)>`, for some specific lifetime `'2`
   = note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no`
error: implementation of `FnOnce` is not general enough
##[error]  --> /checkout/tests/ui/rfcs/rfc-1623-static/rfc1623-2.rs:28:8
   |
LL |     f: &id,
LL |     f: &id,
   |        ^^^ implementation of `FnOnce` is not general enough
   |
   = note: `fn(&Foo<'2>) -> &Foo<'2> {id::<&Foo<'2>>}` must implement `FnOnce<(&'a Foo<'1>,)>`, for any lifetime `'1`...
   = note: ...but it actually implements `FnOnce<(&Foo<'2>,)>`, for some specific lifetime `'2`
   = note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no`
error: aborting due to 8 previous errors

For more information about this error, try `rustc --explain E0308`.
------------------------------------------

rust-log-analyzer avatar Feb 24 '24 16:02 rust-log-analyzer

:sunny: Try build successful - checks-actions Build commit: 80d5fe37e16a0befb4128414cb6a11d544788c0a (80d5fe37e16a0befb4128414cb6a11d544788c0a)

bors avatar Feb 24 '24 17:02 bors

@craterbot check

RalfJung avatar Feb 24 '24 17:02 RalfJung

:ok_hand: Experiment pr-121557 created and queued. :robot: Automatically detected try build 80d5fe37e16a0befb4128414cb6a11d544788c0a :mag: You can check out the queue and this experiment's details.

:information_source: Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot avatar Feb 24 '24 17:02 craterbot

@rust-timer build 80d5fe37e16a0befb4128414cb6a11d544788c0a

RalfJung avatar Feb 24 '24 17:02 RalfJung

Queued 80d5fe37e16a0befb4128414cb6a11d544788c0a with parent 8f359beca4e58bc3ae795a666301a8f47023044c, future comparison URL. There are currently 0 preceding artifacts in the queue. It will probably take at least ~1.2 hours until the benchmark run finishes.

rust-timer avatar Feb 24 '24 17:02 rust-timer

Finished benchmarking commit (80d5fe37e16a0befb4128414cb6a11d544788c0a): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never @rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.9% [0.1%, 6.3%] 43
Regressions ❌
(secondary)
3.3% [0.2%, 9.5%] 24
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.6% [-1.0%, -0.4%] 7
All ❌✅ (primary) 0.9% [0.1%, 6.3%] 43

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
2.4% [1.9%, 3.0%] 3
Regressions ❌
(secondary)
1.5% [0.8%, 1.7%] 4
Improvements ✅
(primary)
-1.9% [-3.1%, -0.8%] 2
Improvements ✅
(secondary)
-4.2% [-4.2%, -4.2%] 1
All ❌✅ (primary) 0.7% [-3.1%, 3.0%] 5

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
6.0% [4.3%, 7.8%] 6
Regressions ❌
(secondary)
5.8% [4.5%, 6.7%] 9
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.0% [-3.0%, -0.9%] 3
All ❌✅ (primary) 6.0% [4.3%, 7.8%] 6

Binary size

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.2% [0.0%, 0.7%] 35
Regressions ❌
(secondary)
2.9% [0.2%, 19.2%] 7
Improvements ✅
(primary)
-0.1% [-0.1%, -0.1%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.2% [-0.1%, 0.7%] 36

Bootstrap: 648.482s -> 651.592s (0.48%) Artifact size: 310.98 MiB -> 311.12 MiB (0.04%)

rust-timer avatar Feb 24 '24 19:02 rust-timer

Hm, I guess that has to be the extra entries in required_consts? Maybe it's not worth it for a sanity check... or maybe we can somehow add only the promoteds that call functions, not all the others.

I am going to worry about perf once crater looks good.

RalfJung avatar Feb 24 '24 19:02 RalfJung

:umbrella: The latest upstream changes (presumably #121569) made this pull request unmergeable. Please resolve the merge conflicts.

bors avatar Feb 25 '24 01:02 bors

:construction: Experiment pr-121557 is now running

:information_source: Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot avatar Mar 08 '24 19:03 craterbot

:tada: Experiment pr-121557 is completed! :bar_chart: 93 regressed and 1 fixed (420397 total) :newspaper: Open the full report.

:warning: If you notice any spurious failure please add them to the blacklist! :information_source: Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot avatar Mar 10 '24 09:03 craterbot

Ah, I screwed up the logic and now it no longer promotes #[rustc_promotable] functions after control flow. That will account for many of these regressions. Fixed it, let's re-run the failed crates.

@bors try

RalfJung avatar Mar 10 '24 10:03 RalfJung

:hourglass: Trying commit 8f4d144e099d2e4734870ca94a4424c8c9e18d63 with merge 983b9dcd30ff3bfd46345f68ea89b90bc1457a15...

bors avatar Mar 10 '24 10:03 bors

:sunny: Try build successful - checks-actions Build commit: 983b9dcd30ff3bfd46345f68ea89b90bc1457a15 (983b9dcd30ff3bfd46345f68ea89b90bc1457a15)

bors avatar Mar 10 '24 11:03 bors

:sunny: Try build successful - checks-actions Build commit: 983b9dcd30ff3bfd46345f68ea89b90bc1457a15 (983b9dcd30ff3bfd46345f68ea89b90bc1457a15)

bors avatar Mar 10 '24 11:03 bors

@craterbot run mode=check-only crates=https://crater-reports.s3.amazonaws.com/pr-121557/retry-regressed-list.txt p=1

RalfJung avatar Mar 10 '24 12:03 RalfJung

:ok_hand: Experiment pr-121557-1 created and queued. :robot: Automatically detected try build 983b9dcd30ff3bfd46345f68ea89b90bc1457a15 :mag: You can check out the queue and this experiment's details.

:information_source: Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot avatar Mar 10 '24 12:03 craterbot

:construction: Experiment pr-121557-1 is now running

:information_source: Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot avatar Mar 11 '24 23:03 craterbot

:tada: Experiment pr-121557-1 is completed! :bar_chart: 0 regressed and 0 fixed (93 total) :newspaper: Open the full report.

:warning: If you notice any spurious failure please add them to the blacklist! :information_source: Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot avatar Mar 12 '24 01:03 craterbot

No more regressions, awesome. :) I'll look into perf.

But meanwhile we can start to get the t-lang process rolling. @rust-lang/lang this PR aims to finally achieve the goal of the infallible promotion RFC. Originally the RFC said we should never promote operations that may panic or otherwise fail. We almost managed to do that, with one exception:

const fn my_function() -> i32 { panic!() }

const C: &'static i32 = {
  let x = &my_function();
  x
};

This would compile because we are promoting all calls to const fn in const/`static initializers. I tried a while ago to forbid this, which found 2 regressions. The regressed code didn't really have great alternatives to switch to (that would require inline consts), so I didn't pursue this further at the time, hoping to pick it up again when inline const get stabilized.

Unfortunately, years later, inline const are still not stable. And even if they become stable (which actually looks like it may not be too far off now :), it is unclear whether we want to break that old code that has worked for so long. It turns out there is a contingency plan that keeps that code working but rejects other, more complicated code that we haven't seen in the wild yet -- but the longer we wait the more likely it is to appear. So this is my contingency plan: the actual goal of the "infallible promotion" RFC was to ensure that we can evaluate any const we see in a MIR body without worrying about whether that would lead to incorrect compilation failures (i.e., leading compilation to fail when it should not). This property is something we want as it means we can traverse the MIR for analyses and optimizations and just "look at" all consts without worrying about possible side-effects. We even guarantee that all consts one explicitly syntactically mentions in a function get evaluated, even in dead code, which is a pattern some crates rely on for soundnes. Promotion adds new consts, which is why it needs to be restricted, or else we need to carefully skip promoteds when evaluating all consts. The easiest way to achieve this is to ensure we only create promoteds that cannot fail, then they trivially cannot break this property. But there is another way: if we look again at the example above, then that program would fail to compile even if we didn't promote. In other words, promoting the &my_function() and then evaluating that promoted does not introduce an incorrect compilation failure! Only creating fallible promoteds in dead code can cause incorrect compilation failure.

So I propose that we achieve the goals of the RFC while maintaining compatibility as much as possible by only promoting function calls in const/static initializers if they are definitely not in dead code. This PR implements a fairly trivial way to achieve that: we walk the CFG of the initializer from the start block until the first branch (i.e., we walk down the "line" of basic blocks at the top that is formed by Goto and function calls), and all these blocks are definitely not dead. We only promote function calls in those blocks. This will miss blocks that are definitely not dead due to merging control flow (i..e, anything after the first if). But it turns out this is already sufficient to give us 0 crater regressions, so a more complicated analysis does not seem worth it.

In other words, the code above still compiles after this PR. The following code no longer compiles after this PR, and that is intended:

const fn my_function() -> i32 { panic!() }

const C: &'static i32 = {
  if false { // or any other condition
    let x = &my_function();
    x
  } else {
    &42
  }
};

The following code also no longer compiles after this PR, which is unfortunate but doesn't seem worth the effort of making it compile:

const fn my_function() -> i32 { panic!() }

const C: &'static i32 = {
  if true {} // or any other condition
  let x = &my_function();
  x
};

Maybe in a future edition we can entirely stop promoting function calls in const/static initializers. That is blocked on having inline consts. However, to make the desired property of being able to evaluate all consts we see actually useful to compiler developers, we need to achieve this property on all editions, so any such edition-related plan does not substitute a PR like this. Also, edition transitions for promotions are very hard (there is no plan for how one could possibly do automatic migration or future-compat lints), so I wouldn't want to bank on this.

I this pretty? No! Promotion rules are suddenly different in different parts of a const/static initializer. If I had a time machine I'd just make sure we never promote const fn calls to begin with. But mistakes were made and we have to live with them if we want to maintain compatibility. All we can do is mitigate the consequences of those mistakes, so I propose we add this new wart to the language to mitigate the problems caused by that old mistake. We already have value-dependent promotion (&(0/1) gets promoted but &(1/0) and &(0/(1*1)) do not, to make sure we do not promote a division by 0), now we add location-dependent promotion. In other words, promotion is already ugly, this just makes it a bit more ugly. I hope users will never run into this, and the fact that crater found no regressions makes me confident that this will at least be extremely rare.

If we ever get inline consts then we can consider cleaning up this mess by entirely stopping to promote function calls in const/static initializers, and accepting the breaking change. But I don't want to bet on the fact that we'll be able to do that, it's been years since the last crater run and the ecosystem has grown significantly since then. So this PR is forward-compatible with making things nice(r) again, but meanwhile accepting this PR ensures that MIR consumers can finally stop worrying about which consts they can or cannot evaluate.

RalfJung avatar Mar 12 '24 06:03 RalfJung

@bors try @rust-timer queue

RalfJung avatar Mar 12 '24 07:03 RalfJung

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

rust-timer avatar Mar 12 '24 07:03 rust-timer

:hourglass: Trying commit e949b497d6a20a43f9b00b1f3036e3022ed8b70e with merge a781613e963cbaafde43953dd4872c1a7fe86b91...

bors avatar Mar 12 '24 07:03 bors

:sunny: Try build successful - checks-actions Build commit: a781613e963cbaafde43953dd4872c1a7fe86b91 (a781613e963cbaafde43953dd4872c1a7fe86b91)

bors avatar Mar 12 '24 08:03 bors

Queued a781613e963cbaafde43953dd4872c1a7fe86b91 with parent 0fa7feaf3f287b900176061053619c06306c67b0, future comparison URL. There is currently 1 preceding artifact in the queue. It will probably take at least ~2.2 hours until the benchmark run finishes.

rust-timer avatar Mar 12 '24 08:03 rust-timer

Finished benchmarking commit (a781613e963cbaafde43953dd4872c1a7fe86b91): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never @rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
3.0% [0.4%, 6.4%] 11
Regressions ❌
(secondary)
4.6% [0.3%, 8.0%] 15
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.7% [-0.9%, -0.6%] 6
All ❌✅ (primary) 3.0% [0.4%, 6.4%] 11

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

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

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
6.1% [4.5%, 8.2%] 6
Regressions ❌
(secondary)
5.3% [3.8%, 7.2%] 12
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 6.1% [4.5%, 8.2%] 6

Binary size

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.2% [0.0%, 0.7%] 40
Regressions ❌
(secondary)
2.8% [0.2%, 18.2%] 7
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.2% [0.0%, 0.7%] 40

Bootstrap: 672.448s -> 672.523s (0.01%) Artifact size: 310.05 MiB -> 309.95 MiB (-0.03%)

rust-timer avatar Mar 12 '24 10:03 rust-timer

Let's see whether this is due to adding more required_consts or due to computing the "promotion-safe blocks".

@bors try @rust-timer queue

RalfJung avatar Mar 12 '24 12:03 RalfJung