refactor(timings): reuse timing metric collection logic between `--timings` and `-Zbuild-analysis` #16497

motorailgun · 2026-01-11T14:13:53Z

What this PR addresses

As described in the issue above, cargo build --timings and cargo report timings each had its own metric collection logic. Because cargo report timings will supersede cargo build --timings, this PR try to remove redundant logics from core/compiler/timings/mod.rs.

Review points

I'm not sure if it's appropriate to modify logger for refactoring. However, logs emitted inside core/compiler/timings/mod.rs alone is not enough for prepare_context_from_iter() to properly replay the build so I think this is an appropriate way.

…ings and -Zbuild-analysis

rustbot · 2026-01-12T15:21:41Z

r? @weihanglo

rustbot has assigned @weihanglo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

weihanglo · 2026-01-12T15:26:43Z

I'm not sure if it's appropriate to modify logger for refactoring. However, logs emitted inside core/compiler/timings/mod.rs alone is not enough for prepare_context_from_iter() to properly replay the build so I think this is an appropriate way.

The overall direction looks reasonable to me!

motorailgun · 2026-01-12T15:27:42Z

Please see #16474 (comment), since they are already assigned to the parent issue, I have no objection to handing this PR over to @kurasaiteja (of course if they want).

weihanglo · 2026-01-12T21:30:58Z

src/cargo/ops/cargo_compile/mod.rs

-    if let Some(logger) = logger {
-        let root_unit_indexes: HashSet<_> =
-            root_units.iter().map(|unit| unit_to_index[&unit]).collect();
+    let root_unit_indexes: HashSet<_> =


This should move into the if block at line 537.

Oh right, thank you!

weihanglo · 2026-01-12T21:31:55Z

src/cargo/ops/cargo_report/timings.rs

-fn prepare_context(log: &Path, run_id: &RunId) -> CargoResult<RenderContext<'static>> {
-    let reader = BufReader::new(File::open(&log)?);
-
+// FIXME: simply the types here once `cargo build --timings` support is dropped


The --timings flag will stay forever. I don't think we'll drop it.

Wasn't aware of that 👀

weihanglo · 2026-01-12T21:32:35Z

src/cargo/ops/cargo_report/timings.rs

-    let reader = BufReader::new(File::open(&log)?);
-
+// FIXME: simply the types here once `cargo build --timings` support is dropped
+pub(crate) fn prepare_context_from_iter<T: Iterator<Item = LogMessage>>(


Some nits

Can just keep calling it prepare_context.

I would probable prefer where I: Iterator if that geenric is a bit too long

weihanglo · 2026-01-12T21:35:59Z

src/cargo/util/logger.rs


-        let (tx, rx) = mpsc::channel::<LogMessage>();
+/// Logger for `-Zbuild-analysis`.
+// fixme: simplify this struct once `cargo build --timings` support dropped


Ditto. It will be supported like forever.

weihanglo · 2026-01-12T21:38:38Z

src/cargo/util/logger.rs

+// for legacy `cargo build --timings` flag
+struct InMemoryLogger {
+    // using mutex to hide mutability
+    logs: Mutex<Vec<LogMessage>>,


Using a Mutex here seems a little bit unfortunate. I think it is safe to use RefCell here to reduce the performance overhead, as timing appears to be on mainthread only.

weihanglo · 2026-01-12T21:42:17Z

src/cargo/core/compiler/fingerprint/mod.rs

-            cause,
-        });
-    }
+    let index = bcx.unit_to_index[unit];


shouldn't we check if bcx.logger.i_senabled() first then construct log message?

At this point, I think it is better to leave logger as Option so that pre-existing check codes doesn't need to be changed. Will fix in such way!

weihanglo · 2026-01-12T21:44:29Z

src/cargo/core/compiler/timings/mod.rs

    /// When Cargo started.
    start: Instant,
    /// A rendered string of when compilation started.
    start_str: String,
    /// A summary of the root units.
    ///
-    /// Tuples of `(package_description, target_descriptions)`.
-    root_targets: Vec<(String, Vec<String>)>,
-    /// The build profile.
-    profile: String,
    /// Total number of fresh units.
    total_fresh: u32,
    /// Total number of dirty units.
    total_dirty: u32,
    /// A map from unit to index.
    unit_to_index: HashMap<Unit, UnitIndex>,
    /// Time tracking for each individual unit.
    unit_times: Vec<UnitTime>,


This can all be dropped I feel like? The only thing we'll track here is CPU usage (and perhaps active)

weihanglo · 2026-01-12T21:46:46Z

src/cargo/core/compiler/timings/mod.rs

+            elapsed,
+            unblocked,
+        });
        self.unit_times.push(unit_time);


I feel like the entire UnitTime struct could be removed. Did you see a reason keeping it?

weihanglo · 2026-01-12T21:48:30Z

src/cargo/core/compiler/timings/mod.rs

            let timestamp = self.start_str.replace(&['-', ':'][..], "");
            let timings_path = build_runner
                .files()
                .timings_dir()
                .expect("artifact-dir was not locked");
            paths::create_dir_all(&timings_path)?;
            let filename = timings_path.join(format!("cargo-timing-{}.html", timestamp));


We could probably change this file name to run-id name, just like how cargo report timings generate the report file name.

rustbot added A-build-execution Area: anything dealing with executing the compiler A-rebuild-detection Area: rebuild detection and fingerprinting A-timings Area: timings Command-report labels Jan 11, 2026

refactor(timings): reuse timing metric collection logic between --tim…

b2dc1e1

…ings and -Zbuild-analysis

motorailgun force-pushed the refactor-timings branch from 7530028 to b2dc1e1 Compare January 12, 2026 15:19

motorailgun marked this pull request as ready for review January 12, 2026 15:21

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 12, 2026

rustbot assigned weihanglo Jan 12, 2026

weihanglo reviewed Jan 12, 2026

View reviewed changes

refactor(timings): reuse timing metric collection logic between --timings and -Zbuild-analysis #16497

Are you sure you want to change the base?

refactor(timings): reuse timing metric collection logic between --timings and -Zbuild-analysis #16497

Uh oh!

Conversation

motorailgun commented Jan 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR addresses

Review points

Uh oh!

rustbot commented Jan 12, 2026

Uh oh!

weihanglo commented Jan 12, 2026

Uh oh!

motorailgun commented Jan 12, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weihanglo Jan 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

refactor(timings): reuse timing metric collection logic between `--timings` and `-Zbuild-analysis` #16497

refactor(timings): reuse timing metric collection logic between `--timings` and `-Zbuild-analysis` #16497

motorailgun commented Jan 11, 2026 •

edited

Loading

weihanglo Jan 12, 2026 •

edited

Loading