Display crate version on timings graph #12420

Angelin01 · 2023-07-31T13:22:52Z

What does this PR try to resolve?

The solution here is the simplest one: since duplicate crates with different versions can cause confusion, we always output the version.

How should we test and review this PR?

Build any create using the --timings option:

cargo build --timings

and verify the resulting HTML file.

Additional information

The formatting is consistent with how the crates are displayed in the terminal output for Cargo and also on the table below the graph. Initially, this comment suggested said formatting for ease of searching, but I do believe the browser can't search in the graph itself.

Sample console output:

   Compiling lazycell v1.3.0
   Compiling unicode-xid v0.2.4
   Compiling unicode-width v0.1.10
   Compiling glob v0.3.1
   Compiling curl-sys v0.4.65+curl-8.2.1
   Compiling curl v0.4.44
   Compiling git2 v0.17.2

Sample rendered HTML output:

Possible issues and future work

A possible issue in this solution is that the output becomes too noisy. Other possible solutions:

Only display the version if there are two "units" with the same name. One possible implementation is to count the names, aggregate them in a simple map and look it up during rendering.
Create a small selection to select the disambiguate method between "Never", "Always" and "Only duplicates". This may be overkill.

rustbot · 2023-07-31T13:22:56Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @epage (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

epage · 2023-07-31T13:38:13Z

src/cargo/core/compiler/timings.js

@@ -111,7 +111,7 @@ function render_pipeline_graph() {
    ctx.textAlign = 'start';
    ctx.textBaseline = 'middle';
    ctx.font = '14px sans-serif';
-    const label = `${unit.name}${unit.target} ${unit.duration}s`;
+    const label = `${unit.name} v${unit.version}${unit.target} ${unit.duration}s`;


For me, my biggest concern is how easily can I digest the timing numbers. I feel that rendering like quote v1.0.28 0.45s makes this difficult

Ideas

@weihanglo mentioned only showing versions on duplicates

In --timings: show crate version in the graph? #7384, parenthesis were used, adding noise but also making it easier to scan

Tooltips might be a possibility

Any other ideas?

Something else I want to keep in mind is that some improvement is better than no improvement, so if we can't converge on a solution quickly enough, we probably should just move forward.

I have no strong opinion, much like @weihanglo. I opened the PR with the simple one line solution exactly for the same reason: displaying the version always is better than not showing at all.

I can implement "showing versions on duplicates" or change the format relatively easily, but tooltips on rendered graphs might be beyond my JS abilities.

I can definitely understanding of limited JS abilities; I'm in a similar boat :). As I said, some improvement is better than none so I didn't want to put too much of a burden on this PR.

Just checked in with @weihanglo and if you are ok with limiting this to dupes, let's go with that. We can iterate on this further later.

I do believe the browser can't search in the graph itself.

Oops. My bad. I forgot it is an SVG.

I got a simple deduplication implementation using the name and target as a "tag", something like this:

const unitCount = new Map(); // snip const tag = `${unit.name}${unit.target}`; const count = unitCount.get(tag) || 0; unitCount.set(tag, count + 1); // snip const tag = `${unit.name}${unit.target}`; const labelName = (unitCount.get(tag) || 0) > 1 ? `${unit.name} v${unit.version}${unit.target}` : tag; const label = `${labelName} ${unit.duration}s`;

(See the full change in the da0fab3 commit, I'd like to squash these once we settle on a proper behavior)

But I'm still getting some duplicate behavior, apparently we are compiling the same crate twice? Or it's showing on the graph twice and I'm not sure why. It does differentiate per target, so at least that:

I'm not exactly happy with the duplicated versions, but it does seem to be at least de-duping multiple versions correctly. Unfortunately, it does create some more noise.

Thanks for trying it out and sharing the results!

I am curious why syn build script (run) doesn't have a version because that should be a dupe.

What do you think of adding separator between labelName and duration so we get a little bit more visual distinction? Maybe : or -?

But I'm still getting some duplicate behavior, apparently we are compiling the same crate twice?

I suspect this is because we compile for "host" for build scripts and "target" for everything else. We try to de-duplicate these build steps. Unsure what is going on that this de-duplication isn't working. I wonder if its related to the debug info shrinking or something else.

I am curious why syn build script (run) doesn't have a version because that should be a dupe.

Because the target is different. I explicitly used name + target for uniqueness, but that's easy enough to change.

What do you think of adding separator between labelName and duration so we get a little bit more visual distinction? Maybe : or -?

I like :.

@epage @weihanglo
I'm willing to implement any chosen format as long as we decide which one. I personally still lean towards always showing the versions as I find it less confusing (someone looking at this might ask: "why is it showing the version for X and Y but not Z?") but that's just me.

If consistent formatting with the rest of the output is not necessary here, may I suggest one of these formats:

serde_json v1.2.3 syn build script (run): 0.21s

serde_json (v1.2.3) syn build script (run): 0.21s

serde_json v1.2.3 syn build script (run) - 0.21s

serde_json (v1.2.3) syn build script (run) - 0.21s

Bear in mind that target can be empty string and, since we're showing versions only on duplicate entries, so can it.

Unsure what is going on that this de-duplication isn't working. I wonder if its related to the debug info shrinking or something else.

Could be. I think we can move on and deal with it afterward.

Regarding the format, I'll leave the choice to you :)

BTW, could you help updates screenshots in that pages? Some of them could be in a better resolution.

weihanglo

Thanks! Let's merge it :)

weihanglo · 2023-08-01T08:42:52Z

@bors r+

bors · 2023-08-01T08:42:54Z

📌 Commit da0fab3 has been approved by weihanglo

It is now in the queue for this repository.

bors · 2023-08-01T08:43:01Z

⌛ Testing commit da0fab3 with merge 2da47eb...

Display crate version on timings graph ### What does this PR try to resolve? Fixes #7384 The solution here is the simplest one: since duplicate crates with different versions can cause confusion, we always output the version. ### How should we test and review this PR? Build any create using the `--timings` option: ``` cargo build --timings ``` and verify the resulting HTML file. ### Additional information The formatting is consistent with how the crates are displayed in the terminal output for Cargo and also on the table below the graph. Initially, [this comment](#7384 (comment)) suggested said formatting for ease of searching, but I do believe the browser can't search in the graph itself. Sample console output: ``` Compiling lazycell v1.3.0 Compiling unicode-xid v0.2.4 Compiling unicode-width v0.1.10 Compiling glob v0.3.1 Compiling curl-sys v0.4.65+curl-8.2.1 Compiling curl v0.4.44 Compiling git2 v0.17.2 ``` Sample rendered HTML output: ![image](https://github.com/rust-lang/cargo/assets/17818024/23aae84e-5107-4352-8d0f-ecdfcac59187) ### Possible issues and future work A possible issue in this solution is that the output becomes too noisy. Other possible solutions: - Only display the version if there are two "units" with the same name. One possible implementation is to count the names, aggregate them in a simple map and look it up during rendering. - Create a small selection to select the disambiguate method between "Never", "Always" and "Only duplicates". This may be overkill.

weihanglo · 2023-08-01T08:48:25Z

Thought you've changed the format but apparently not.

Will wait for you replying back :)

@bors r-

bors · 2023-08-01T09:25:15Z

☀️ Try build successful - checks-actions
Build commit: 2da47eb (2da47eb48cc8b76013e55d4dfaf614888d2217e4)

Angelin01 · 2023-08-01T12:08:15Z

@weihanglo I've adjusted the format to number 2, only showing the version when there are duplicate crates and taking epage's suggestion to add a : separator and put the version between (). Here's a report so you can have a look yourself if you want.

cargo-timing-20230801T120113Z.tar.gz

BTW, could you help updates screenshots in that pages? Some of them could be in a better resolution.

I'm sorry, I don't know which screenshots you mean, but I'm happy to help if you point me in the correct direction.

weihanglo · 2023-08-01T13:32:19Z

Was talking about https://doc.rust-lang.org/nightly/cargo/reference/timings.html, though it is not necessary for this pull request. Anyway, thanks for the contribution!

@bors r+

bors · 2023-08-01T13:32:21Z

📌 Commit 28674b9 has been approved by weihanglo

It is now in the queue for this repository.

bors · 2023-08-01T13:32:27Z

⌛ Testing commit 28674b9 with merge 63ec5e2...

weihanglo · 2023-08-01T13:36:48Z

But I'm still getting some duplicate behavior, apparently we are compiling the same crate twice? Or it's showing on the graph twice and I'm not sure why.

Anything different in the Unit can contribute to the duplicates. For examples, build script dependencies have different profile settings than normal runtime dependencies. And different "CompileMode" would also make it looks like a duplicate.

cargo/src/cargo/core/compiler/unit.rs

Lines 37 to 85 in c91a693

    
           /// Internal fields of `Unit` which `Unit` will dereference to. 
        
           #[derive(Clone, Hash, PartialEq, Eq, PartialOrd, Ord)] 
        
           pub struct UnitInner { 
        
               /// Information about available targets, which files to include/exclude, etc. Basically stuff in 
        
               /// `Cargo.toml`. 
        
               pub pkg: Package, 
        
               /// Information about the specific target to build, out of the possible targets in `pkg`. Not 
        
               /// to be confused with *target-triple* (or *target architecture* ...), the target arch for a 
        
               /// build. 
        
               pub target: Target, 
        
               /// The profile contains information about *how* the build should be run, including debug 
        
               /// level, etc. 
        
               pub profile: Profile, 
        
               /// Whether this compilation unit is for the host or target architecture. 
        
               /// 
        
               /// For example, when 
        
               /// cross compiling and using a custom build script, the build script needs to be compiled for 
        
               /// the host architecture so the host rustc can use it (when compiling to the target 
        
               /// architecture). 
        
               pub kind: CompileKind, 
        
               /// The "mode" this unit is being compiled for. See [`CompileMode`] for more details. 
        
               pub mode: CompileMode, 
        
               /// The `cfg` features to enable for this unit. 
        
               /// This must be sorted. 
        
               pub features: Vec<InternedString>, 
        
               // if `true`, the dependency is an artifact dependency, requiring special handling when 
        
               // calculating output directories, linkage and environment variables provided to builds. 
        
               pub artifact: IsArtifact, 
        
               /// Whether this is a standard library unit. 
        
               pub is_std: bool, 
        
               /// A hash of all dependencies of this unit. 
        
               /// 
        
               /// This is used to keep the `Unit` unique in the situation where two 
        
               /// otherwise identical units need to link to different dependencies. This 
        
               /// can happen, for example, when there are shared dependencies that need 
        
               /// to be built with different features between normal and build 
        
               /// dependencies. See `rebuild_unit_graph_shared` for more on why this is 
        
               /// done. 
        
               /// 
        
               /// This value initially starts as 0, and then is filled in via a 
        
               /// second-pass after all the unit dependencies have been computed. 
        
               pub dep_hash: u64, 
        
               /// This is used for target-dependent feature resolution and is copied from 
        
               /// [`FeaturesFor::ArtifactDep`], if the enum matches the variant. 
        
               /// 
        
               /// [`FeaturesFor::ArtifactDep`]: crate::core::resolver::features::FeaturesFor::ArtifactDep 
        
               pub artifact_target_for_features: Option<CompileTarget>, 
        
           }

epage · 2023-08-01T14:15:30Z

Thanks for working with us on this!

bors · 2023-08-01T14:32:06Z

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 63ec5e2 to master...

Angelin01 · 2023-08-01T14:32:56Z

No problem! This was quite an easy process. Thanks for the reviews and iteration on the PR! I'll contribute again when I can.

Update cargo 8 commits in c91a693e7977e33a1064b63a5daf5fb689f01651..6dc1deaddf62c7748c9097c7ea88e9ec77ff1a1a 2023-07-31 00:26:46 +0000 to 2023-08-02 00:23:54 +0000 - `#[allow(internal_features)]` in RUSTC_BOOTSTRAP test (rust-lang/cargo#12429) - ci: rewrite bump check and respect semver (rust-lang/cargo#12395) - fix(update): Tweak CLI behavior (rust-lang/cargo#12428) - chore(deps): update compatible (rust-lang/cargo#12426) - Display crate version on timings graph (rust-lang/cargo#12420) - chore(deps): update alpine docker tag to v3.18 (rust-lang/cargo#12427) - Use thiserror for credential provider errors (rust-lang/cargo#12424) - Clarify in `--help` that `cargo test --all-targets` excludes doctests (rust-lang/cargo#12422) r? `@ghost`

Update cargo 8 commits in c91a693e7977e33a1064b63a5daf5fb689f01651..6dc1deaddf62c7748c9097c7ea88e9ec77ff1a1a 2023-07-31 00:26:46 +0000 to 2023-08-02 00:23:54 +0000 - `#[allow(internal_features)]` in RUSTC_BOOTSTRAP test (rust-lang/cargo#12429) - ci: rewrite bump check and respect semver (rust-lang/cargo#12395) - fix(update): Tweak CLI behavior (rust-lang/cargo#12428) - chore(deps): update compatible (rust-lang/cargo#12426) - Display crate version on timings graph (rust-lang/cargo#12420) - chore(deps): update alpine docker tag to v3.18 (rust-lang/cargo#12427) - Use thiserror for credential provider errors (rust-lang/cargo#12424) - Clarify in `--help` that `cargo test --all-targets` excludes doctests (rust-lang/cargo#12422) r? ``@ghost``

Update cargo 10 commits in c91a693e7977e33a1064b63a5daf5fb689f01651..020651c52257052d28f6fd83fbecf5cfa1ed516c 2023-07-31 00:26:46 +0000 to 2023-08-02 16:00:37 +0000 - Update rustix to 0.38.6 (rust-lang/cargo#12436) - replace `master` branch by default branch in documentation (rust-lang/cargo#12435) - `#[allow(internal_features)]` in RUSTC_BOOTSTRAP test (rust-lang/cargo#12429) - ci: rewrite bump check and respect semver (rust-lang/cargo#12395) - fix(update): Tweak CLI behavior (rust-lang/cargo#12428) - chore(deps): update compatible (rust-lang/cargo#12426) - Display crate version on timings graph (rust-lang/cargo#12420) - chore(deps): update alpine docker tag to v3.18 (rust-lang/cargo#12427) - Use thiserror for credential provider errors (rust-lang/cargo#12424) - Clarify in `--help` that `cargo test --all-targets` excludes doctests (rust-lang/cargo#12422) r? `@ghost`

rustbot assigned epage Jul 31, 2023

rustbot added A-timings Area: timings S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 31, 2023

epage reviewed Jul 31, 2023

View reviewed changes

weihanglo approved these changes Aug 1, 2023

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 1, 2023

bors added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 1, 2023

Disambiguate duplicate unit names using versions

28674b9

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. labels Aug 1, 2023

bors merged commit 63ec5e2 into rust-lang:master Aug 1, 2023
18 checks passed

Angelin01 deleted the timings-show-version branch August 1, 2023 14:32

weihanglo mentioned this pull request Aug 2, 2023

Update cargo rust-lang/rust#114364

Merged

ehuss added this to the 1.73.0 milestone Aug 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Display crate version on timings graph #12420

Display crate version on timings graph #12420

Angelin01 commented Jul 31, 2023

rustbot commented Jul 31, 2023

epage Jul 31, 2023

Angelin01 Jul 31, 2023

epage Jul 31, 2023

This comment was marked as duplicate.

weihanglo Jul 31, 2023

Angelin01 Jul 31, 2023 •

edited

Loading

epage Jul 31, 2023

Angelin01 Jul 31, 2023 •

edited

Loading

weihanglo Jul 31, 2023

weihanglo left a comment

weihanglo commented Aug 1, 2023

bors commented Aug 1, 2023

bors commented Aug 1, 2023

weihanglo commented Aug 1, 2023

bors commented Aug 1, 2023

Angelin01 commented Aug 1, 2023

weihanglo commented Aug 1, 2023

bors commented Aug 1, 2023

bors commented Aug 1, 2023

weihanglo commented Aug 1, 2023

epage commented Aug 1, 2023

bors commented Aug 1, 2023

Angelin01 commented Aug 1, 2023

Display crate version on timings graph #12420

Display crate version on timings graph #12420

Conversation

Angelin01 commented Jul 31, 2023

What does this PR try to resolve?

How should we test and review this PR?

Additional information

Possible issues and future work

rustbot commented Jul 31, 2023

epage Jul 31, 2023

Choose a reason for hiding this comment

Angelin01 Jul 31, 2023

Choose a reason for hiding this comment

epage Jul 31, 2023

Choose a reason for hiding this comment

This comment was marked as duplicate.

weihanglo Jul 31, 2023

Choose a reason for hiding this comment

Angelin01 Jul 31, 2023 • edited Loading

Choose a reason for hiding this comment

epage Jul 31, 2023

Choose a reason for hiding this comment

Angelin01 Jul 31, 2023 • edited Loading

Choose a reason for hiding this comment

weihanglo Jul 31, 2023

Choose a reason for hiding this comment

weihanglo left a comment

Choose a reason for hiding this comment

weihanglo commented Aug 1, 2023

bors commented Aug 1, 2023

bors commented Aug 1, 2023

weihanglo commented Aug 1, 2023

bors commented Aug 1, 2023

Angelin01 commented Aug 1, 2023

weihanglo commented Aug 1, 2023

bors commented Aug 1, 2023

bors commented Aug 1, 2023

weihanglo commented Aug 1, 2023

epage commented Aug 1, 2023

bors commented Aug 1, 2023

Angelin01 commented Aug 1, 2023

Angelin01 Jul 31, 2023 •

edited

Loading

Angelin01 Jul 31, 2023 •

edited

Loading