Release profile used instead of bench profile in benchmark

[jean-airoldie]

Since the --release flag is automatically added to commands, using cargo pgo bolt build -- --benches results in the release profile incorrectly being used instead of the bench profile.

[Kobzol]

Thanks for the report! Hmm, that is unfortunate, I didn't want to inspect the arguments after --. But I guess that scanning for --bench or --benches and avoiding passing the --release flag in that case might not be that bad.

Would you like to try to implement this and send a PR? If not, I'll add it to my TODO list.

[jean-airoldie]

What is the reasoning for passing the --release flag in the first place? I understand that it makes little sense to run profile guided optimization on a debug build, but letting the user specify the profile would avoid this problem as well as be in line with cargo's default build behavior.

The use case I had in mind would be to have a custom profile for the PGO step with full debug=true, LTO=off and then a final release step with debug=strip,LTO=true.

[Kobzol]

What is the reasoning for passing the --release flag in the first place? I understand that it makes little sense to run profile guided optimization on a debug build, but letting the user specify the profile would avoid this problem as well as be in line with cargo's default build behavior.

Well, in retrospect, it might not have been such a great idea :) I indeed wanted to avoid the very common situation where people forget to pass --release. But perhaps just printing a warning in that case might be a better idea, it would certainly make the code of cargo-pgo simpler and remove similar edge-cases.

The use case I had in mind would be to have a custom profile for the PGO step with full debug=true, LTO=off and then a final release step with debug=strip,LTO=true.

From my experience, to achieve the best results, you should PGO instrument the already LTO-optimized code, and then PGO optimize again LTO-optimized code. But use-cases might vary, of course. I wonder why you need debug=true for the PGO step though? 🤔

[jean-airoldie]

But perhaps just printing a warning in that case might be a better idea, it would certainly make the code of cargo-pgo simpler and remove similar edge-cases.

Ouais that sounds simpler indeed.

From my experience, to achieve the best results, you should PGO instrument the already LTO-optimized code, and then PGO optimize again LTO-optimized code. But use-cases might vary, of course. I wonder why you need debug=true for the PGO step though? 🤔

I will keep that in mind about LTO. As for the debug=true, from my understanding bolt throws a linker error if you strip the debug annotations, and ideally I don't want debug info in my release. Worst case I could strip the debug info manually via strip(1) after the fact.

[Kobzol]

I remember some issues with BOLT and stripping, but at least on rustc it seems to work fine now, IIRC. If you don't want debuginfo in the final binary, then you shouldn't set debug=true anywhere though.

[Kobzol]

By the way, is there a reason why you cannot use cargo pgo bolt bench, instead of build --benches? In that case cargo-pgo doesn't set the --release flag.

[jean-airoldie]

I remember some issues with BOLT and stripping, but at least on rustc it seems to work fine now, IIRC. If you don't want debuginfo in the final binary, then you shouldn't set debug=true anywhere though.

On rustc rustc 1.85.0-nightly (5f23ef7d3 2024-12-20), I still get some linking errors if I strip the debug info currently with a note:

[...]
  = note: rust-lld: error: --strip-all and --emit-relocs may not be used together
          collect2: error: ld returned 1 exit status

By the way, is there a reason why you cannot use cargo pgo bolt bench, instead of build --benches? In that case cargo-pgo doesn't set the --release flag.

As of v0.2.9 this doesn't seem to be a supported command. There is a cargo pgo bench command but nothing for cargo pgo bolt bench.

[Kobzol]

Uhh, sorry, I suggested something that I clearly hallucinated (I promise that I'm not an AI 😆). I guess I could add that command, for me I considered running PGO/BOLT on benches to not be the ideal thing, so I didn't pay it that much attention.

I'm not sure what to do about the --release flag situation, tbh. If I removed --release now, there will probably be a lot of projects that will suddenly start using debug mode, because they did not pass --release before (since cargo-pgo did it for them). Even if I released a breaking version, that doesn't amount to much, as cargo-pgo is a binary tool, not a library.

[jean-airoldie]

I'm not sure what to do about the --release flag situation, tbh.

Yeah that's not an easy situation.

The only thing i could think off would be to either add a non-default feature to disable that behavior or some kind of config file, cargo editions style. However, this makes it fairly unlikely that an entirely new user would use said feature, and makes the tool more complicated to use.

[tbillington]

What about an optional --profile argument the same way cargo works. If none is provided --release is used, if a profile is provided it uses the specified profile? I think it's not uncommon to have a profile called distribution etc which is different from release.

[Kobzol]

cargo-pgo already detects --profile (IIRC) and doesn't pass --release in that case. The problem are commands like bench that set the profile implicitly.

[tbillington]

Ah, my bad. Thanks for the correction