rust-lang / cargo

The Rust package manager
https://doc.rust-lang.org/cargo
Apache License 2.0
12.71k stars 2.41k forks source link

Make `bench` profile set `debug = true` by default #14032

Open CraftSpider opened 4 months ago

CraftSpider commented 4 months ago

Problem

Currently, cargo benchmarks use the bench profile by default. This profile inherits release, which makes sense - benchmarks should generally be run in the same context as your optimized release code. However, there's one major difference - benchmarks are also often used for profiling (Such as with criterions --profile-time), and for that, the lack of debug info is a lot more annoying, since it means the results of profiling are significantly less accurate

For a more specific example, working on chumsky, before turning debuginfo on, most all parser code ended up inlined into either the top-level parse call, or the Repeated combinator. This meant that profiling only ever showed those two functions taking up all the time. After turning on debug info, it was made clear that the code actually executing was sourced from a leaf parser (Such as one_of), and knowing that helped me to focus my efforts on improving the performance of that parser, speeding up the whole tree.

Proposed Solution

Keep bench inheriting from release, but make it override just one option - set debug = true

Notes

I don't know the details of profile inheritance, or how hard this would be. I also don't care if it's instead decided to do debuginfo = "linetables" or similar. Line-level info is helpful, but column-level I'm ambivalent about.

weihanglo commented 4 months ago

Thanks for the detailed use case. It is very clear where you come from. One I don't understand is why the default needs to be changed. It is definitely a breaking change to people relying on the current default. Is there anything that hinders you from customizing your profile?

CraftSpider commented 4 months ago

I can (and do) configure this on every project I am a major contributor to if possible. I propose changing the default because this is the kind of thing I need to explicitly remember to do everywhere - the default behavior isn't ever what I want, and I opened this because I believe it is likely very rare that it's what most users want. Rust tends to be very good at doing the right thing by default, and this feels like (an admittedly minor, but constant) papercut to that.

As for it being a breaking change - I have some thoughts: 1) I didn't think adding debug info would really be particularly breaking - it's purely additive, in the same way enabling a Cargo feature on a dependency is, and I'd expect it to count as a 'Minor' breaking change. 2) Even given that, how many people would rely on the lack of debug info in benchmarks? Sure, everything is relied on by someone (XKCD 1172). But rustc has made the decision several times to make breaking changes if they have very low practical impact. Bench seems like a dev profile that happens to want to mostly inherit from 'release'. It would be nice if I could prove this, but I doubt there are lots of people out there who use the bench profile to generate user-facing release artifacts.