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

Update submodules in parallel #49093

Merged
merged 1 commit into from
Mar 22, 2018
Merged

Update submodules in parallel #49093

merged 1 commit into from
Mar 22, 2018

Conversation

Zoxc
Copy link
Contributor

@Zoxc Zoxc commented Mar 16, 2018

No description provided.

@Zoxc Zoxc force-pushed the speedup branch 2 times, most recently from 273b934 to a4f05a6 Compare March 16, 2018 22:25
@Zoxc Zoxc changed the title [WIP] Testing Update submodules in parallel Mar 16, 2018
@Zoxc
Copy link
Contributor Author

Zoxc commented Mar 16, 2018

This seems to save a couple of seconds.

r? @alexcrichton

@Zoxc Zoxc force-pushed the speedup branch 12 times, most recently from 6a7853c to 0725b0c Compare March 17, 2018 12:54
@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 17, 2018
@Zoxc Zoxc force-pushed the speedup branch 7 times, most recently from 9f747f5 to 37487be Compare March 17, 2018 14:09
@Zoxc
Copy link
Contributor Author

Zoxc commented Mar 17, 2018

I changed to code to get tarballs for more submodules. I've seen the time taken to check out submodules go as low as 13 seconds on Travis, it used to take around 90 seconds before. This is probably more helpful for AppVeyor since it takes longer to check out submodules.

This may result in submodules incorrectly assuming that the rustc's repo commit hash is their own. We should check that that doesn't happen.

@alexcrichton
Copy link
Member

r? @aidanhs

@aidanhs
Copy link
Member

aidanhs commented Mar 19, 2018

This may result in submodules incorrectly assuming that the rustc's repo commit hash is their own. We should check that that doesn't happen.

Regarding this, if you touch .git inside each of the submodules, it appears that confuses git sufficiently that it will error (hopefully allowing us to catch this kind of error when it happens):

/tmp/tmp.ZwUF7zEhcg $ git status
fatal: Not a git repository (or any of the parent directories): .git
/tmp/tmp.ZwUF7zEhcg $ git init
Initialised empty Git repository in /tmp/tmp.ZwUF7zEhcg/.git/
/tmp/tmp.ZwUF7zEhcg $ git status
On branch master

Initial commit

nothing to commit (create/copy files and use "git add" to track)
/tmp/tmp.ZwUF7zEhcg $ mkdir a
/tmp/tmp.ZwUF7zEhcg $ cd a
/tmp/tmp.ZwUF7zEhcg/a $ git status
On branch master

Initial commit

nothing to commit (create/copy files and use "git add" to track)
/tmp/tmp.ZwUF7zEhcg/a $ touch .git
/tmp/tmp.ZwUF7zEhcg/a $ git status
fatal: Invalid gitfile format: .git
/tmp/tmp.ZwUF7zEhcg/a $ echo $?
128

One example that springs to mind - cargo --version prints a commitish. I assume that means it's one of the ones that needs cloning.

@aidanhs
Copy link
Member

aidanhs commented Mar 19, 2018

There are two parts here:

  1. cloning in parallel (-j to submodule init)
  2. defaulting to download from a .zip

I love 1! That flag must've been added recently, I don't have it on my version - since this only runs in CI, it's probably fine to require it (we can see if the appveyor git version plays ball).

I understand the motivation of 2 and the results certainly speak for themselves, but I feel a little cautious about an 'on-by-default' approach. Taking an arbitrary recent build log from travis, we can get most of the win just by including a couple more repos to be downloaded as a .zip (book, rust-by-example). Combined with 1, I suspect it'll bring us down to a similar speed but with less additional risk.

[00:00:05] Cloning into '/home/travis/rustsrc/src/src/dlmalloc'...
[00:00:05] Cloning into '/home/travis/rustsrc/src/src/doc/book'...
[00:00:09] Cloning into '/home/travis/rustsrc/src/src/doc/nomicon'...
[00:00:10] Cloning into '/home/travis/rustsrc/src/src/doc/reference'...
[00:00:11] Cloning into '/home/travis/rustsrc/src/src/doc/rust-by-example'...
[00:00:43] Cloning into '/home/travis/rustsrc/src/src/jemalloc'...
[00:00:44] Cloning into '/home/travis/rustsrc/src/src/libcompiler_builtins'...
[00:00:45] Cloning into '/home/travis/rustsrc/src/src/liblibc'...
[00:00:47] Cloning into '/home/travis/rustsrc/src/src/stdsimd'...
[00:00:48] Cloning into '/home/travis/rustsrc/src/src/tools/cargo'...
[00:00:50] Cloning into '/home/travis/rustsrc/src/src/tools/clippy'...
[00:00:52] Cloning into '/home/travis/rustsrc/src/src/tools/lld'...
[00:00:54] Cloning into '/home/travis/rustsrc/src/src/tools/miri'...
[00:00:55] Cloning into '/home/travis/rustsrc/src/src/tools/rls'...
[00:00:56] Cloning into '/home/travis/rustsrc/src/src/tools/rust-installer'...
[00:00:56] Cloning into '/home/travis/rustsrc/src/src/tools/rustfmt'...

@Zoxc
Copy link
Contributor Author

Zoxc commented Mar 19, 2018

I've changed it to use git submodules by default.

@Zoxc
Copy link
Contributor Author

Zoxc commented Mar 19, 2018

Could we use git submodules only for dist build? This would ensure that artifacts shipped to users don't have the wrong commit hash.

@Zoxc Zoxc force-pushed the speedup branch 3 times, most recently from 6693998 to 1578b1e Compare March 19, 2018 20:02
@Zoxc
Copy link
Contributor Author

Zoxc commented Mar 19, 2018

I ran this on AppVeyor. I got the following timings (best 3 of 4):

Tarball by default: 36s 42s 44s
Git submodule by default: 64s 77s 80s

@aidanhs
Copy link
Member

aidanhs commented Mar 19, 2018

I'm ok with 30s for the increased peace of mind :)

@bors r+

@bors
Copy link
Contributor

bors commented Mar 19, 2018

📌 Commit 1578b1e has been approved by aidanhs

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 19, 2018
@aidanhs
Copy link
Member

aidanhs commented Mar 19, 2018

(thanks for the great work by the way, the slowness of cloning has long been an annoyance of mine)

kennytm added a commit to kennytm/rust that referenced this pull request Mar 21, 2018
kennytm added a commit to kennytm/rust that referenced this pull request Mar 22, 2018
bors added a commit that referenced this pull request Mar 22, 2018
@alexcrichton alexcrichton merged commit 1578b1e into rust-lang:master Mar 22, 2018
@Zoxc Zoxc deleted the speedup branch March 22, 2018 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants