Skip to content

Cache cargo binaries #4659

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 17, 2019
Merged

Cache cargo binaries #4659

merged 1 commit into from
Oct 17, 2019

Conversation

@tesuji
Copy link
Contributor

@tesuji tesuji commented Oct 12, 2019

changelog: none

@tesuji tesuji marked this pull request as ready for review October 12, 2019 14:03
@tesuji tesuji changed the title Try caching cargo install binaries [WIP] Try caching cargo install binaries Oct 12, 2019
@tesuji tesuji force-pushed the caching branch 2 times, most recently from f23309a to 2b4e9ae Compare October 12, 2019 17:08
@tesuji tesuji changed the title [WIP] Try caching cargo install binaries Cache cargo binaries Oct 12, 2019
@tesuji
Copy link
Contributor Author

tesuji commented Oct 12, 2019

This should cut build time in half.
r? @flip1995

tesuji
tesuji commented Oct 13, 2019
View reviewed changes
@tesuji
Copy link
Contributor Author

tesuji commented Oct 13, 2019

Hm, cannot install master toolchain on osx. Investigating!

@tesuji
Copy link
Contributor Author

tesuji commented Oct 13, 2019

Reported upstream: kennytm/rustup-toolchain-install-master#28

@flip1995
Copy link
Member

flip1995 commented Oct 13, 2019

I also tried this once. But the caches this builds are >500MB big. I'm not sure if this is a trade-off we want to / can take.

@bors
Copy link
Contributor

bors commented Oct 14, 2019

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

@bors
Copy link
Contributor

bors commented Oct 15, 2019

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

@tesuji tesuji force-pushed the caching branch 2 times, most recently from b3f0fa6 to 49fabde Compare October 16, 2019 04:50
@tesuji
Use CI caching and cargo feature install-upgrade
d81191e 
* do not force to install rustfmt
* use cargo-cache
* disable bash trace
* clone single branch
@phansch
Copy link
Contributor

phansch commented Oct 17, 2019

@bors try

bors added a commit that referenced this pull request Oct 17, 2019
@bors
Auto merge of #4659 - lzutao:caching, r=<try>
011e46b 
Cache cargo binaries

changelog: none
@bors
Copy link
Contributor

bors commented Oct 17, 2019

⌛ Trying commit d81191e with merge 011e46b...

@phansch
Copy link
Contributor

phansch commented Oct 17, 2019

So, we should be able to easily test this: The first try run is using an empty cache, because there's no cache for the try branch yet:

attempting to download cache archive
fetching try/cache-osx-7170efeb401a81a9aa99b33477d640a2606564208e3f17dea84058061c3ec1a5.tgz
fetching master/cache-osx-7170efeb401a81a9aa99b33477d640a2606564208e3f17dea84058061c3ec1a5.tgz
could not download cache

(https://travis-ci.com/rust-lang/rust-clippy/jobs/246547175#L71)

The second try run should be faster then

@phansch phansch added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 17, 2019
@bors
Copy link
Contributor

bors commented Oct 17, 2019

☀️ Try build successful - checks-travis, status-appveyor
Build commit: 011e46b (011e46b7fd256a176159328d7975ea2be37368c8)

@phansch
Copy link
Contributor

phansch commented Oct 17, 2019

Full cache upload took 15-20 seconds in the various jobs.

@bors try

@bors
Copy link
Contributor

bors commented Oct 17, 2019

⌛ Trying commit d81191e with merge e8c160b...

bors added a commit that referenced this pull request Oct 17, 2019
@bors
Auto merge of #4659 - lzutao:caching, r=<try>
e8c160b 
Cache cargo binaries

changelog: none
@phansch
Copy link
Contributor

phansch commented Oct 17, 2019

The cache download seems to take less than 10 seconds in every case. Less than 5 in most cases. Let's see the total time savings ⏳

@bors
Copy link
Contributor

bors commented Oct 17, 2019

☀️ Try build successful - checks-travis, status-appveyor
Build commit: e8c160b (e8c160b7a793e5a0e2c555d33e944549c01608fc)

@phansch
Copy link
Contributor

phansch commented Oct 17, 2019

The second try run was ~5 minutes faster. Not sure if that's within the normal variance. Let's try again

@bors try

@bors
Copy link
Contributor

bors commented Oct 17, 2019

⌛ Trying commit d81191e with merge 2d416cb...

bors added a commit that referenced this pull request Oct 17, 2019
@bors
Auto merge of #4659 - lzutao:caching, r=<try>
2d416cb 
Cache cargo binaries

changelog: none
@bors
Copy link
Contributor

bors commented Oct 17, 2019

☀️ Try build successful - checks-travis, status-appveyor
Build commit: 2d416cb (2d416cb9e7f295035f7af7122006bb593dfd5f3f)

@flip1995
Copy link
Member

flip1995 commented Oct 17, 2019

So the build time for the base tests is about 5~7 minutes faster, which is about 18~25%. I don't think that this will make much of a difference on try and r+ runs, since they are mostly limited by the limited amount of concurrent jobs. What I like most about this is that almost all integration tests are under 10 minutes and around 7~8 minutes. I think that this will improve the running time for try and r+ builds the most.

Sadly it doesn't display the Ran for time, because the windows build got interrupted/never started.

The total time is about 35~37% faster though.

@phansch
Copy link
Contributor

phansch commented Oct 17, 2019

Let's get this merged, it's a good improvement in any case!

@bors r+

@bors
Copy link
Contributor

bors commented Oct 17, 2019

📌 Commit d81191e has been approved by phansch

@bors
Copy link
Contributor

bors commented Oct 17, 2019

⌛ Testing commit d81191e with merge 4a388e1...

bors added a commit that referenced this pull request Oct 17, 2019
@bors
Auto merge of #4659 - lzutao:caching, r=phansch
4a388e1 
Cache cargo binaries

changelog: none
@bors
Copy link
Contributor

bors commented Oct 17, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: phansch
Pushing 4a388e1 to master...

@bors bors merged commit d81191e into rust-lang:master Oct 17, 2019
@tesuji tesuji deleted the caching branch October 17, 2019 18:06
@bors bors mentioned this pull request Oct 17, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@tesuji @flip1995 @bors @phansch