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

Stabilize automatic garbage collection. #14287

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Jul 22, 2024

This proposes to stabilize automatic garbage collection of Cargo's global cache data in the cargo home directory.

What is being stabilized?

This PR stabilizes automatic garbage collection, which is triggered at most once per day by default. This automatic gc will delete old, unused files in cargo's home directory.

It will delete files that need to be downloaded from the network after 3 months, and files that can be generated without network access after 1 month. These thresholds are intended to balance the intent of reducing cargo's disk usage versus deleting too often forcing cargo to do extra work when files are missing.

Tracking of the last-use data is stored in a sqlite database in the cargo home directory. Cargo updates timestamps in that database whenever it accesses a file in the cache. This part is already stabilized.

This PR also stabilizes the gc.auto.frequency configuration option. The primary use case for when a user may want to set that is to set it to "never" to disable gc should the need arise to avoid it.

When gc is initiated, and there are files to delete, there will be a progress bar while it is deleting them. The progress bar will disappear when it finishes. If the user runs with -v verbose option, then cargo will also display which files it deletes.

If there is an error while cleaning, cargo will only display a warning, and otherwise continue.

What is not being stabilized?

The manual garbage collection option (via cargo clean gc) is not proposed to be stabilized at this time. That still needs some design work. This is tracked in #13060.

Additionally, there are several low-level config options currently implemented which define the thresholds for when it will delete files. I think these options are probably too low-level and specific. This is tracked in #13061.

Garbage collection of build artifacts is not yet implemented, and tracked in #13136.

Background

This feature is tracked in #12633 and was implemented in a variety of PRs, primarily #12634.

The tests for this feature are located in https://github.com/rust-lang/cargo/blob/master/tests/testsuite/global_cache_tracker.rs.

Cargo started tracking the last-use data on stable via #13492 in 1.78 which was released 2024-05-02. This PR is proposing to stabilize automatic deletion in 1.82 which will be released in 2024-10-17.

Risks

Users who frequently use versions of Rust older than 1.78 will not have the last-use data tracking updated. If they infrequently use 1.78 or newer, and use the same cache files, then the last-use tracking will only be updated by the newer versions. If that time frame is more than 1 month (or 3 months for downloaded data), then cargo will delete files that the older versions are still using. This means the next time they run the older version, it will have to re-download or re-extract the files.

The effects of deleting cache data in environments where cargo's cache is modified by external tools is not fully known. For example, CI caching systems may save and restore cargo's cache. Similarly, things like Docker images that try to save the cache in a layer, or mount the cache in a read-only filesystem may have undesirable interactions.

The once-a-day performance hit might be noticeable to some people. I've been using this for several months, and almost never notice it. However, slower systems, or situations where there is a lot of data to delete might take a while (on the order of seconds hopefully).

@ehuss ehuss added T-cargo Team: Cargo Z-gc Nightly: garbage collection labels Jul 22, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jul 22, 2024

r? @weihanglo

rustbot has assigned @weihanglo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-documenting-cargo-itself Area: Cargo's documentation S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 22, 2024
@ehuss
Copy link
Contributor Author

ehuss commented Jul 22, 2024

@rfcbot fcp merge

@rfcbot
Copy link
Collaborator

rfcbot commented Jul 22, 2024

Team member @ehuss has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period An FCP proposal has started, but not yet signed off. disposition-merge FCP with intent to merge labels Jul 22, 2024
@epage
Copy link
Contributor

epage commented Jul 22, 2024

@rfcbot concern field-name

