Enable frame pointers for the standard library

[saethlin]

There's been a few past experiments for enabling frame pointers for all our artifacts. I don't think that frame pointers in the distributed compiler are nearly as useful as frame pointers in the standard library. Our users are much more likely to be profiling apps written in Rust than they are profiling the Rust compiler.

So yeah it would be cool to have frame pointers in the compiler, but much more of the value is having them on the precompiled standard library. That's what this PR does.

[saethlin]

@bors try @rust-timer queue

[bors]

⌛ Trying commit aeb3447 with merge 389d0b1...

[bors]

☀️ Try build successful - checks-actions
Build commit: 389d0b1 (389d0b1a4be0151c5071f00bc3c4836c87ccb5ab)

[rust-timer]

Finished benchmarking commit (389d0b1): comparison URL.

Overall result: ❌ regressions - 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.3%	[0.2%, 0.6%]	67
Regressions ❌ (secondary)	0.3%	[0.2%, 1.5%]	20
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.3%	[0.2%, 0.6%]	67

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)	3.0%	[3.0%, 3.0%]	1
Regressions ❌ (secondary)	2.8%	[2.2%, 3.4%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	3.0%	[3.0%, 3.0%]	1

Cycles

This benchmark run did not return any relevant results for this metric.

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.2%, 0.2%]	4
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.9%	[-2.0%, -0.1%]	11
Improvements ✅ (secondary)	-1.8%	[-2.6%, -0.2%]	39
All ❌✅ (primary)	-0.6%	[-2.0%, 0.2%]	15

Bootstrap: 668.447s -> 667.741s (-0.11%)
Artifact size: 312.75 MiB -> 312.75 MiB (-0.00%)

[saethlin]

There are no cycles regressions. I think that fact alone justifies @rustbot label: +perf-regression-triaged

But in addition, I looked at a number of the other regressions near the top. The cachegrind diff indicates that in all cases I checked, at least half the icount regression is from __rdl_alloc + __rdl_dealloc. Many of the significance factors here are less than 2. If someone really wants to make the icount number go down, someone can figure out how to special-case the __rdl_ symbols.

[saethlin]

r? bootstrap

[Noratrieb]

for the motivation: This makes frame pointers fully functional for users, so now when a user enables frame pointers, their entire program will be compiled with them (which is pretty important) instead of just their code and their dependencies.

[Noratrieb]

previous attempts at this have mentioned that x86(32) really benefits from not having frame pointers (unlike most other ISAs). Maybe it would be worth special casing it and not enabling frame pointers there. Maybe also not without a benchmark, and I don't plan on benchmarking it.

[onur-ozkan]

@bors r+

[bors]

📌 Commit aeb3447 has been approved by onur-ozkan

It is now in the queue for this repository.

[Noratrieb]

tbh, I feel like this might have warranted an MCP, we've done MCPs for things that are smaller in scope. but i don't feel strongly about it. I think this change should at least be announced in the next compiler meeting for some visibility, @apiraino could you do that?
also I think this is worth pointing out in the release notes
@rustbot label relnotes

[bors]

⌛ Testing commit aeb3447 with merge 3cdcdaf...

[jonhoo]

ref #103711 (comment), is there an argument for starting to enable them by default for user code too? maybe at an edition boundary?

[Kobzol]

I wonder what's the motivation for that for user code? Without debuginfo, which needs to be opted into anyway for user code in release, it's not that useful, or is it?

[Noratrieb]

I think there is an argument to be made, rather successfully even probably. That would require an MCP for sure.

[Noratrieb]

I wonder what's the motivation for that for user code? Without debuginfo, which needs to be opted into anyway for user code in release, it's not that useful, or is it?

It's pretty useful without debuginfo, symbols+frame pointers give fairly meaningful (although thanks to inlining not extremely granular) backtraces.

[bjorn3]

Also it allows correctly unwinding through stdlib functions into user code compiled with debuginfo, thereby correctly accounting the amount of time spent in user code where previously it would result in samples in those functions without frame pointers effectively being garbage.

[tiberiusferreira]

I wonder what's the motivation for that for user code? Without debuginfo, which needs to be opted into anyway for user code in release, it's not that useful, or is it?

Hey, this is very useful for continuous profiling, such as when using https://github.com/tikv/pprof-rs .

Enabling it by default would be a great QoL improvement.

[workingjubilee]

For those wanting to recommend we enable frame pointers by default, it may be best to propose it to the compiler team and perhaps first get answers to some questions like "wait, doesn't the aarch64 call ABI use frame-pointers anyways? so the perf regressions, such as they are, don't necessarily materialize on all platforms? has anyone checked the assembly to verify this?"

@jonhoo I don't really see what the edition has to do with this, honestly!

[saethlin]

We already build the precompiled standard library with full debuginfo and unwinding panics, even though those compromise the optimization quality and build time for users who do not need/want those settings, because if we don't enable those settings in the precompiled standard library there is no way for stable users to opt back into them.

The default value of a stable flag does not have the same one-way-door effect. Perhaps enabling frame pointers is a better default. But it's a different conversation.

[nnethercote]

This recent blog post is relevant, and an argument in favour of frame pointers on platforms other than (32-bit) x86.

[saethlin]

😂 That blog post was linked on the issue that this PR was spawned from, and was indeed the kick I needed to try this out. I hope Brendan appreciates how quickly his blog has had an effect.

[Kobzol]

(Oh my god, this is what has been causing these super weird flamecharts all this time?)

[ag039st]

Great thanks

[Kobzol]

There is a small instruction count cost across the board, but cycles and walltimes are mostly unaffected. Given that this can apparently make profiling much better, I think that it is worth it.

@rustbot label: +perf-regression-triaged

[workingjubilee]

Yeah, I would expect most superscalar processors to be smart enough to handle the frame pointer push/pop operations "at the same time" as other things. Thus more instructions, same-ish throughput.

[RalfJung]

previous attempts at this have mentioned that x86(32) really benefits from not having frame pointers (unlike most other ISAs). Maybe it would be worth special casing it and not enabling frame pointers there. Maybe also not without a benchmark, and I don't plan on benchmarking it.

I don't think this was answered properly?

FWIW I agree this would have warranted somewhat wider discussion.