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

Less copying during dist installation #1744

Merged
merged 1 commit into from Apr 14, 2019

Conversation

Projects
None yet
3 participants
@rbtcollins
Copy link
Contributor

commented Apr 9, 2019

Per #904 copying the contents of dists out of the extracted staging
directory and then later deleting that same staging directory consumes
60s out of a total of 200s on Windows.

Wall clock testing shows this patch reduces
rustup toolchain install nightly from 3m45 to 2m23 for me - including
download times etc.

I'm sure there is more that can be done, thus I'm not marking this as
a closing merge for #904.

@rbtcollins rbtcollins force-pushed the rbtcollins:lesscopies branch from dfd512d to 1e12e31 Apr 9, 2019

Less copying during dist installation
Per #904 copying the contents of dists out of the extracted staging
directory and then later deleting that same staging directory consumes
60s out of a total of 200s on Windows.

Wall clock testing shows this patch reduces
`rustup toolchain install nightly` from 3m45 to 2m23 for me - including
download times etc.

I'm sure there is more that can be done, thus I'm not marking this as
a closing merge for #904.

@rbtcollins rbtcollins force-pushed the rbtcollins:lesscopies branch from 1e12e31 to d74a13c Apr 9, 2019

@kinnison
Copy link
Collaborator

left a comment

Over all, I think this looks good. I like the cleanup for dest_abs_path() and the copy-vs-move semantics seem sound to me.

If at all possible, I'd love to see if @brson has enough memory of this code area to comment, otherwise given the tests are green I'm okay for merging this.

@kinnison

This comment has been minimized.

Copy link
Collaborator

commented Apr 11, 2019

I can confirm that this branch has been working fine for me in local testing for 2 days.

@nrc

nrc approved these changes Apr 14, 2019

Copy link
Member

left a comment

LGTM, thanks!

@nrc nrc merged commit 88364b1 into rust-lang:master Apr 14, 2019

2 checks passed

Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@rbtcollins rbtcollins deleted the rbtcollins:lesscopies branch Apr 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.