Something that came up on the tracking issue is whether people will get confused with the gc name (#12633 (comment)). I suspect that won't be a problem but I want to make sure we acknowledge it and agree it isn't a problem first.

I am also somewhat concerned about how to organize and name the config for if/when we get to GC within target directories. I wonder if this would affect the top-level table name (e.g. calling it global-cache). In other ways, only stabilizing auto gives us a lot of leeway in what it does.

(sorry, thought of this after the conversation about stabilizing this)

@cessen
Copy link

cessen commented Jul 23, 2024

Something that came up on the tracking issue is whether people will get confused with the gc name

I agree that this isn't likely to be an issue in practice.

However, I also feel like we might as well be more accurate and call it something like "cache cleaning", which is what it actually is. As I mentioned in the other thread, garbage collection is usually related to some concept of reachability and resultant high confidence that something is no longer used. This doesn't reflect how this feature actually works, or ever reasonably could work.

And unless I'm missing something here (which is always possible), I don't think there's any cost to simply naming this feature more accurately, aside from taking a bit of time to decide on that name.

@elenakrittik
Copy link

Sorry if this is the wrong place to ask, but will there be a way to disable this behaviour?

@Skgland
Copy link

Skgland commented Jul 23, 2024

Sorry if this is the wrong place to ask, but will there be a way to disable this behaviour?

I think the PR description already answers this:

This PR also stabilizes the gc.auto.frequency configuration option. The primary use case for when a user may want to set that is to set it to "never" to disable gc should the need arise to avoid it.

@ehuss
Copy link
Contributor Author

ehuss commented Jul 23, 2024

Sure, would be happy to rename it. I opened #14292 to have that suggestion, please express your thoughts over there.


* `"never"` --- Never deletes old files.
* `"always"` --- Checks to delete old files every time Cargo runs.
* An integer followed by "seconds", "minutes", "hours", "days", "weeks", or "months" --- Checks to delete old files at most the given time frame.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

“months” is an approximate number.

cargo/src/cargo/core/gc.rs

Lines 373 to 381 in ea14e86

let factor = match right {
"second" | "seconds" => 1,
"minute" | "minutes" => 60,
"hour" | "hours" => 60 * 60,
"day" | "days" => 24 * 60 * 60,
"week" | "weeks" => 7 * 24 * 60 * 60,
"month" | "months" => 2_629_746, // average is 30.436875 days
_ => return None,
};

I can foresee someone will interpret months as monthly and think it will be clean on the same day when set, while it is not true especially in February. I don't think this is a thing we can't change after stabilization. Just calling it out if someone disagrees.

Copy link

@teohhanhui teohhanhui Aug 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just remove months? It'd be counter-intuitive to anyone trying to use it... The user can already specify something like 180 days for ~6 months, no?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For myself, being able to say "6 months" is much easier than calculating out the number of days and reading the number of days.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@epage But would anyone expect it to be 182.62125 days? Principle of least surprise...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an inherent approximation when giving a unit. The larger the unit, the larger the approximation. If you say "6 months", you shouldn't care whether thats 180, 186, 182, or 182.62125.

btw laughing emoji's in a technical discussion like this come across as rude.

Copy link

@teohhanhui teohhanhui Aug 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw laughing emoji's in a technical discussion like this come across as rude.

Sigh... Here we go again. Intent does not carry across text (or emojis), so please don't jump to conclusions like that.

I was not even disagreeing with what you said. Just pointing out that it'd be surpirising for the user, as the OP of this thread has already pointed out (a different surprising aspect).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@epage But would anyone expect it to be 182.62125 days? Principle of least surprise...

168 would be surprising; a bit over 180 not so much. The kind of surprise we're trying to avoid is purging data way earlier than expected.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For myself, being able to say "6 months" is much easier than calculating out the number of days and reading the number of days.

If the user goes to the trouble of customizing this in the config, I don't think having to calculate the number of days would be much of an extra hurdle. In effect, removing months would just simplify things with no real downside (and prevent future support questions where people are arguing over this again / trying to figure out what's going on with this approximation).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A month equals 30 days is easier for me to accept

@bors
Copy link
Contributor

bors commented Nov 9, 2024

☔ The latest upstream changes (presumably #14388) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Nov 25, 2024

☔ The latest upstream changes (presumably 4c39aaf) made this pull request unmergeable. Please resolve the merge conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-documenting-cargo-itself Area: Cargo's documentation disposition-merge FCP with intent to merge proposed-final-comment-period An FCP proposal has started, but not yet signed off. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-cargo Team: Cargo Z-gc Nightly: garbage collection
Projects
Status: FCP blocked
Development

Successfully merging this pull request may close these issues.