add const_panic macro to make it easier to fall back to non-formatting panic in const #132542

RalfJung · 2024-11-02T22:23:54Z

library/core/src/panic.rs

tgross35 · 2024-11-03T02:05:12Z

I am mildly amused about how similar this is to the patch I had but never finished up. Even down to both of us calling the const string "boring" in the example 😆

rust/library/core/src/panic.rs

Line 196 in 6a9bdd0

    
           /// const_panic!("boring message", "flavored message {a} {b:?}", a: u32 = foo.len(), b: Something = bar);

https://github.com/tgross35/rust/blob/92e6632422a568500bcd49b03943e4ea9e6bb0ca/library/core/src/macros/mod.rs#L931-L932

RalfJung · 2024-11-03T06:21:42Z

I am mildly amused about how similar this is to the patch I had but never finished up. Even down to both of us calling the const string "boring" in the example

I copied that example from you ;)

tgross35

I copied that example from you ;)

Oh, well I guess that explains where I got it too 😄

r=me with green CI

RalfJung · 2024-11-03T07:48:16Z

That's weird, I did run doc tests locally...

…g panic in const

tgross35 · 2024-11-03T11:35:32Z

@bors r+

bors · 2024-11-03T11:35:35Z

📌 Commit bc757f9 has been approved by tgross35

It is now in the queue for this repository.

bors · 2024-11-03T16:20:54Z

⌛ Testing commit bc757f9 with merge e3a918e...

RalfJung · 2024-11-03T18:39:45Z

library/core/src/macros/mod.rs

Hm, maybe I shouldn't put them in this file, since this is generally for macros that are stable / intended to be stabilized?

Did you have it in core::panic before? I guess that could make sense, considering most of the macros in core::panic are doc(hidden) and meant for internal use.

Okay, I've moved them in #132571.

bors · 2024-11-03T19:41:11Z

☀️ Test successful - checks-actions
Approved by: tgross35
Pushing e3a918e to master...

rust-timer · 2024-11-03T21:07:28Z

Finished benchmarking commit (e3a918e): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

Our benchmarks found a performance regression caused by this PR.
This might be an actual regression, but it can also be just noise.

Next Steps:

If the regression was expected or you think it can be justified,
please write a comment with sufficient written justification, and add
@rustbot label: +perf-regression-triaged to it, to mark the regression as triaged.
If you think that you know of a way to resolve the regression, try to create
a new PR with a fix for the regression.
If you do not understand the regression or you think that it is just noise,
you can ask the @rust-lang/wg-compiler-performance working group for help (members of this group
were already notified of this PR).

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	0.8%	[0.2%, 1.8%]	6
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.5%	[-0.9%, -0.1%]	5
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.2%	[-0.9%, 1.8%]	11

Max RSS (memory usage)

Results (primary 3.9%, secondary 3.2%)

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)	5.1%	[2.4%, 11.9%]	6
Regressions ❌ (secondary)	3.2%	[2.7%, 3.8%]	2
Improvements ✅ (primary)	-3.4%	[-3.4%, -3.4%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	3.9%	[-3.4%, 11.9%]	7

Cycles

Results (primary 0.1%)

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)	1.7%	[1.5%, 1.8%]	4
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.9%	[-3.2%, -1.1%]	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.1%	[-3.2%, 1.8%]	7

Binary size

Results (primary 0.3%, secondary -0.0%)

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.4%	[0.0%, 5.2%]	46
Regressions ❌ (secondary)	0.4%	[0.4%, 0.4%]	3
Improvements ✅ (primary)	-0.2%	[-0.5%, -0.0%]	11
Improvements ✅ (secondary)	-0.0%	[-0.2%, -0.0%]	37
All ❌✅ (primary)	0.3%	[-0.5%, 5.2%]	57

Bootstrap: 780.556s -> 780.261s (-0.04%)
Artifact size: 335.29 MiB -> 335.33 MiB (0.01%)

