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

Improve performance and accuracy by baking in targets structure knowledge? #6

Closed
epage opened this issue Nov 11, 2018 · 7 comments
Closed

Comments

@epage
Copy link

epage commented Nov 11, 2018

targets has a basic structure. Once you have the directory for a target, a lot of dependencies are kept in hashed folders. I suspect those hashes include the version and feature flags.

  • This means for each of those folders, we can bail out at the first file that has too new of an atime.
  • We probably want to keep these entire folders rather than delete only part of them

The problem is this becomes more brittle because it becomes dependent on how cargo works.

@holmgr
Copy link
Owner

holmgr commented Nov 11, 2018

This would be a nice change, but it would perhaps also involve some restructuring since we would have to defer the actual deletion until we are certain that all files are old enough and then delete the entire directory.

I also do get the issue of being sensitive to changes of how cargo/rustc stores the files in the target directory, although I would guess that it is pretty stable by this point.

@Eh2406
Copy link
Collaborator

Eh2406 commented Dec 15, 2018

I would guess that it is pretty stable by this point.

It is pretty stable, in that It has not changed in a very long time. It is an implementation detail, in that the Cargo team would be comfortable changing it. There has been some buzz about adding a folder to make it clear what is from crates.io (and unlikely to change) vs other sources (more likely to change), or a folder for each compiler version. If there are changes to the structure that would make it easier for cargo-sweep to work well, please let us know at the Cargo repo.

@Eh2406
Copy link
Collaborator

Eh2406 commented Dec 17, 2018

While investigating #2 (comment), I learned some things about how the folder is structured. As I understand it, the folders come in sets. For example, if there is a folder atty-5ea146758e503e24 in /target/debug/.fingerprint then there may be a corresponding folder in /build, /incremental, /native and files with that name in /deps and /. I think it would be most reliable to keep/remove all of these files as a set. I believe that cargo looks at the files in /.fingerprint for every transitive dependency on every bild. So the atime from there may be the most reliable. I also think that it will not be a big PR to have cargo add a timestamp file while it is looking at the fingerprint. Edit: Verified. compare_old_fingerprint reads the /.fingerprint/lib-... file, and is called in prepare_target, witch is called in compile, which calls itself recursively on the deps.

Meanwhile, the fingerprint already includes the version of rust needed for the files, sow next time I have a sec I will work on a version that lets you cargo +stable build && cargo +nightly build && rustup toolchain remove nightly && cargo sweep --installed and it will remove all the files related to nightly leaving you with the same files as if you had only run cargo +stable build. (Witch a lot of peepal need to clean the cache folder on CI when nightly updates. like rust-lang/crates.io#1578)

@holmgr
Copy link
Owner

holmgr commented Dec 20, 2018

Hmm, looks interesting. However, are we sure that .fingerprint is actually touched when doing a compile every time, i.e even if we compile twice in a row?

If you have time on work on that feature that cleans the cache folder on CI on nightly updates I would really appreciate it!

@Eh2406
Copy link
Collaborator

Eh2406 commented Dec 23, 2018

I have a branch that builds on #14 to remove sets of files based on the fingerprints atime. I have been testing it with

cargo +nightly build // to make sure deps are around
cargo +stable build // to make sure deps are around
cargo sweep -s
cargo +stable build  // make sure no deps are rebuilt
cargo sweep -f
cargo +stable build  // make sure no deps are rebuilt
cargo +nightly build  // make sure deps are rebuilt, I.E. we cleaned something above. 

and so far it has worked on everything I have tried. (well I am on a system that does not support atime. so I had to use a local copy of Cargo with rust-lang/cargo#6477, but that just touches a file instead of reading it.)

@Eh2406
Copy link
Collaborator

Eh2406 commented Jan 3, 2019

Move to close?

@holmgr
Copy link
Owner

holmgr commented Jan 3, 2019

👍

@holmgr holmgr closed this as completed Jan 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants