Skip to content
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

azure: split slow linux builders #61370

Closed
wants to merge 6 commits into from

Conversation

pietroalbini
Copy link
Member

@pietroalbini pietroalbini commented May 30, 2019

This PR splits into two the 6 Linux builders that regularly approach or go over the 3 hours mark on Azure, using the make ci-subset-{1,2} already used by other builders on Windows.

r? @alexcrichton
cc #61185

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 30, 2019
@rust-highfive

This comment has been minimized.

@pietroalbini
Copy link
Member Author

Fixed the build failure.

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm somewhat tempted to hold off on this if we can, it seems like it's going to be hard to manage what's split and what isn't over time. Additionally in terms of resources this is wasteful in the sense that the split builds have the same first halves.

I think we should probably first explore disabling assertions where possible and see if we can recover enough time that way. Distcheck for example has no need to turn on assertions, same with builders like cargo. I was hoping to look more into this today but it's looking like I may not have enough time today

x86_64-gnu-aux:
IMAGE: x86_64-gnu-aux
SCRIPT: make check-aux EXCLUDE_CARGO=1

x86_64-gnu-cargo:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did we forget to include this from before?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, on Travis x86_64-gnu-cargo is included inside x86_64-gnu-aux. Instead on AppVeyor, to speed things up, cargo is tested inside its own job. I replicated the same thing we do on AppVeyor in Azure Linux.

@pietroalbini
Copy link
Member Author

Ok, let's wait on the results of disabling assertions.

@rustbot modify labels: +S-blocked -S-waiting-on-review

@rustbot rustbot added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 1, 2019
@alexcrichton
Copy link
Member

Ok so to say again that I haven't forgotten about this:

I'll circle back tomorrow with timings after that all runs overnight.

@alexcrichton
Copy link
Member

Ok got some good builds, although there's some outliers we've got a good set of data:

https://gist.github.com/alexcrichton/4350118b27fa2ae356cfde3d21f6e45d

The build with assertions had to rebuild some containers (namely the huge linux ones) so a few 6 hour builds are outliers. Relative times are relative to the first travis/appveyor column.

Some things:

  • Something feels wrong about builders like i686-gnu. 4 cores on Travis is equivalent to 2 cores on Azure. The CPUs don't even look that far off from one another there (same clock rates). I think we really need to plumb through @johnterickson's script to get CPU usage over time across all builders.
  • Same-host dist builds are all through the roof on AppVeyor. We're looking at a blanket 20 minute slowdown up to 40 minutes for dist-x86_64-musl
  • Disabling assertions has a massive impact on Windows tests, often saving 30-40 minutes. (x86_64-mingw-2)
  • Some windows tests remain stubborn (even after assertions are disabled) and execute for quite some time. (i686-msvc-2 and i686-mingw-2)

As for builders affected by this PR specifically:

Travis CI/AppVeyor Azure Pipelines (assertions) Azure Pipelines (no assertions)
i686-gnu 2:28:47 3:19:05 (+0:50:18) 2:29:24 (+0:00:37)
i686-gnu-nopt 2:30:52 3:08:23 (+0:37:31) 2:21:25 (-0:09:27)
x86_64-gnu 2:25:52 2:53:57 (+0:28:05) 2:27:42 (+0:01:50)
x86_64-gnu-nopt 2:10:02 3:08:37 (+0:58:35) 2:25:52 (+0:15:50)
x86_64-gnu-distcheck 2:26:33 3:05:53 (+0:39:20) 2:28:32 (+0:01:59)

It looks like assertions makes up the lion's share of time differences for these builders, except maybe the x86_64-gnu-nopt one which may benefit from splitting.


The conclusions I'm drawing here are that we should disable LLVM/debug assertions by default on CI since we can't afford them and get such huge wins without them. We should still have a builder or two with them enabled, but perhaps don't run the full test suite and just a few things here and there. (also across platforms to cover OSX/Linux/MSVC at least).

Additionally we probably don't want to start the splitting as proposed in this PR just yet. I think disabling assertions gets us almost the way there. The last major hurdle will be getting the dist builders under control, which I'll be taking a look into more shortly.

@bors
Copy link
Contributor

bors commented Jun 13, 2019

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

@pietroalbini pietroalbini added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Jun 25, 2019
@pietroalbini
Copy link
Member Author

Rebased, and also disabled Windows builders on auto and Windows and macOS builders on try.

r? @alexcrichton

@alexcrichton
Copy link
Member

I talked with @pietroalbini on discord briefly about this, but I think we may not want to land this just yet. In our transition plan to Azure I think we're just going to eat a regression in compile times until the 4 core machines come online, and we're transitioning everything to Azure so speeding up some of the builders here won't translate to build time wins as long as at least one builder is lagging behind. I think the try changes can land at any time, but I've also included those in #62142 since they're required there as well.

@pietroalbini
Copy link
Member Author

Yeah, closing this.

@pietroalbini pietroalbini deleted the split-builders branch July 1, 2019 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants