-
Notifications
You must be signed in to change notification settings - Fork 13k
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
ci: Print out how long each step takes on CI #49094
Conversation
r? @aturon (rust_highfive has picked a reviewer for you, use r? to override) |
r? @kennytm |
src/bootstrap/bin/rustc.rs
Outdated
fn exec_cmd(cmd: &mut Command) -> ::std::io::Result<ExitStatus> { | ||
cmd.status() | ||
fn exec_cmd(cmd: &mut Command) -> io::Result<i32> { | ||
cmd.status().code().unwrap() |
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.
unwrap? 😕 But it is returning a 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.
Oh oops, code().unwrap()
on the wrong object
(on Windows code()
always returns Some
)
8b7555c
to
d287e78
Compare
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.
Could you temporarily enable --set build.print-step-timings
everywhere so we could see its effect before merging?
config.toml.example
Outdated
@@ -182,6 +182,10 @@ | |||
# essentially skipping stage0 as the local compiler is recompiling itself again. | |||
#local-rebuild = false | |||
|
|||
# Print out how long each rustbuild step took in JSON format (mostly intended |
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.
Those println!
output doesn't look like JSON at all 😄
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 also prints both how long rustc takes to compile each crate and how long each step takes; I'd sort of expect to see this documented here?
It's also a bit odd that rustc prints to stderr whereas the step timing is printed to stdout.
Is there an example log from this up somewhere? I went to continue playing with analyzing it but the travis job for this didn't seem to have any RUSTC-TIMING lines... |
@@ -25,6 +25,8 @@ source "$ci_dir/shared.sh" | |||
|
|||
if [ "$TRAVIS" == "true" ] && [ "$TRAVIS_BRANCH" != "auto" ]; then | |||
RUST_CONFIGURE_ARGS="$RUST_CONFIGURE_ARGS --enable-quiet-tests" | |||
else | |||
RUST_CONFIGURE_ARGS="$RUST_CONFIGURE_ARGS --set build.print-step-timings" |
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.
Why not unconditionally enable this for all CI? i.e. put it down with the enable-sccache lines below.
d287e78
to
0d7fec7
Compare
Ok I've pushed a version that should enable the timing printing on the current CI run it's doing. (cc @kennytm / @scottmcm) @aidanhs the output is pretty noisy and I figured that for PR builds where lots of people are looking at them it's probably best to make it quieter by default (and we're less worried about timing there anyway) |
210accf
to
a30e9a9
Compare
re-r? @kennytm (took out the printing) |
@alexcrichton Unfortunately the version at #49094 (comment) failed tidy due to FIXME vs TODO, so we can't see the complete picture 😄. Could you push another version which can run all tests? 🙂 Some observations:
|
Sure thing, the full build should hopefully show up here The two |
Timing result:
Several observations.
|
Hmm, I worry that just using Example: two crates, each with 1s of single-core work and 2s of parallelizable work. If they run serially on a two-core machine, they'll have an instant timing of 2s each, but the total job will take 4s. If they run in parallel, they'll look slower, with an instant timing of 3s each, but the total compilation will finish faster -- just 3s. The prototype handled this by recording start and finish timestamps so it could give discounts for things running in parallel, but that's even more spammy. Maybe record the end timestamp so the overlaps can be calculated in the script? That way would be compatible with omitting the <0.1s tasks. |
We can probably use intrinsics::type_name to disambiguate since that gives the whole path. It's somewhat painful to use an intrinsic but seems fine for now, IMO. |
@kennytm certainly! Seems reasonable to me to cut down on the noise @scottmcm oh I wasn't thinking we'd just sum up times but rather use this for statistical tracking over time. In theory steps themselves are relatively stable over time (as should crate compilations), but if we see something spike it should be relatively easy to drill into "which step had the highest variance" and we take a look at that. In that sense I think CPU utilization will show us how well we're using all the cores rather than the timing information. |
This commit updates CI configuration to inform rustbuild that it should print out how long each step takes on CI. This'll hopefully allow us to track the duration of steps over time and follow regressions a bit more closesly (as well as have closer analysis of differences between two builds). cc rust-lang#48829
fe6ab5c
to
8ce800d
Compare
8ce800d
to
1b5eb17
Compare
The most recent run with everything less than 100ms filtered out, and otherwise, this should be ready to go |
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.
r=me except one thing to confirm
@@ -25,6 +25,8 @@ source "$ci_dir/shared.sh" | |||
|
|||
if [ "$TRAVIS" == "true" ] && [ "$TRAVIS_BRANCH" != "auto" ]; then |
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 believe TRAVIS
is false
on AppVeyor. We should generalize this condition to cover AppVeyor as well.
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.
Indeed it is! This is only here, though, for making PR builds a little less noisy. We only do full bors builds on AppVeyor so I think I'll leave this for now until that situation changes
@bors: r=kennytm |
📌 Commit 1b5eb17 has been approved by |
@bors p=1 Might be nice to have at least some timings by all hands... |
@bors p=5 |
ci: Print out how long each step takes on CI This commit updates CI configuration to inform rustbuild that it should print out how long each step takes on CI. This'll hopefully allow us to track the duration of steps over time and follow regressions a bit more closesly (as well as have closer analysis of differences between two builds). cc #48829
☀️ Test successful - status-appveyor, status-travis |
This commit updates CI configuration to inform rustbuild that it should print
out how long each step takes on CI. This'll hopefully allow us to track the
duration of steps over time and follow regressions a bit more closesly (as well
as have closer analysis of differences between two builds).
cc #48829