Stabilize -Ztimings as --timings

[joshtriplett]

The -Ztimings option has existed for years, and many people use it to
profile and optimize their builds. It's one of the common reasons people
use nightly cargo.

The machine-readable JSON output may warrant further careful inspection
before we commit to a stable format. However, for the human-readable
output we don't need to make any commitment about the exact output.

Add a --timings option, as the stable equivalent to -Ztimings.
(Passing html or json requires -Zunstable-options, but the default --timings does not.)

Document the new option, and update the testsuite.

[rust-highfive]

r? @Eh2406

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

[ehuss]

Thanks for moving this forward!

I'm reluctant to block progress, because I know some people find this feature useful as-is. However, from my perspective it was never finished. I'm just a little leery of stabilizing something that might be somewhat buggy or a poor user experience. There are some concerns I'd like to talk about:

• I don't know if it is useful to have the info form. I haven't seen anyone indicate that it is useful, and it does add a lot of noise (I have never found it useful). The tracking issue also had some discussion about different ways it could be presented.
• The location where the html file is saved may not be ideal (the current directory). It creates a mess of files that the user then needs to manually clean up. I've been thinking it may make more sense to save them somewhere in the target directory?
• The canvas drawing mechanism has issues. It breaks on large graphs (no graphs appear). Adjusting the sliders incorrectly can cause it to break. It is dependent on the browser and graphics card memory limits. Perhaps it works well enough for most people?
• I've noticed some people being a little confused about how to read the information. Ideally I think there would be more built-in help to understand the information. (This isn't any sort of blocker, just a note about it being an overall poor user experience.)
  • Relatedly, some users have been confused due to it not being obvious that you need to scroll horizontally.
• The mouse hover drawing can be quite slow on large graphs.
• The presentation is a little on the ugly side.

Also, just FYI, I've been wanting to support generating reports after the build. Maybe something like cargo report timings, which would generate a report for the most recent build. This would require saving the raw information every time, assuming it isn't too slow. I think it is still useful to have something like --timings to generate reports on every build, though.

Overall I'm excited to move forward! However, I think there might be a few small things to address first (especially the first two points).

[joshtriplett]

@ehuss wrote:

Thanks for moving this forward!

I'm reluctant to block progress, because I know some people find this feature useful as-is. However, from my perspective it was never finished. I'm just a little leery of stabilizing something that might be somewhat buggy or a poor user experience. There are some concerns I'd like to talk about:

* I don't know if it is useful to have the `info` form. I haven't seen anyone indicate that it is useful, and it does add a lot of noise (I have never found it useful). The tracking issue also had some discussion about different ways it could be presented.

While I think it's noisy enough to debate whether to include by default, I do think it adds value when trying to investigate build performance. Sometimes you want the full HTML graph, but sometimes you just want information at the granularity of "Did that specific crate take as long as it looked like it did? Wow, that one takes 20 seconds all by itself."

I think it adds enough value to be worth turning on by default with --timings, but I'm not especially attached to it if you consider it a blocker.

* The location where the html file is saved may not be ideal (the current directory). It creates a mess of files that the user then needs to manually clean up. I've been thinking it may make more sense to save them somewhere in the target directory?

The target directory has the disadvantage of being cleaned by cargo clean, and when benchmarking build performance you may want to do many clean builds and compare the results. Yes, the user has to manually clean them up when done with them, but this avoids accidentally losing valuable data that takes time to collect.

Plus, git status will show them, which will get the user to notice them and clean them up eventually when not needed.

I agree that it isn't the perfect place to put them, but I also don't have a better suggestion. We don't currently have anything else of the form "semi-precious but still a build output, don't clean lightly but make easy to clean".

If you'd prefer, I could make the option create a directory cargo-timing and create all the reports under that, which would make them much easier to clean up with one rm -r cargo-timing.

* The canvas drawing mechanism has issues.  It breaks on large graphs (no graphs appear). Adjusting the sliders incorrectly can cause it to break. It is dependent on the browser and graphics card memory limits.  Perhaps it works well enough for most people?

I haven't seen the former, but I have seen the latter with settings of the sliders. I think it works well enough to be quite useful, and we can always fix further issues in the future.

* I've noticed some people being a little confused about how to read the information.  Ideally I think there would be more built-in help to understand the information.  (This isn't any sort of blocker, just a note about it being an overall poor user experience.)

  * Relatedly, some users have been confused due to it not being obvious that you need to scroll horizontally.

* The mouse hover drawing can be quite slow on large graphs.

* The presentation is a little on the ugly side.

I think all four of these points ware worth evaluating and working on further; it could be more self-explanatory and better presented. But I don't think these are blockers either. And once --timings becomes more widely available, I'm hoping that leads more people to contribute to its usability.

I think the information is more valuable than the exact format.

Also, just FYI, I've been wanting to support generating reports after the build. Maybe something like cargo report timings, which would generate a report for the most recent build. This would require saving the raw information every time, assuming it isn't too slow. I think it is still useful to have something like --timings to generate reports on every build, though.

I love that idea, if it doesn't add significantly to build time. I did notice that the Timings class went out of its way to turn itself into a stub if disabled, so I assumed that it had a non-trivial performance impact. (Not least of which, it appears that having it enabled at all will make rmeta_required return true when it otherwise wouldn't have.)

Overall I'm excited to move forward! However, I think there might be a few small things to address first (especially the first two points).

Happy to address any of these that you consider blockers, and happy to document the rest in issues with an appropriate label.

[alexcrichton]

Personally I agree that it's probably time to stabilize this given how useful it's been throughout its unstable lifecycle (yay nightly sorta?). I agree with all of @ehuss's points but I think that for stabilization, as @joshtriplett mentions as well, we don't necessarily need to solve all of them. I think it'd be good to think about what stability of this option means and only focus on that part. For example I wouldn't consider the literal HTML output stable at all, only that we do output HTML with something human-readable. In that sense any UI issues or performance issues of the HTML itself I think are fine to say "good enough for now and can be improved later".

On the other hand though the fact that we output HTML I think is something worth scrutinizing, in addition to the interface requesting it (e.g. --timings or not). In that sense I don't think that we ever really seriously considered what a stable interface for this would be, so I think it might be worthwhile to consider that before stabilizing.

Personally I would love it if we could enable this by default and always emit target/debug/timings/cargo-timings.html or something like that (along with a duplicate timestamped-file). If we did that then we wouldn't need a CLI flag at all and we could have a section of documentation explaining it and how to read the output as well (and the generated HTML itself could perhaps link to our documentation as well). If we could go this route it's also the smallest to stabilize I think where it's simply an extra file being produced on the build, and deciding the path for that shouldn't be too too bad.

[bors]

☔ The latest upstream changes (presumably #10254) made this pull request unmergeable. Please resolve the merge conflicts.

[joshtriplett]

I've rewritten this PR to remove -Ztimings=info, and to move the timings reports to target/cargo-timings.

[joshtriplett]

Rebased on current HEAD to pick up recent CI fixes.

[joshtriplett]

Since we have unanimous signoff:

@bors r+

[bors]

📌 Commit 85589e1 has been approved by joshtriplett

[bors]

⌛ Testing commit 85589e1 with merge 3bc0e6d...

[bors]

☀️ Test successful - checks-actions
Approved by: joshtriplett
Pushing 3bc0e6d to master...