Report codegen timings for binary crates

[bjorn3]

This is done by enabling --emit metadata for binary crates too despite them not actually supporting crate metadata. Rustc will produce a harmless empty metadata file, but also report exactly when codegen starts just like currently happens for rlibs.

[rust-highfive]

r? @ehuss

(rust-highfive has picked a reviewer for you, use r? to override)

[bjorn3]

To copy what I have said on zulip. cargo clean -p deletes the new .rmeta files. In addition this is also a prerequisite for https://github.com/rust-lang/rust/issues/93945 once I revive it as that PR needs .rmeta files for dylib crates too.

[weihanglo]

Ping @bjorn3 . Just checking in to see if you are still interested in working on this, or if you could have time to check there is no artifact name conflict on those cases (and maybe others out of our radar).

[bjorn3]

I'm still interested in this. A similar problem with respect to filename conflicts exists for split debuginfo and it seems like we are just going to accept that there are conflicts in that case: https://github.com/rust-lang/cargo/pull/11384#issuecomment-1319783149 Split debuginfo is not read back by rustc, so it is merely a problem for debuggers, but rmeta files are read by rustc, so it may actually cause issues. One idea would be to force the metadata hash in the filename for rmeta files even if the main linked artifact doesn't have it. This is safe as rmeta files don't contain their own filename, unlike dylibs. WDYT @weihanglo?

[weihanglo]

Thank you for the response. To me, the situation of this PR is different from debuginfo one: The latter would introduce new broken stuff, but the former might break existing code.

I think emitting rmeta with metadata hash name might work, though I am not sure how much we will pay for the change. Do you have a workable patch to share?

[bjorn3]

I think emitting rmeta with metadata hash name might work, though I am not sure how much we will pay for the change. Do you have a workable patch to share?

I don't have a patch yet as I first wanted to know if it would be acceptable. I think using --emit metadata=/path/to/libfoo-hash.rmeta rather than --emit metadata at https://github.com/rust-lang/cargo/blob/70898e522116f6c23971e2a554b2dc85fd4c84cd/src/cargo/core/compiler/mod.rs#L915-L924 would be enough for that.

[weihanglo]

I think using --emit metadata=/path/to/libfoo-hash.rmeta rather than --emit metadata at

Hmm… that doesn't look too good to me. Should we track them by calc_rustc_outputs(…) and list them in cx.outputs(…)? I am a bit worried about losing tracked by Cargo's compilation outputs.

Apart from that, below is a report from cargo b --timings upon commit https://github.com/rust-lang/cargo/pull/10960/commits/1e3648bd12ca3b3b7eeeff0ce0a7a6c0affd9192. Does it look correct for a binary unit containing both metadata and codegen parts in this graph? I thought binaries only have to codegen, so it should be all lavender. I am not an expert and might be wrong though.

[bjorn3]

Does it look correct for a binary unit containing both metadata and codegen parts in this graph? I thought binaries only have to codegen, so it should be all lavender. I am not an expert and might be wrong though.

The blue part is everything that happens before codegen as metadata emission is the last thing happening before codegen. For binaries the metadata is empty, but it is still useful to know when codegen starts.

[weihanglo]

For binaries the metadata is empty, but it is still useful to know when codegen starts.

Yep. I meant shouldn't the codegen start at the same time of the binary unit starts? Below is the current (1.66) graph which shows no codegen (no lavender color) at all. I am confused. Sorry 😞.

And it looks like the graph is drawn as "no rmeta -> all blue" regardless of codegen or not.

https://github.com/rust-lang/cargo/blob/c7fb75640a605027ae61cbd75d52caaeb8d3ca13/src/cargo/core/compiler/timings.js#L98-L109

[bjorn3]

Yep. I meant shouldn't the codegen start at the same time of the binary unit starts?

Codegen can't start until macro expansion, typeck, borrowck, ... are done.

Below is the current (1.66) graph which shows no codegen (no lavender color) at all.

Cargo uses the moment that the rmeta file is emitted as proxy for the moment that codegen starts. As such before this PR it wouldn't record any codegen time as no rmeta file is created, even though codegen does actually happen. This PR unconditionally asks rustc to emit metadata precisely to allow metadata emission to be a proxy for the moment codegen starts for all crate types, not just those that don't need linking.

[weihanglo]

Make a lot of sense. Thanks for the explanation!

Unfortunately, I don't quite sure about the consequence of setting explicit --emit path 😞. I still feel like it should be tracked in any case. We may also need a new the approach to always give each .rmeta a hash but no for artifact explicitly excluded (dylib and friends?).

I believe @ehuss could tell a better story than me.

[bjorn3]

Unfortunately, I don't quite sure about the consequence of setting explicit --emit path

Me neither. I would expect it to be properly tracked due to the depinfo file produced by rustc containing the right path, but I'm not familiar with the cargo internals, so I don't know if it would have any other side effects.

We may also need a new the approach to always give each .rmeta a hash but no for artifact explicitly excluded (https://github.com/rust-lang/cargo/pull/10960#pullrequestreview-1082980119).

Possibly

[ehuss]

I think it would be ok to use an explicit path as long as the output-tracking code was kept in sync. Unfortunately my instinct is that might be a little messy. If it can be done without too much trouble, I think it should be fine.

I still feel that it would be better to add messages to rustc to report timing information. I imagine knowing link time would be really useful. I understand that's a much bigger project, but I wouldn't expect it to be too difficult, since rustc is already tracking that information. It seems like it would mostly be a discussion around the exact JSON structure, how it is enabled, and maybe making it clear that the structure can change over time.

[bors]

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