Revert "Disable preserving mtimes on archives"

[cwndrws]

This reverts commit 8c92e88.

Fixes #7590

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[alexcrichton]

The technical bits look good to me here, but I agree with @ehuss on the request for performance numbers if we can

[cwndrws]

To roughly test performance, I'm running cargo fetch on the Warp project on a Windows 10 machine on the spinning disk in that machine. I'm going to run it on cargo built from master and cargo built from my branch, both with the --release flag. I will delete the $CARGO_HOME/registry directory between runs.

Cargo release build off of master (no mtime calls):

D:\src\warp [master ≡]> rm -force $HOME/.cargo/registry

Confirm
The item at C:\Users\charl\.cargo\registry has children and the Recurse parameter was not specified. If you
continue, all children will be removed with the item. Are you sure you want to continue?
[Y] Yes  [A] Yes to All  [N] No  [L] No to All  [S] Suspend  [?] Help (default is "Y"): Y
D:\src\warp [master ≡]> Measure-Command { ..\cargo\target\release\cargo.exe fetch }
    Updating crates.io index
  ... # removed crate download output for brevity
  Downloaded 127 crates (16.2 MB) in 1m 07s (largest was `ring` at 5.3 MB)


Days              : 0
Hours             : 0
Minutes           : 1
Seconds           : 10
Milliseconds      : 231
Ticks             : 702314651
TotalDays         : 0.000812864179398148
TotalHours        : 0.0195087403055556
TotalMinutes      : 1.17052441833333
TotalSeconds      : 70.2314651
TotalMilliseconds : 70231.4651

Cargo release build off of my branch (with mtime calls):

D:\src\warp [master ≡]> rm -force $HOME/.cargo/registry

Confirm
The item at C:\Users\charl\.cargo\registry has children and the Recurse parameter was not specified. If you continue,
all children will be removed with the item. Are you sure you want to continue?
[Y] Yes  [A] Yes to All  [N] No  [L] No to All  [S] Suspend  [?] Help (default is "Y"):
D:\src\warp [master ≡]> Measure-Command { ..\cargo\target\release\cargo.exe fetch }
    Updating crates.io index
  ... # removed crate download output for brevity
  Downloaded 172 crates (20.0 MB) in 1m 05s (largest was `ring` at 5.3 MB)


Days              : 0
Hours             : 0
Minutes           : 1
Seconds           : 19
Milliseconds      : 569
Ticks             : 795694809
TotalDays         : 0.000920943065972222
TotalHours        : 0.0221026335833333
TotalMinutes      : 1.326158015
TotalSeconds      : 79.5694809
TotalMilliseconds : 79569.4809

So in this quick an dirty test it seems like performance on this one project got about 13% worse. I'm not sure what is tolerable for this sort of thing, so I'll leave that up to discussion. Let me know if you want me to test on any other systems.

[pnkfelix]

To me, the 13% performance loss is strong evidence that we should add some way to let users opt into the mtime-lossy system.

• Update: that is, add an opt-in for the users, or figure out a way to let individual crates opt into mtime preservation. But the latter is not as trivial as one might hope.

But the 13% performance loss does not serve as evidence that we should continue to make the mtime-lossy behavior the default, given that we have crate authors stating that their bulld scripts are relying on mtime preservations.

[ehuss]

The Cargo team briefly discussed this, and I've been doing some more testing. Since this only affects downloading new crates, generally the performance hit is lost in the noise. Although we still contend that relying on mtime in a build script is a bad idea, it doesn't seem important enough to stop people from trying.

On SSD setups, the "extraction" phase barely registers in my testing. However, on my desktop machine with a magnetic HD, I see it taking a huge amount of time (a cargo fetch that normally takes 30s takes 80s on the HD). Whether or not mtime is preserved doesn't really impact the total time. Lesson here is to use an SSD. 😉

If we really want to improve extraction time, one option is to make extraction run in parallel with downloading. I tried to implement this a couple years ago, but it is very difficult due to the way Cargo's download system works. I'm uncertain if refactoring all that code would be justified since it is already pretty tricky.

@bors r+

[bors]

📌 Commit 25b90e7 has been approved by ehuss

[bors]

⌛ Testing commit 25b90e7 with merge 94093c2...

[bors]

☀️ Test successful - checks-azure
Approved by: ehuss
Pushing 94093c2 to master...