feat(gc): record workspace manifest and target dir in global cache tracker #13846

baby230211 · 2024-05-02T12:48:35Z

What does this PR try to resolve?

Fix #13136
This PR aims to track of the following data in GC for further garbage collection use.

root-manifest path
target directory path

There will be three new table for this features.

worksapce_manifest_index table - index and record the root manifest path

id	workspace_manifest	timestamp
?	/foo/Cargo.toml	?

target_dir_index table - index and record the target_dir path

id	target_dir	timestamp
?	/foo/target	?

workspace_src table - combine the previous two table

id	worksapce_id	target_dir_id	timestamp
?	?	?	?

How should we test and review this PR?

Run compile (cargo run build or else)
inspect your root gc file (~/.cargo/.global-cache)
see if the three table is record correctly.

rustbot · 2024-05-02T12:48:40Z

r? @ehuss

rustbot has assigned @ehuss.
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

epage · 2024-05-02T13:18:37Z

src/cargo/core/global_cache_tracker.rs

+        basic_migration(
+            "CREATE TABLE workspace_manifest_index (
+                id INTEGER PRIMARY KEY AUTOINCREMENT,
+                name TEXT UNIQUE NOT NULL,
+                timestamp INTEGER NOT NULL
+            )",
+        ),
+        basic_migration(
+            "CREATE TABLE target_dir_index (
+                id INTEGER PRIMARY KEY AUTOINCREMENT,
+                name TEXT UNIQUE NOT NULL,
+                timestamp INTEGER NOT NULL
+            )",
+        ),
+        basic_migration(
+            "CREATE TABLE workspace_src (
+                workspace_id INTEGER NOT NULL,
+                target_dir_id INTEGER NOT NULL,
+                timestamp INTEGER NOT NULL,
+                PRIMARY KEY (workspace_id, target_dir_id),
+                FOREIGN KEY (workspace_id) REFERENCES workspace_manifest_index (id) ON DELETE CASCADE,
+                FOREIGN KEY (target_dir_id) REFERENCES target_dir_index (id) ON DELETE CASCADE
+            )",


Whats the motivation for

Tracking this as 3 separate tables

Having timestamps for workspace dir, target dir, and workspace dir + target dir

epage · 2024-05-02T13:20:02Z

src/cargo/core/global_cache_tracker.rs

+    /// New workspace manifest entries to insert.
+    workspace_db_timestamps: HashMap<WorkspaceManifestIndex, Timestamp>,
+    /// New target dir entries to insert.
+    target_dir_db_timestamps: HashMap<TargetDirIndex, Timestamp>,
+    /// New workspace src entries to insert.
+    workspace_src_timestamps: HashMap<WorkspaceSrc, Timestamp>,


What does db mean in this context?

I think for git

git_db is the .git

git_checkout is a work dir of a shared .git

epage · 2024-05-02T13:24:42Z

src/cargo/core/global_cache_tracker.rs

+    pub encoded_workspace_manifest_name: InternedString,
+    pub encoded_target_dir_name: InternedString,


When the others refer to "encoded" they are referring to how we translate package names to file names.

That doesn't quite make sense here and just naming these target_dir and workspace_manifest_path should work

i'll rename it with target_dir_path and workspace_manifest_path for these naming.
Thanks.

epage · 2024-05-02T13:25:24Z

src/cargo/core/global_cache_tracker.rs

+    /// Indicates the given [`WorkspaceManifest`] has been used right now.
+    ///
+    /// Also implicitly marks the workspace manifest used, too.
+    pub fn mark_workspace_src_used(&mut self, workspace_src: WorkspaceSrc) {


Huh, I wonder why we exposed these in the API, rather than taking parameters. Seems like it creates more boilerplate for the callers

epage · 2024-05-02T13:26:36Z

src/cargo/ops/cargo_compile/mod.rs

@@ -264,6 +264,16 @@ pub fn create_bcx<'a, 'gctx>(
            HasDevUnits::No
        }
    };
+    let _ = &gctx.deferred_global_last_use()?.mark_workspace_src_used(


Why let _ = &?

If we remove let _ = &, rustc would be warning as follow

rustc: unused borrow that must be used (unused_must_use)

I removed it locally and didn't get any message...

gctx.deferred_global_last_use()? .mark_workspace_src_used(global_cache_tracker::WorkspaceSrc { workspace_manifest_name: InternedString::new(ws.root_manifest().to_str().unwrap()), target_dir_name: InternedString::new( ws.target_dir().as_path_unlocked().to_str().unwrap(), ), });

This is the error msg where runs on CI.
https://github.com/rust-lang/cargo/actions/runs/9055535414/job/24876799051#step:5:687

That has a leading & that can be removed

It works. Thanks, I've updated.

epage · 2024-05-02T13:27:35Z

src/cargo/core/global_cache_tracker.rs

@@ -1632,6 +1734,32 @@ impl DeferredGlobalLastUse {
        );
    }