…r=oli-obk add const_eval_select macro to reduce redundancy I played around a bit with a macro to make const_eval_select invocations look a bit nicer and avoid repeating the argument lists. Here's what I got. What do you think? I didn't apply this everywhere yet because I wanted to gather feedback first. The second commit moves the macros from rust-lang#132542 into a more sensible place. It didn't seem worth its own PR and would conflict with this PR if done separately. Cc `@oli-obk` `@saethlin` `@tgross35`

…<try> add const_eval_select macro to reduce redundancy I played around a bit with a macro to make const_eval_select invocations look a bit nicer and avoid repeating the argument lists. Here's what I got. What do you think? I didn't apply this everywhere yet because I wanted to gather feedback first. The second commit moves the macros from rust-lang#132542 into a more sensible place. It didn't seem worth its own PR and would conflict with this PR if done separately. Cc `@oli-obk` `@saethlin` `@tgross35` try-job: dist-aarch64-msvc

…r=oli-obk add const_eval_select macro to reduce redundancy I played around a bit with a macro to make const_eval_select invocations look a bit nicer and avoid repeating the argument lists. Here's what I got. What do you think? I didn't apply this everywhere yet because I wanted to gather feedback first. The second commit moves the macros from rust-lang#132542 into a more sensible place. It didn't seem worth its own PR and would conflict with this PR if done separately. Cc `@oli-obk` `@saethlin` `@tgross35` try-job: dist-aarch64-msvc

…r=oli-obk add const_eval_select macro to reduce redundancy I played around a bit with a macro to make const_eval_select invocations look a bit nicer and avoid repeating the argument lists. Here's what I got. What do you think? I didn't apply this everywhere yet because I wanted to gather feedback first. The second commit moves the macros from rust-lang#132542 into a more sensible place. It didn't seem worth its own PR and would conflict with this PR if done separately. Cc ``@oli-obk`` ``@saethlin`` ``@tgross35`` try-job: dist-aarch64-msvc

rylev · 2024-11-05T20:27:33Z

@RalfJung looks like most of the regressions are taking longer in LLVM related queries (e.g., LLVM_module_codegen_emit_obj) which doesn't make a ton of sense to me. Any ideas? The regressions are small enough that it could be argued we could just let this go, but I wanted to ask before marking as triaged.

RalfJung · 2024-11-05T20:51:18Z

Looking at just the first two benchmarks, this seems to have immediately gone back to the old state in the next PR.

lqd · 2024-11-05T20:58:15Z

Which graph are you looking at? Here are the first 2 benchmarks:

RalfJung · 2024-11-05T21:03:22Z

I looked at the first things it lists when clicking the "instruction count" link. But apparently I just can't read the graph. ;)

So, this PR does change the inline attributes for some of these functions. I can try tweaking them to see if that helps... but it'd be unfortunate if we have to custom-tweak them differently for different panic sites.

RalfJung · 2024-11-05T21:04:52Z

I'll do some experiments in #132662.

tweak attributes for const panic macro Let's do some random mutations of this macro to see if that can re-gain the perf lost in rust-lang#132542.

Rollup merge of rust-lang#132571 - RalfJung:const_eval_select_macro, r=oli-obk add const_eval_select macro to reduce redundancy I played around a bit with a macro to make const_eval_select invocations look a bit nicer and avoid repeating the argument lists. Here's what I got. What do you think? I didn't apply this everywhere yet because I wanted to gather feedback first. The second commit moves the macros from rust-lang#132542 into a more sensible place. It didn't seem worth its own PR and would conflict with this PR if done separately. Cc ``@oli-obk`` ``@saethlin`` ``@tgross35`` try-job: dist-aarch64-msvc

tweak attributes for const panic macro Let's do some random mutations of this macro to see if that can re-gain the perf lost in rust-lang#132542.

…oss35 tweak attributes for const panic macro Let's do some random mutations of this macro to see if that can re-gain the perf lost in rust-lang#132542.

tweak attributes for const panic macro Let's do some random mutations of this macro to see if that can re-gain the perf lost in rust-lang/rust#132542.

rustbot assigned tgross35 Nov 2, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Nov 2, 2024

