Skip to content

Commit 1e12e31

Browse files
rbtcollinsrbtcollins
committed
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.
1 parent f07e26f commit 1e12e31

File tree

4 files changed

+82
-47
lines changed

4 files changed

+82
-47
lines changed

‎src/dist/component/components.rs

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,16 @@ impl<'a> ComponentBuilder<'a> {
109109
.push(ComponentPart("dir".to_owned(), path.clone()));
110110
self.tx.copy_dir(&self.name, path, src)
111111
}
112-
112+
pub fn move_file(&mut self, path: PathBuf, src: &Path) -> Result<()> {
113+
self.parts
114+
.push(ComponentPart("file".to_owned(), path.clone()));
115+
self.tx.move_file(&self.name, path, src)
116+
}
117+
pub fn move_dir(&mut self, path: PathBuf, src: &Path) -> Result<()> {
118+
self.parts
119+
.push(ComponentPart("dir".to_owned(), path.clone()));
120+
self.tx.move_dir(&self.name, path, src)
121+
}
113122
pub fn finish(mut self) -> Result<Transaction<'a>> {
114123
// Write component manifest
115124
let path = self.components.rel_component_manifest(&self.name);

‎src/dist/component/package.rs

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -35,17 +35,19 @@ pub trait Package: fmt::Debug {
3535
pub struct DirectoryPackage {
3636
path: PathBuf,
3737
components: HashSet<String>,
38+
copy: bool,
3839
}
3940

4041
impl DirectoryPackage {
41-
pub fn new(path: PathBuf) -> Result<Self> {
42+
pub fn new(path: PathBuf, copy: bool) -> Result<Self> {
4243
validate_installer_version(&path)?;
4344

4445
let content = utils::read_file("package components", &path.join("components"))?;
4546
let components = content.lines().map(|l| l.to_owned()).collect();
4647
Ok(DirectoryPackage {
4748
path: path,
4849
components: components,
50+
copy: copy,
4951
})
5052
}
5153
}
@@ -97,8 +99,14 @@ impl Package for DirectoryPackage {
9799
let src_path = root.join(&path);
98100

99101
match &*part.0 {
100-
"file" => builder.copy_file(path.clone(), &src_path)?,
101-
"dir" => builder.copy_dir(path.clone(), &src_path)?,
102+
"file" => {
103+
if self.copy { builder.copy_file(path.clone(), &src_path)? }
104+
else { builder.move_file(path.clone(), &src_path)? }
105+
}
106+
"dir" => {
107+
if self.copy { builder.copy_dir(path.clone(), &src_path)? }
108+
else { builder.move_dir(path.clone(), &src_path)? }
109+
}
102110
_ => return Err(ErrorKind::CorruptComponent(name.to_owned()).into()),
103111
}
104112

@@ -187,7 +195,7 @@ impl<'a> TarPackage<'a> {
187195
unpack_without_first_dir(&mut archive, &*temp_dir)?;
188196

189197
Ok(TarPackage(
190-
DirectoryPackage::new(temp_dir.to_owned())?,
198+
DirectoryPackage::new(temp_dir.to_owned(), false)?,
191199
temp_dir,
192200
))
193201
}

‎src/dist/component/transaction.rs

Lines changed: 52 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -133,6 +133,22 @@ impl<'a> Transaction<'a> {
133133
Ok(())
134134
}
135135

136+
/// Move a file to a relative path of the install prefix.
137+
pub fn move_file(&mut self, component: &str, relpath: PathBuf, src: &Path) -> Result<()> {
138+
assert!(relpath.is_relative());
139+
let item = ChangedItem::move_file(&self.prefix, component, relpath, src)?;
140+
self.change(item);
141+
Ok(())
142+
}
143+
144+
/// Recursively move a directory to a relative path of the install prefix.
145+
pub fn move_dir(&mut self, component: &str, relpath: PathBuf, src: &Path) -> Result<()> {
146+
assert!(relpath.is_relative());
147+
let item = ChangedItem::move_dir(&self.prefix, component, relpath, src)?;
148+
self.change(item);
149+
Ok(())
150+
}
151+
136152
pub fn temp(&self) -> &'a temp::Cfg {
137153
self.temp_cfg
138154
}
@@ -195,8 +211,8 @@ impl<'a> ChangedItem<'a> {
195211
}
196212
Ok(())
197213
}
198-
fn add_file(prefix: &InstallPrefix, component: &str, relpath: PathBuf) -> Result<(Self, File)> {
199-
let abs_path = prefix.abs_path(&relpath);
214+
fn dest_abs_path(prefix: &InstallPrefix, component: &str, relpath: &PathBuf) -> Result<PathBuf> {
215+
let abs_path = prefix.abs_path(relpath);
200216
if utils::path_exists(&abs_path) {
201217
Err(ErrorKind::ComponentConflict {
202218
name: component.to_owned(),
@@ -207,53 +223,34 @@ impl<'a> ChangedItem<'a> {
207223
if let Some(p) = abs_path.parent() {
208224
utils::ensure_dir_exists("component", p, &|_| ())?;
209225
}
210-
let file = File::create(&abs_path)
211-
.chain_err(|| format!("error creating file '{}'", abs_path.display()))?;
212-
213-
Ok((ChangedItem::AddedFile(relpath), file))
226+
Ok(abs_path)
214227
}
215228
}
229+
fn add_file(prefix: &InstallPrefix, component: &str, relpath: PathBuf) -> Result<(Self, File)> {
230+
let abs_path = ChangedItem::dest_abs_path(prefix, component, &relpath)?;
231+
let file = File::create(&abs_path)
232+
.chain_err(|| format!("error creating file '{}'", abs_path.display()))?;
233+
Ok((ChangedItem::AddedFile(relpath), file))
234+
}
216235
fn copy_file(
217236
prefix: &InstallPrefix,
218237
component: &str,
219238
relpath: PathBuf,
220239
src: &Path,
221240
) -> Result<Self> {
222-
let abs_path = prefix.abs_path(&relpath);
223-
if utils::path_exists(&abs_path) {
224-
Err(ErrorKind::ComponentConflict {
225-
name: component.to_owned(),
226-
path: relpath.clone(),
227-
}
228-
.into())
229-
} else {
230-
if let Some(p) = abs_path.parent() {
231-
utils::ensure_dir_exists("component", p, &|_| ())?;
232-
}
233-
utils::copy_file(src, &abs_path)?;
234-
Ok(ChangedItem::AddedFile(relpath))
235-
}
241+
let abs_path = ChangedItem::dest_abs_path(prefix, component, &relpath)?;
242+
utils::copy_file(src, &abs_path)?;
243+
Ok(ChangedItem::AddedFile(relpath))
236244
}
237245
fn copy_dir(
238246
prefix: &InstallPrefix,
239247
component: &str,
240248
relpath: PathBuf,
241249
src: &Path,
242250
) -> Result<Self> {
243-
let abs_path = prefix.abs_path(&relpath);
244-
if utils::path_exists(&abs_path) {
245-
Err(ErrorKind::ComponentConflict {
246-
name: component.to_owned(),
247-
path: relpath.clone(),
248-
}
249-
.into())
250-
} else {
251-
if let Some(p) = abs_path.parent() {
252-
utils::ensure_dir_exists("component", p, &|_| ())?;
253-
}
254-
utils::copy_dir(src, &abs_path, &|_| ())?;
255-
Ok(ChangedItem::AddedDir(relpath))
256-
}
251+
let abs_path = ChangedItem::dest_abs_path(prefix, component, &relpath)?;
252+
utils::copy_dir(src, &abs_path, &|_| ())?;
253+
Ok(ChangedItem::AddedDir(relpath))
257254
}
258255
fn remove_file(
259256
prefix: &InstallPrefix,
@@ -311,4 +308,25 @@ impl<'a> ChangedItem<'a> {
311308
Ok(ChangedItem::ModifiedFile(relpath, None))
312309
}
313310
}
311+
fn move_file(
312+
prefix: &InstallPrefix,
313+
component: &str,
314+
relpath: PathBuf,
315+
src: &Path,
316+
) -> Result<Self> {
317+
let abs_path = ChangedItem::dest_abs_path(prefix, component, &relpath)?;
318+
utils::rename_file("component", src, &abs_path)?;
319+
Ok(ChangedItem::AddedFile(relpath))
320+
}
321+
fn move_dir(
322+
prefix: &InstallPrefix,
323+
component: &str,
324+
relpath: PathBuf,
325+
src: &Path,
326+
) -> Result<Self> {
327+
let abs_path = ChangedItem::dest_abs_path(prefix, component, &relpath)?;
328+
utils::rename_dir("component", src, &abs_path)?;
329+
Ok(ChangedItem::AddedDir(relpath))
330+
}
331+
314332
}

‎tests/dist_install.rs

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@ fn package_contains() {
6767

6868
mock.build(tempdir.path());
6969

70-
let package = DirectoryPackage::new(tempdir.path().to_owned()).unwrap();
70+
let package = DirectoryPackage::new(tempdir.path().to_owned(), true).unwrap();
7171
assert!(package.contains("mycomponent", None));
7272
assert!(package.contains("mycomponent2", None));
7373
}
@@ -88,7 +88,7 @@ fn package_bad_version() {
8888
let mut ver = File::create(tempdir.path().join("rust-installer-version")).unwrap();
8989
writeln!(ver, "100").unwrap();
9090

91-
assert!(DirectoryPackage::new(tempdir.path().to_owned()).is_err());
91+
assert!(DirectoryPackage::new(tempdir.path().to_owned(), true).is_err());
9292
}
9393

9494
#[test]
@@ -122,7 +122,7 @@ fn basic_install() {
122122

123123
let components = Components::open(prefix.clone()).unwrap();
124124

125-
let pkg = DirectoryPackage::new(pkgdir.path().to_owned()).unwrap();
125+
let pkg = DirectoryPackage::new(pkgdir.path().to_owned(), true).unwrap();
126126

127127
let tx = pkg.install(&components, "mycomponent", None, tx).unwrap();
128128
tx.commit();
@@ -168,7 +168,7 @@ fn multiple_component_install() {
168168

169169
let components = Components::open(prefix.clone()).unwrap();
170170

171-
let pkg = DirectoryPackage::new(pkgdir.path().to_owned()).unwrap();
171+
let pkg = DirectoryPackage::new(pkgdir.path().to_owned(), true).unwrap();
172172

173173
let tx = pkg.install(&components, "mycomponent", None, tx).unwrap();
174174
let tx = pkg.install(&components, "mycomponent2", None, tx).unwrap();
@@ -218,7 +218,7 @@ fn uninstall() {
218218

219219
let components = Components::open(prefix.clone()).unwrap();
220220

221-
let pkg = DirectoryPackage::new(pkgdir.path().to_owned()).unwrap();
221+
let pkg = DirectoryPackage::new(pkgdir.path().to_owned(), true).unwrap();
222222

223223
let tx = pkg.install(&components, "mycomponent", None, tx).unwrap();
224224
let tx = pkg.install(&components, "mycomponent2", None, tx).unwrap();
@@ -275,7 +275,7 @@ fn component_bad_version() {
275275

276276
let components = Components::open(prefix.clone()).unwrap();
277277

278-
let pkg = DirectoryPackage::new(pkgdir.path().to_owned()).unwrap();
278+
let pkg = DirectoryPackage::new(pkgdir.path().to_owned(), true).unwrap();
279279

280280
let tx = pkg.install(&components, "mycomponent", None, tx).unwrap();
281281
tx.commit();
@@ -336,7 +336,7 @@ fn unix_permissions() {
336336

337337
let components = Components::open(prefix.clone()).unwrap();
338338

339-
let pkg = DirectoryPackage::new(pkgdir.path().to_owned()).unwrap();
339+
let pkg = DirectoryPackage::new(pkgdir.path().to_owned(), true).unwrap();
340340

341341
let tx = pkg.install(&components, "mycomponent", None, tx).unwrap();
342342
tx.commit();
@@ -421,7 +421,7 @@ fn install_to_prefix_that_does_not_exist() {
421421

422422
let components = Components::open(prefix.clone()).unwrap();
423423

424-
let pkg = DirectoryPackage::new(pkgdir.path().to_owned()).unwrap();
424+
let pkg = DirectoryPackage::new(pkgdir.path().to_owned(), true).unwrap();
425425

426426
let tx = pkg.install(&components, "mycomponent", None, tx).unwrap();
427427
tx.commit();

0 commit comments

Comments
 (0)