-
Notifications
You must be signed in to change notification settings - Fork 13.4k
Implement initial support for timing sections (--json=timings
)
#142123
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
let kind = match kind { | ||
TimingSectionKind::Start => "start", | ||
TimingSectionKind::End => "end", | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be a method on TimingSectionKind
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, but the TimingSectionKind
is a shared data structure for all emitters, and this form of output is specific for JSON. It should IMO live here, in theory we could emit the timings also in other formats. It also avoids having to go to a different file to see how the output format looks like.
I'm fine with moving it to a function in the json
module, but I don't think it belongs as a method on the section kind.
}; | ||
let name = match section { | ||
TimingSection::Linking => "link", | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto, for TimingSection
.
Seems reasonable as a starting point. I feel like hierarchy is inevitable -- every profiler I've ever written has ended up with it. |
I agree. Are you fine with the "implicit hierarchy", which then has to be reconstructed by the user reading the JSON events? |
Sure. Keeping it simple to start with sounds good, given that it's somewhat unclear exactly how this data will be used. r=me when you're ready. |
Added documentation and fixed a few review remarks. |
compiler/rustc_errors/src/json.rs
Outdated
let name = match section { | ||
TimingSection::Linking => "link", | ||
}; | ||
let timestamp = SystemTime::now() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should use Instant rather than SystemTime. Using the latter causes issues when changing the system time during a build, for example because your ntp daemon finally got an internet connection. And IMHO it should preferrably use CLOCK_MONOTONIC rather than CLOCK_BOOTTIME as it doesn't make sense to count time where the system is suspended in performance profiles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already use system time for some incr dir names, I think, but you're right that for profiling Instant makes more sense. But then I have to count the duration from some origin time again. Before it was from the creation of the JSON emitter, but that had some issues w.r.t. multiple emitters existing. Should I just take a snapshot when rustc starts or something? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right and Instant
doesn't provide a way to serialize it. Maybe call clock_gettime(CLOCK_MONOTONIC)
(unix) or QueryPerformanceCounter
(windows) directly and return the raw result?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uhh, I would really like to avoid dealing with OS specific code here, especially since the main use-case for this right now is Cargo, which will most likely ignore the timestamp field anyway, because it streams the output and just uses current time when an event is generated for the timestamp. I'll just go back to instant, but this take count it from the start of the compilation session.
☔ The latest upstream changes (presumably #142550) made this pull request unmergeable. Please resolve the merge conflicts. |
Rebased. I made some changes based on the discussion in review comments around the usage of @rustbot ready |
r=me with the nits above addressed. I figure this output will need to be used in earnest by some external tool before we really know what the exact format should be. |
Definitely agreed. I plan to integrate this into Cargo once this PR is merged. @bors r=nnethercote |
Implement initial support for timing sections (`--json=timings`) This PR implements initial support for emitting high-level compilation section timings. The idea is to provide a very lightweight way of emitting durations of various compilation sections (frontend, backend, linker, or on a more granular level macro expansion, typeck, borrowck, etc.). The ultimate goal is to stabilize this output (in some form), make Cargo pass `--json=timings` and then display this information in the HTML output of `cargo build --timings`, to make it easier to quickly profile "what takes so long" during the compilation of a Cargo project. I would personally also like if Cargo printed some of this information in the interactive `cargo build` output, but the `build --timings` use-case is the main one. Now, this information is already available with several other sources, but I don't think that we can just use them as they are, which is why I proposed a new way of outputting this data (`--json=timings`): - This data is available under `-Zself-profile`, but that is very expensive and forever unstable. It's just a too big of a hammer to tell us the duration it took to run the linker. - It could also be extracted with `-Ztime-passes`. That is pretty much "for free" in terms of performance, and it can be emitted in a structured form to JSON via `-Ztime-passes-format=json`. I guess that one alternative might be to stabilize this flag in some form, but that form might just be `--json=timings`? I guess what we could do in theory is take the already emitted time passes and reuse them for `--json=timings`. Happy to hear suggestions! I'm sending this PR mostly for a vibeck, to see if the way I implemented it is passable. There are some things to figure out: - How do we represent the sections? Originally I wanted to output `{ section, duration }`, but then I realized that it might be more useful to actually emit `start` and `end` events. Both because it enables to see the output incrementally (in case compilation takes a long time and you read the outputs directly, or Cargo decides to show this data in `cargo build` some day in the future), and because it makes it simpler to represent hierarchy (see below). The timestamps currently emit microseconds elapsed from a predetermined point in time (~start of rustc), but otherwise they are fully opaque, and should be only ever used to calculate the duration using `end - start`. We could also precompute the duration for the user in the `end` event, but that would require doing more work in rustc, which I would ideally like to avoid :P - Do we want to have some form of hierarchy? I think that it would be nice to show some more granular sections rather than just frontend/backend/linker (e.g. macro expansion, typeck and borrowck as a part of the frontend). But for that we would need some way of representing hierarchy. A simple way would be something like `{ parent: "frontend" }`, but I realized that with start/end timestamps we get the hierarchy "for free", only the client will need to reconstruct it from the order of start/end events (e.g. `start A`, `start B` means that `B` is a child of `A`). - What exactly do we want to stabilize? This is probably a question for later. I think that we should definitely stabilize the format of the emitted JSON objects, and *maybe* some specific section names (but we should also make it clear that they can be missing, e.g. you don't link everytime you invoke `rustc`). The PR be tested e.g. with `rustc +stage1 src/main.rs --json=timings --error-format=json -Zunstable-options` on a crate without dependencies (it is not easy to use `--json` with stock Cargo, because it also passes this flag to `rustc`, so this will later need Cargo integration to be usable with it). Zulip discussions: [#t-compiler > Outputting time spent in various compiler sections](https://rust-lang.zulipchat.com/#narrow/channel/131828-t-compiler/topic/Outputting.20time.20spent.20in.20various.20compiler.20sections/with/518850162) MCP: rust-lang/compiler-team#873 r? `@nnethercote`
Rollup of 12 pull requests Successful merges: - #135656 (Add `-Z hint-mostly-unused` to tell rustc that most of a crate will go unused) - #140774 (Affirm `-Cforce-frame-pointers=off` does not override) - #141610 (Stabilize `feature(generic_arg_infer)`) - #142123 (Implement initial support for timing sections (`--json=timings`)) - #142383 (CodeGen: rework Aggregate implemention for rvalue_creates_operand cases) - #142502 (rustdoc_json: improve handling of generic args) - #142591 (Add spawn APIs for BootstrapCommand to support deferred command execution) - #142606 (AsyncDrop trait without sync Drop generates an error) - #142619 (apply clippy::or_fun_call) - #142624 (Actually take `--build` into account in bootstrap) - #142627 (Add `StepMetadata` to describe steps) - #142660 (remove joboet from review rotation) r? `@ghost` `@rustbot` modify labels: rollup
This PR implements initial support for emitting high-level compilation section timings. The idea is to provide a very lightweight way of emitting durations of various compilation sections (frontend, backend, linker, or on a more granular level macro expansion, typeck, borrowck, etc.). The ultimate goal is to stabilize this output (in some form), make Cargo pass
--json=timings
and then display this information in the HTML output ofcargo build --timings
, to make it easier to quickly profile "what takes so long" during the compilation of a Cargo project. I would personally also like if Cargo printed some of this information in the interactivecargo build
output, but thebuild --timings
use-case is the main one.Now, this information is already available with several other sources, but I don't think that we can just use them as they are, which is why I proposed a new way of outputting this data (
--json=timings
):-Zself-profile
, but that is very expensive and forever unstable. It's just a too big of a hammer to tell us the duration it took to run the linker.-Ztime-passes
. That is pretty much "for free" in terms of performance, and it can be emitted in a structured form to JSON via-Ztime-passes-format=json
. I guess that one alternative might be to stabilize this flag in some form, but that form might just be--json=timings
? I guess what we could do in theory is take the already emitted time passes and reuse them for--json=timings
. Happy to hear suggestions!I'm sending this PR mostly for a vibeck, to see if the way I implemented it is passable. There are some things to figure out:
{ section, duration }
, but then I realized that it might be more useful to actually emitstart
andend
events. Both because it enables to see the output incrementally (in case compilation takes a long time and you read the outputs directly, or Cargo decides to show this data incargo build
some day in the future), and because it makes it simpler to represent hierarchy (see below). The timestamps currently emit microseconds elapsed from a predetermined point in time (~start of rustc), but otherwise they are fully opaque, and should be only ever used to calculate the duration usingend - start
. We could also precompute the duration for the user in theend
event, but that would require doing more work in rustc, which I would ideally like to avoid :P{ parent: "frontend" }
, but I realized that with start/end timestamps we get the hierarchy "for free", only the client will need to reconstruct it from the order of start/end events (e.g.start A
,start B
means thatB
is a child ofA
).rustc
).The PR be tested e.g. with
rustc +stage1 src/main.rs --json=timings --error-format=json -Zunstable-options
on a crate without dependencies (it is not easy to use--json
with stock Cargo, because it also passes this flag torustc
, so this will later need Cargo integration to be usable with it).Zulip discussions: #t-compiler > Outputting time spent in various compiler sections
MCP: rust-lang/compiler-team#873
r? @nnethercote