RalfJung force-pushed the const_panic branch from 2977230 to 6a9bdd0 Compare November 2, 2024 22:28

This comment has been minimized.

Sign in to view

tgross35 reviewed Nov 3, 2024

View reviewed changes

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

tgross35 reviewed Nov 3, 2024

View reviewed changes

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

RalfJung force-pushed the const_panic branch from 6a9bdd0 to 0111d4f Compare November 3, 2024 06:20

This comment has been minimized.

Sign in to view

RalfJung force-pushed the const_panic branch from 0111d4f to f7b18b5 Compare November 3, 2024 06:29

This comment has been minimized.

Sign in to view

RalfJung force-pushed the const_panic branch from f7b18b5 to fc5dd07 Compare November 3, 2024 06:48

tgross35 approved these changes Nov 3, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

RalfJung force-pushed the const_panic branch from fc5dd07 to 1c4474b Compare November 3, 2024 07:58

add const_panic macro to make it easier to fall back to non-formattin…

bc757f9

…g panic in const

RalfJung force-pushed the const_panic branch from 1c4474b to bc757f9 Compare November 3, 2024 07:58

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 Nov 3, 2024

RalfJung commented Nov 3, 2024

View reviewed changes

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 3, 2024

bors merged commit e3a918e into rust-lang:master Nov 3, 2024
7 checks passed

rustbot added this to the 1.84.0 milestone Nov 3, 2024

RalfJung deleted the const_panic branch November 3, 2024 20:45

rustbot added the perf-regression Performance regression. label Nov 3, 2024

RalfJung mentioned this pull request Nov 3, 2024

add const_eval_select macro to reduce redundancy #132571

Merged

RalfJung mentioned this pull request Nov 5, 2024

tweak attributes for const panic macro #132662

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add const_panic macro to make it easier to fall back to non-formatting panic in const #132542

add const_panic macro to make it easier to fall back to non-formatting panic in const #132542

RalfJung commented Nov 2, 2024

This comment has been minimized.

tgross35 commented Nov 3, 2024

RalfJung commented Nov 3, 2024

This comment has been minimized.

This comment has been minimized.

tgross35 left a comment

This comment has been minimized.

RalfJung commented Nov 3, 2024 via email

tgross35 commented Nov 3, 2024

bors commented Nov 3, 2024

bors commented Nov 3, 2024

RalfJung Nov 3, 2024

tgross35 Nov 3, 2024

RalfJung Nov 3, 2024

bors commented Nov 3, 2024

rust-timer commented Nov 3, 2024

rylev commented Nov 5, 2024

RalfJung commented Nov 5, 2024

lqd commented Nov 5, 2024 •

edited

Loading

RalfJung commented Nov 5, 2024

RalfJung commented Nov 5, 2024

add const_panic macro to make it easier to fall back to non-formatting panic in const #132542

add const_panic macro to make it easier to fall back to non-formatting panic in const #132542

Conversation

RalfJung commented Nov 2, 2024

This comment has been minimized.

tgross35 commented Nov 3, 2024

RalfJung commented Nov 3, 2024

This comment has been minimized.

This comment has been minimized.

tgross35 left a comment

Choose a reason for hiding this comment

This comment has been minimized.

RalfJung commented Nov 3, 2024 via email

tgross35 commented Nov 3, 2024

bors commented Nov 3, 2024

bors commented Nov 3, 2024

RalfJung Nov 3, 2024

Choose a reason for hiding this comment

tgross35 Nov 3, 2024

Choose a reason for hiding this comment

RalfJung Nov 3, 2024

Choose a reason for hiding this comment

bors commented Nov 3, 2024

rust-timer commented Nov 3, 2024

Overall result: ❌✅ regressions and improvements - please read the text below

Instruction count

Max RSS (memory usage)

Cycles

Binary size

rylev commented Nov 5, 2024

RalfJung commented Nov 5, 2024

lqd commented Nov 5, 2024 • edited Loading

RalfJung commented Nov 5, 2024

RalfJung commented Nov 5, 2024

lqd commented Nov 5, 2024 •

edited

Loading