-
Notifications
You must be signed in to change notification settings - Fork 159
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
[Feature] Mark-and-sweep GC. #3504
Conversation
@lemmih This design is not taking into account the |
The CAR files contain read-only data. There's no need for the GC to traverse them. In the future, we do want to find unused CAR files but you can leave that for another PR. |
Right, that makes sense. |
Co-authored-by: David Himmelstrup <[email protected]>
Co-authored-by: David Himmelstrup <[email protected]>
Co-authored-by: David Himmelstrup <[email protected]>
Sounds good! |
src/db/gc/mod.rs
Outdated
/// | ||
/// * `db` - A reference to the database instance. | ||
/// * `get_heaviest_tipset` - A function that facilitates heaviest tipset retrieval. | ||
/// * `depth` - The number of state-roots to retain. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What should depth
be initialized to? In the associated issue we're speaking of a number of 2 x chain_finality
, here we're taking max of chain_finality
and recent_state_roots
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not garbage collector's concern, the decision is made by the caller. But it's a good catch, and I will comment and fix it to be 2x chain_finality.
Should this new implementation of GC fix our long-standing send issue? (#3645) Eventually, let's try to uncomment the test in |
Manual GC is deprecated, it makes no sense with the current approach. |
@ruseinov You probably have to update the branch before the CI checks will run. |
so it seems! |
This reverts commit defce38.
Summary of changes
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes #3072
Other information and links
This will require a
0.13.1
release due to a database migration needed to deprecate RollingDB.Perhaps a good idea is to incorporate the
CHANGELOG.md
changes within this PR as well to simplify things.Change checklist