+    // Flushes all of the `target_dir_db_timestamps` to the database,


Review note: I've not yet dug into all of the DB operations

bors · 2024-06-07T18:35:39Z

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

epage · 2024-06-20T01:19:24Z

As this is marked as a draft, @baby230211 what do you see as the next steps before marking this ready for review?

baby230211 · 2024-06-20T16:15:36Z

@epage , Sorry for pending this PR so long, the final step here is to record the manifest lock file in manifest table i think.

epage · 2024-06-21T15:30:58Z

You're ok. I just wanted to check if there was something waiting on me.

…acker

…recorded correctly

baby230211 · 2024-10-16T15:06:25Z

Hello @epage , i would like to re-work on this PR recently.
Due to recording the manifest lock file is not related to tracking the target/, i wonder if it's fine to not to track that lock file here.

epage · 2024-10-24T13:14:15Z

Lockfile tracking was mostly intended to prep for #13137. We'll likely need to database migrations anyways so probably fine to skip for now so long as we understand how we would support it in the future.

rustbot assigned ehuss May 2, 2024

baby230211 marked this pull request as draft May 2, 2024 12:48

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 2, 2024

baby230211 force-pushed the feat-13136 branch from f2d5a7d to 534397c Compare May 2, 2024 12:49

rustbot added the A-infrastructure Area: infrastructure around the cargo repo, ci, releases, etc. label May 2, 2024

baby230211 force-pushed the feat-13136 branch 3 times, most recently from 5f32f71 to 15e2863 Compare May 2, 2024 12:52

epage reviewed May 2, 2024

View reviewed changes

baby230211 force-pushed the feat-13136 branch 2 times, most recently from 9f069a9 to d55dda7 Compare May 15, 2024 12:39

baby230211 force-pushed the feat-13136 branch 5 times, most recently from 7d4fbbe to 481de08 Compare May 23, 2024 03:29

baby230211 force-pushed the feat-13136 branch from 481de08 to 37a7d5a Compare June 13, 2024 13:04

baby230211 added 2 commits August 21, 2024 10:46

feat(gc): record workspace manifest and target dir in global cache tr…

d542986

…acker

test(global_cache_tracker): verify workspace manifest and target dir …

7382a2d

…recorded correctly

baby230211 force-pushed the feat-13136 branch from 37a7d5a to 7382a2d Compare August 21, 2024 03:09

baby230211 marked this pull request as ready for review October 20, 2024 04:28

baby230211 changed the title ~~[WIP] feat(gc): record workspace manifest and target dir in global cache tracker~~ feat(gc): record workspace manifest and target dir in global cache tracker Oct 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(gc): record workspace manifest and target dir in global cache tracker #13846

feat(gc): record workspace manifest and target dir in global cache tracker #13846

baby230211 commented May 2, 2024 •

edited

Loading

rustbot commented May 2, 2024

epage May 2, 2024

epage May 2, 2024

epage May 2, 2024

baby230211 May 22, 2024

epage May 2, 2024

epage May 2, 2024

baby230211 May 22, 2024 •

edited

Loading

epage May 22, 2024

baby230211 May 23, 2024

epage Jun 7, 2024

baby230211 Jun 15, 2024

epage May 2, 2024

bors commented Jun 7, 2024

epage commented Jun 20, 2024

baby230211 commented Jun 20, 2024

epage commented Jun 21, 2024

baby230211 commented Oct 16, 2024 •

edited

Loading

epage commented Oct 24, 2024

		pub encoded_workspace_manifest_name: InternedString,
		pub encoded_target_dir_name: InternedString,

feat(gc): record workspace manifest and target dir in global cache tracker #13846

Are you sure you want to change the base?

feat(gc): record workspace manifest and target dir in global cache tracker #13846

Conversation

baby230211 commented May 2, 2024 • edited Loading

What does this PR try to resolve?

How should we test and review this PR?

rustbot commented May 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baby230211 May 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Jun 7, 2024

epage commented Jun 20, 2024

baby230211 commented Jun 20, 2024

epage commented Jun 21, 2024

baby230211 commented Oct 16, 2024 • edited Loading

epage commented Oct 24, 2024

baby230211 commented May 2, 2024 •

edited

Loading

baby230211 May 22, 2024 •

edited

Loading

baby230211 commented Oct 16, 2024 •

edited

Loading