[WIP] 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

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

…acker

…recorded correctly

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 from 73a060c to 37b25eb Compare May 22, 2024 02:19

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

0eb4ca7

…acker

baby230211 force-pushed the feat-13136 branch 3 times, most recently from a1492d3 to 7d4fbbe Compare May 22, 2024 15:00

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

481de08

…recorded correctly

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

baby230211 commented May 2, 2024 •

edited

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

epage May 22, 2024

baby230211 May 23, 2024

epage May 2, 2024

		pub encoded_workspace_manifest_name: InternedString,
		pub encoded_target_dir_name: InternedString,

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

Are you sure you want to change the base?

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

Conversation

baby230211 commented May 2, 2024 • edited

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

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 commented May 2, 2024 •

edited

baby230211 May 22, 2024 •

edited