From 6c7a5ddc95b9fc9250bfab2e2eeef5f1d4b63470 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Tue, 24 Feb 2026 12:14:50 -0800 Subject: [PATCH 1/5] Expand missing prefetch mitigation usage Currently, the mitigation to download commit packs when we encounter missing trees during git history traversal only applies if PrefetchHasBeenDone() is false. This means that if cache servers are not providing the full up-to-date commit graph we won't apply the mitigation. - Remove PrefetchHasBeenDone() check from ShouldDownloadCommitPack and UpdateTreesForDownloadedCommits so the commit pack download mitigation applies regardless of whether prefetch has been done, protecting against cases where prefetch pack cache servers are not up-to-date. - Add LruCache to GVFS.Common - Replace Dictionary treesWithDownloadedCommits in InProcessMount with LruCache capped at 4000 (10 times the threshold per commit pack) entries to prevent unbounded growth since we are no longer limiting to only the period before first prefetch --- GVFS/GVFS.Common/LruCache.cs | 123 ++++++++ GVFS/GVFS.Mount/InProcessMount.cs | 44 ++- GVFS/GVFS.UnitTests/Common/LruCacheTests.cs | 294 ++++++++++++++++++++ 3 files changed, 433 insertions(+), 28 deletions(-) create mode 100644 GVFS/GVFS.Common/LruCache.cs create mode 100644 GVFS/GVFS.UnitTests/Common/LruCacheTests.cs diff --git a/GVFS/GVFS.Common/LruCache.cs b/GVFS/GVFS.Common/LruCache.cs new file mode 100644 index 000000000..9fc31b8d8 --- /dev/null +++ b/GVFS/GVFS.Common/LruCache.cs @@ -0,0 +1,123 @@ +using System.Collections.Generic; + +namespace GVFS.Common +{ + /// + /// A fixed-capacity dictionary that evicts the least-recently-used entry when full. + /// All operations are O(1) and thread-safe. + /// + /// + /// In future if we upgrade to .NET 10, we can replace this implementation with + /// one using OrderedDictionary which was added in .NET 9. + public class LruCache + { + private readonly int capacity; + private readonly Dictionary>> map; + private readonly LinkedList> order; + private readonly object syncLock = new object(); + + public LruCache(int capacity) + { + this.capacity = capacity; + this.map = new Dictionary>>(capacity); + this.order = new LinkedList>(); + } + + public int Count + { + get { lock (this.syncLock) { return this.map.Count; } } + } + + public bool TryGetValue(TKey key, out TValue value) + { + lock (this.syncLock) + { + if (this.map.TryGetValue(key, out var node)) + { + this.order.Remove(node); + this.order.AddFirst(node); + value = node.Value.Value; + return true; + } + + value = default(TValue); + return false; + } + } + + public void Set(TKey key, TValue value) + { + lock (this.syncLock) + { + if (this.map.TryGetValue(key, out var existing)) + { + this.order.Remove(existing); + this.map.Remove(key); + } + else if (this.map.Count >= this.capacity) + { + var lru = this.order.Last; + this.order.RemoveLast(); + this.map.Remove(lru.Value.Key); + } + + var node = new LinkedListNode>(new KeyValuePair(key, value)); + this.order.AddFirst(node); + this.map[key] = node; + } + } + + public bool Remove(TKey key) + { + lock (this.syncLock) + { + if (this.map.TryGetValue(key, out var node)) + { + this.order.Remove(node); + this.map.Remove(key); + return true; + } + + return false; + } + } + + /// + /// Returns a snapshot of all entries in MRU to LRU order. + /// + public IList> GetEntries() + { + lock (this.syncLock) + { + return new List>(this.order); + } + } + + /// + /// Removes all entries whose value equals . + /// Returns the number of entries removed. + /// + public int RemoveAllWithValue(TValue value) + { + lock (this.syncLock) + { + int removed = 0; + LinkedListNode> node = this.order.First; + while (node != null) + { + LinkedListNode> next = node.Next; + if (EqualityComparer.Default.Equals(node.Value.Value, value)) + { + this.map.Remove(node.Value.Key); + this.order.Remove(node); + removed++; + } + + node = next; + } + + return removed; + } + } + } +} diff --git a/GVFS/GVFS.Mount/InProcessMount.cs b/GVFS/GVFS.Mount/InProcessMount.cs index e6d43a842..e068d0838 100644 --- a/GVFS/GVFS.Mount/InProcessMount.cs +++ b/GVFS/GVFS.Mount/InProcessMount.cs @@ -33,6 +33,9 @@ public class InProcessMount // all the trees in a commit to ~2-3 seconds. private const int MissingTreeThresholdForDownloadingCommitPack = 200; + // Allows tracking up to ~20 commits worth of missing trees before the oldest entries are evicted. + private const int TreesWithDownloadedCommitsCapacity = MissingTreeThresholdForDownloadingCommitPack * 20; + private readonly bool showDebugWindow; private FileSystemCallbacks fileSystemCallbacks; @@ -52,7 +55,7 @@ public class InProcessMount private HeartbeatThread heartbeat; private ManualResetEvent unmountEvent; - private readonly Dictionary treesWithDownloadedCommits = new Dictionary(); + private readonly LruCache treesWithDownloadedCommits = new LruCache(TreesWithDownloadedCommitsCapacity); // True if InProcessMount is calling git reset as part of processing // a folder dehydrate request @@ -590,19 +593,20 @@ private void HandleDownloadObjectRequest(NamedPipeMessages.Message message, Name this.context.Repository.GVFSLock.Stats.RecordObjectDownload(objectType == Native.ObjectTypes.Blob, downloadTime.ElapsedMilliseconds); if (objectType == Native.ObjectTypes.Commit - && !this.PrefetchHasBeenDone() && !this.context.Repository.CommitAndRootTreeExists(objectSha, out var treeSha) && !string.IsNullOrEmpty(treeSha)) { /* If a commit is downloaded, it wasn't prefetched. - * If any prefetch has been done, there is probably a commit in the prefetch packs that is close enough that - * loose object download of missing trees will be faster than downloading a pack of all the trees for the commit. - * Otherwise, the trees for the commit may be needed soon depending on the context. + * The trees for the commit may be needed soon depending on the context. * e.g. git log (without a pathspec) doesn't need trees, but git checkout does. * + * If any prefetch has been done there is probably a similar commit/tree in the graph, + * but in case there isn't (such as if the cache server repack maintenance job is failing) + * we should still try to avoid downloading an excessive number of loose trees for a commit. + * * Save the tree/commit so if more trees are requested we can download all the trees for the commit in a batch. */ - this.treesWithDownloadedCommits[treeSha] = objectSha; + this.treesWithDownloadedCommits.Set(treeSha, objectSha); } } } @@ -610,21 +614,9 @@ private void HandleDownloadObjectRequest(NamedPipeMessages.Message message, Name connection.TrySendResponse(response.CreateMessage()); } - private bool PrefetchHasBeenDone() - { - var prefetchPacks = this.gitObjects.ReadPackFileNames(this.enlistment.GitPackRoot, GVFSConstants.PrefetchPackPrefix); - var result = prefetchPacks.Length > 0; - if (result) - { - this.treesWithDownloadedCommits.Clear(); - } - return result; - } - private bool ShouldDownloadCommitPack(string objectSha, out string commitSha) { - if (!this.treesWithDownloadedCommits.TryGetValue(objectSha, out commitSha) - || this.PrefetchHasBeenDone()) + if (!this.treesWithDownloadedCommits.TryGetValue(objectSha, out commitSha)) { return false; } @@ -635,7 +627,8 @@ private bool ShouldDownloadCommitPack(string objectSha, out string commitSha) * trees left, we'll probably need to download many more trees for the commit so we should download the pack. */ var commitShaLocal = commitSha; // can't use out parameter in lambda - int missingTreeCount = this.treesWithDownloadedCommits.Where(x => x.Value == commitShaLocal).Count(); + int missingTreeCount = this.treesWithDownloadedCommits.GetEntries().Where(x => x.Value == commitShaLocal).Count(); + return missingTreeCount > MissingTreeThresholdForDownloadingCommitPack; } @@ -646,8 +639,7 @@ private void UpdateTreesForDownloadedCommits(string objectSha) * as a heuristic to decide whether to batch download all the trees for the commit the * next time a missing one is requested. */ - if (!this.treesWithDownloadedCommits.TryGetValue(objectSha, out var commitSha) - || this.PrefetchHasBeenDone()) + if (!this.treesWithDownloadedCommits.TryGetValue(objectSha, out var commitSha)) { return; } @@ -662,18 +654,14 @@ private void UpdateTreesForDownloadedCommits(string objectSha) { foreach (var missingSubTree in missingSubTrees) { - this.treesWithDownloadedCommits[missingSubTree] = commitSha; + this.treesWithDownloadedCommits.Set(missingSubTree, commitSha); } } } private void DownloadedCommitPack(string commitSha) { - var toRemove = this.treesWithDownloadedCommits.Where(x => x.Value == commitSha).ToList(); - foreach (var tree in toRemove) - { - this.treesWithDownloadedCommits.Remove(tree.Key); - } + this.treesWithDownloadedCommits.RemoveAllWithValue(commitSha); } private void HandlePostFetchJobRequest(NamedPipeMessages.Message message, NamedPipeServer.Connection connection) diff --git a/GVFS/GVFS.UnitTests/Common/LruCacheTests.cs b/GVFS/GVFS.UnitTests/Common/LruCacheTests.cs new file mode 100644 index 000000000..23882d48e --- /dev/null +++ b/GVFS/GVFS.UnitTests/Common/LruCacheTests.cs @@ -0,0 +1,294 @@ +using GVFS.Common; +using GVFS.Tests.Should; +using NUnit.Framework; +using System.Collections.Generic; +using System.Threading; +using System.Threading.Tasks; + +namespace GVFS.UnitTests.Common +{ + [TestFixture] + public class LruCacheTests + { + [TestCase] + public void TryGetValue_ReturnsFalse_WhenEmpty() + { + LruCache cache = new LruCache(5); + + string value; + cache.TryGetValue("missing", out value).ShouldEqual(false); + value.ShouldBeNull(); + } + + [TestCase] + public void Set_And_TryGetValue_ReturnsValue() + { + LruCache cache = new LruCache(5); + + cache.Set("key", "value"); + + string value; + cache.TryGetValue("key", out value).ShouldEqual(true); + value.ShouldEqual("value"); + } + + [TestCase] + public void Count_ReflectsCurrentSize() + { + LruCache cache = new LruCache(5); + + cache.Count.ShouldEqual(0); + cache.Set("a", "1"); + cache.Count.ShouldEqual(1); + cache.Set("b", "2"); + cache.Count.ShouldEqual(2); + cache.Remove("a"); + cache.Count.ShouldEqual(1); + } + + [TestCase] + public void Set_OverwritesExistingKey() + { + LruCache cache = new LruCache(5); + + cache.Set("key", "first"); + cache.Set("key", "second"); + + cache.Count.ShouldEqual(1); + string value; + cache.TryGetValue("key", out value).ShouldEqual(true); + value.ShouldEqual("second"); + } + + [TestCase] + public void Set_EvictsLRU_WhenAtCapacity() + { + LruCache cache = new LruCache(3); + + cache.Set("a", "1"); + cache.Set("b", "2"); + cache.Set("c", "3"); + + // Adding a fourth entry should evict "a" (the oldest/LRU) + cache.Set("d", "4"); + + cache.Count.ShouldEqual(3); + string value; + cache.TryGetValue("a", out value).ShouldEqual(false); + cache.TryGetValue("b", out value).ShouldEqual(true); + cache.TryGetValue("c", out value).ShouldEqual(true); + cache.TryGetValue("d", out value).ShouldEqual(true); + } + + [TestCase] + public void TryGetValue_PromotesToMRU_SoItIsNotNextEvicted() + { + LruCache cache = new LruCache(3); + + cache.Set("a", "1"); + cache.Set("b", "2"); + cache.Set("c", "3"); + + // Access "a" to promote it to MRU — "b" becomes the LRU + string value; + cache.TryGetValue("a", out value); + + // Adding a fourth entry should now evict "b", not "a" + cache.Set("d", "4"); + + cache.Count.ShouldEqual(3); + cache.TryGetValue("a", out value).ShouldEqual(true); + cache.TryGetValue("b", out value).ShouldEqual(false); + cache.TryGetValue("c", out value).ShouldEqual(true); + cache.TryGetValue("d", out value).ShouldEqual(true); + } + + [TestCase] + public void Set_OverwriteExistingKey_DoesNotEvictOtherEntries() + { + LruCache cache = new LruCache(3); + + cache.Set("a", "1"); + cache.Set("b", "2"); + cache.Set("c", "3"); + + // Overwriting an existing key must not count as a new entry and must not trigger eviction + cache.Set("a", "updated"); + + cache.Count.ShouldEqual(3); + string value; + cache.TryGetValue("a", out value).ShouldEqual(true); + value.ShouldEqual("updated"); + cache.TryGetValue("b", out value).ShouldEqual(true); + cache.TryGetValue("c", out value).ShouldEqual(true); + } + + [TestCase] + public void Remove_ReturnsTrueAndRemovesEntry() + { + LruCache cache = new LruCache(5); + cache.Set("key", "value"); + + cache.Remove("key").ShouldEqual(true); + + cache.Count.ShouldEqual(0); + string value; + cache.TryGetValue("key", out value).ShouldEqual(false); + } + + [TestCase] + public void Remove_ReturnsFalse_WhenKeyNotPresent() + { + LruCache cache = new LruCache(5); + + cache.Remove("nonexistent").ShouldEqual(false); + } + + [TestCase] + public void GetEntries_ReturnsSnapshotInMRUOrder() + { + LruCache cache = new LruCache(5); + + cache.Set("a", "1"); + cache.Set("b", "2"); + cache.Set("c", "3"); + + // Inserted a, b, c ? MRU order: c, b, a + // Access "a" to promote it ? MRU order: a, c, b + string value; + cache.TryGetValue("a", out value); + + IList> entries = cache.GetEntries(); + + entries.Count.ShouldEqual(3); + entries[0].Key.ShouldEqual("a"); + entries[1].Key.ShouldEqual("c"); + entries[2].Key.ShouldEqual("b"); + } + + [TestCase] + public void GetEntries_ReturnsSnapshot_IndependentOfSubsequentMutations() + { + LruCache cache = new LruCache(5); + cache.Set("a", "1"); + cache.Set("b", "2"); + + IList> snapshot = cache.GetEntries(); + cache.Set("c", "3"); + cache.Remove("a"); + + // The snapshot must not be affected by mutations after it was taken + snapshot.Count.ShouldEqual(2); + } + + [TestCase] + public void RemoveAllWithValue_RemovesAllMatchingEntries() + { + LruCache cache = new LruCache(10); + + cache.Set("tree1", "commitA"); + cache.Set("tree2", "commitA"); + cache.Set("tree3", "commitB"); + cache.Set("tree4", "commitA"); + + int removed = cache.RemoveAllWithValue("commitA"); + + removed.ShouldEqual(3); + cache.Count.ShouldEqual(1); + string value; + cache.TryGetValue("tree1", out value).ShouldEqual(false); + cache.TryGetValue("tree2", out value).ShouldEqual(false); + cache.TryGetValue("tree4", out value).ShouldEqual(false); + cache.TryGetValue("tree3", out value).ShouldEqual(true); + value.ShouldEqual("commitB"); + } + + [TestCase] + public void RemoveAllWithValue_RetainsNonMatchingEntries() + { + LruCache cache = new LruCache(10); + + cache.Set("tree1", "commitA"); + cache.Set("tree2", "commitB"); + cache.Set("tree3", "commitC"); + + int removed = cache.RemoveAllWithValue("commitB"); + + removed.ShouldEqual(1); + cache.Count.ShouldEqual(2); + string value; + cache.TryGetValue("tree1", out value).ShouldEqual(true); + cache.TryGetValue("tree2", out value).ShouldEqual(false); + cache.TryGetValue("tree3", out value).ShouldEqual(true); + } + + [TestCase] + public void RemoveAllWithValue_ReturnsZero_WhenNoMatch() + { + LruCache cache = new LruCache(5); + cache.Set("tree1", "commitA"); + + int removed = cache.RemoveAllWithValue("commitX"); + + removed.ShouldEqual(0); + cache.Count.ShouldEqual(1); + } + + [TestCase] + public void RemoveAllWithValue_ReturnsZero_WhenEmpty() + { + LruCache cache = new LruCache(5); + + int removed = cache.RemoveAllWithValue("commitA"); + + removed.ShouldEqual(0); + } + + [TestCase] + public void ThreadSafety_ConcurrentSetAndGet_DoesNotThrow() + { + const int threadCount = 8; + const int operationsPerThread = 500; + const int capacity = 20; + + LruCache cache = new LruCache(capacity); + ManualResetEventSlim ready = new ManualResetEventSlim(false); + + Task[] tasks = new Task[threadCount]; + for (int t = 0; t < threadCount; t++) + { + int threadIndex = t; + tasks[t] = Task.Run(() => + { + ready.Wait(); + for (int i = 0; i < operationsPerThread; i++) + { + string key = "key" + ((threadIndex * operationsPerThread + i) % (capacity * 2)); + string value = "val" + i; + + cache.Set(key, value); + + string retrieved; + cache.TryGetValue(key, out retrieved); + + if (i % 10 == 0) + { + cache.Remove(key); + } + + if (i % 20 == 0) + { + cache.RemoveAllWithValue(value); + } + } + }); + } + + ready.Set(); + Task.WaitAll(tasks); + + // No exceptions thrown and count is within valid bounds + cache.Count.ShouldBeAtMost(capacity); + } + } +} From 6edff559bd4e8064a03458109daa964dda2cb14d Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Wed, 25 Feb 2026 16:09:44 -0800 Subject: [PATCH 2/5] Replace LruCache with MissingTreeTracker to further optimize lookups --- GVFS/GVFS.Common/LruCache.cs | 123 ------- GVFS/GVFS.Common/MissingTreeTracker.cs | 174 ++++++++++ GVFS/GVFS.Mount/InProcessMount.cs | 20 +- GVFS/GVFS.UnitTests/Common/LruCacheTests.cs | 294 ---------------- .../Common/MissingTreeTrackerTests.cs | 313 ++++++++++++++++++ 5 files changed, 497 insertions(+), 427 deletions(-) delete mode 100644 GVFS/GVFS.Common/LruCache.cs create mode 100644 GVFS/GVFS.Common/MissingTreeTracker.cs delete mode 100644 GVFS/GVFS.UnitTests/Common/LruCacheTests.cs create mode 100644 GVFS/GVFS.UnitTests/Common/MissingTreeTrackerTests.cs diff --git a/GVFS/GVFS.Common/LruCache.cs b/GVFS/GVFS.Common/LruCache.cs deleted file mode 100644 index 9fc31b8d8..000000000 --- a/GVFS/GVFS.Common/LruCache.cs +++ /dev/null @@ -1,123 +0,0 @@ -using System.Collections.Generic; - -namespace GVFS.Common -{ - /// - /// A fixed-capacity dictionary that evicts the least-recently-used entry when full. - /// All operations are O(1) and thread-safe. - /// - /// - /// In future if we upgrade to .NET 10, we can replace this implementation with - /// one using OrderedDictionary which was added in .NET 9. - public class LruCache - { - private readonly int capacity; - private readonly Dictionary>> map; - private readonly LinkedList> order; - private readonly object syncLock = new object(); - - public LruCache(int capacity) - { - this.capacity = capacity; - this.map = new Dictionary>>(capacity); - this.order = new LinkedList>(); - } - - public int Count - { - get { lock (this.syncLock) { return this.map.Count; } } - } - - public bool TryGetValue(TKey key, out TValue value) - { - lock (this.syncLock) - { - if (this.map.TryGetValue(key, out var node)) - { - this.order.Remove(node); - this.order.AddFirst(node); - value = node.Value.Value; - return true; - } - - value = default(TValue); - return false; - } - } - - public void Set(TKey key, TValue value) - { - lock (this.syncLock) - { - if (this.map.TryGetValue(key, out var existing)) - { - this.order.Remove(existing); - this.map.Remove(key); - } - else if (this.map.Count >= this.capacity) - { - var lru = this.order.Last; - this.order.RemoveLast(); - this.map.Remove(lru.Value.Key); - } - - var node = new LinkedListNode>(new KeyValuePair(key, value)); - this.order.AddFirst(node); - this.map[key] = node; - } - } - - public bool Remove(TKey key) - { - lock (this.syncLock) - { - if (this.map.TryGetValue(key, out var node)) - { - this.order.Remove(node); - this.map.Remove(key); - return true; - } - - return false; - } - } - - /// - /// Returns a snapshot of all entries in MRU to LRU order. - /// - public IList> GetEntries() - { - lock (this.syncLock) - { - return new List>(this.order); - } - } - - /// - /// Removes all entries whose value equals . - /// Returns the number of entries removed. - /// - public int RemoveAllWithValue(TValue value) - { - lock (this.syncLock) - { - int removed = 0; - LinkedListNode> node = this.order.First; - while (node != null) - { - LinkedListNode> next = node.Next; - if (EqualityComparer.Default.Equals(node.Value.Value, value)) - { - this.map.Remove(node.Value.Key); - this.order.Remove(node); - removed++; - } - - node = next; - } - - return removed; - } - } - } -} diff --git a/GVFS/GVFS.Common/MissingTreeTracker.cs b/GVFS/GVFS.Common/MissingTreeTracker.cs new file mode 100644 index 000000000..b1ee3eac6 --- /dev/null +++ b/GVFS/GVFS.Common/MissingTreeTracker.cs @@ -0,0 +1,174 @@ +using System.Collections.Generic; +using System.Linq; + +namespace GVFS.Common +{ + /// + /// Tracks missing trees per commit to support batching tree downloads. + /// Maintains LRU eviction based on commits (not individual trees). + /// + public class MissingTreeTracker + { + private readonly int capacity; + private readonly object syncLock = new object(); + + // Primary storage: commit -> set of missing trees + private readonly Dictionary> missingTreesByCommit; + + // Reverse lookup: tree -> commit (for fast lookups) + private readonly Dictionary commitByTree; + + // LRU ordering based on commits + private readonly LinkedList commitOrder; + private readonly Dictionary> commitNodes; + + public MissingTreeTracker(int capacity) + { + this.capacity = capacity; + this.missingTreesByCommit = new Dictionary>(); + this.commitByTree = new Dictionary(); + this.commitOrder = new LinkedList(); + this.commitNodes = new Dictionary>(); + } + + /// + /// Records a missing tree for a commit. Marks the commit as recently used. + /// + public void AddMissingTree(string treeSha, string commitSha) + { + lock (this.syncLock) + { + // If tree already tracked for a different commit, remove it first + if (this.commitByTree.TryGetValue(treeSha, out string existingCommit) && existingCommit != commitSha) + { + this.RemoveTreeFromCommit(treeSha, existingCommit); + } + + // Add or update the commit's missing trees + if (!this.missingTreesByCommit.TryGetValue(commitSha, out var trees)) + { + trees = new HashSet(); + this.missingTreesByCommit[commitSha] = trees; + + // Check capacity and evict LRU commit if needed + if (this.commitNodes.Count >= this.capacity) + { + this.EvictLruCommit(); + } + + // Add new commit node to the front (MRU) + var node = this.commitOrder.AddFirst(commitSha); + this.commitNodes[commitSha] = node; + } + else + { + // Move existing commit to front (mark as recently used) + this.MarkCommitAsUsed(commitSha); + } + + trees.Add(treeSha); + this.commitByTree[treeSha] = commitSha; + } + } + + /// + /// Tries to get the commit associated with a tree SHA. + /// + public bool TryGetCommit(string treeSha, out string commitSha) + { + lock (this.syncLock) + { + if (this.commitByTree.TryGetValue(treeSha, out commitSha)) + { + // Mark the commit as recently used + this.MarkCommitAsUsed(commitSha); + return true; + } + + commitSha = null; + return false; + } + } + + /// + /// Gets the count of missing trees for a specific commit. + /// + public int GetMissingTreeCount(string commitSha) + { + lock (this.syncLock) + { + if (this.missingTreesByCommit.TryGetValue(commitSha, out var trees)) + { + return trees.Count; + } + + return 0; + } + } + + /// + /// Removes all missing trees associated with a commit (e.g., after downloading the commit pack). + /// + public void RemoveCommit(string commitSha) + { + lock (this.syncLock) + { + if (!this.missingTreesByCommit.TryGetValue(commitSha, out var trees)) + { + return; + } + + // Remove all tree -> commit reverse lookups + foreach (var tree in trees) + { + this.commitByTree.Remove(tree); + } + + // Remove commit from primary storage + this.missingTreesByCommit.Remove(commitSha); + + // Remove from LRU order + if (this.commitNodes.TryGetValue(commitSha, out var node)) + { + this.commitOrder.Remove(node); + this.commitNodes.Remove(commitSha); + } + } + } + + private void MarkCommitAsUsed(string commitSha) + { + if (this.commitNodes.TryGetValue(commitSha, out var node)) + { + this.commitOrder.Remove(node); + var newNode = this.commitOrder.AddFirst(commitSha); + this.commitNodes[commitSha] = newNode; + } + } + + private void EvictLruCommit() + { + if (this.commitOrder.Last != null) + { + string lruCommit = this.commitOrder.Last.Value; + this.RemoveCommit(lruCommit); + } + } + + private void RemoveTreeFromCommit(string treeSha, string commitSha) + { + if (this.missingTreesByCommit.TryGetValue(commitSha, out var trees)) + { + trees.Remove(treeSha); + + // If no more trees for this commit, remove the commit entirely + if (trees.Count == 0) + { + this.RemoveCommit(commitSha); + } + } + + this.commitByTree.Remove(treeSha); + } + } +} diff --git a/GVFS/GVFS.Mount/InProcessMount.cs b/GVFS/GVFS.Mount/InProcessMount.cs index e068d0838..87a579e44 100644 --- a/GVFS/GVFS.Mount/InProcessMount.cs +++ b/GVFS/GVFS.Mount/InProcessMount.cs @@ -33,8 +33,9 @@ public class InProcessMount // all the trees in a commit to ~2-3 seconds. private const int MissingTreeThresholdForDownloadingCommitPack = 200; - // Allows tracking up to ~20 commits worth of missing trees before the oldest entries are evicted. - private const int TreesWithDownloadedCommitsCapacity = MissingTreeThresholdForDownloadingCommitPack * 20; + // Number of commits with missing trees to track with LRU eviction. This is to support batching tree + // downloads without using an unbounded amount of memory. + private const int TrackedCommitsCapacity = 20; private readonly bool showDebugWindow; @@ -55,7 +56,7 @@ public class InProcessMount private HeartbeatThread heartbeat; private ManualResetEvent unmountEvent; - private readonly LruCache treesWithDownloadedCommits = new LruCache(TreesWithDownloadedCommitsCapacity); + private readonly MissingTreeTracker missingTreeTracker = new MissingTreeTracker(TrackedCommitsCapacity); // True if InProcessMount is calling git reset as part of processing // a folder dehydrate request @@ -606,7 +607,7 @@ private void HandleDownloadObjectRequest(NamedPipeMessages.Message message, Name * * Save the tree/commit so if more trees are requested we can download all the trees for the commit in a batch. */ - this.treesWithDownloadedCommits.Set(treeSha, objectSha); + this.missingTreeTracker.AddMissingTree(treeSha, objectSha); } } } @@ -616,7 +617,7 @@ private void HandleDownloadObjectRequest(NamedPipeMessages.Message message, Name private bool ShouldDownloadCommitPack(string objectSha, out string commitSha) { - if (!this.treesWithDownloadedCommits.TryGetValue(objectSha, out commitSha)) + if (!this.missingTreeTracker.TryGetCommit(objectSha, out commitSha)) { return false; } @@ -626,8 +627,7 @@ private bool ShouldDownloadCommitPack(string objectSha, out string commitSha) * Conversely, if we know (from previously downloaded missing trees) that a commit has a lot of missing * trees left, we'll probably need to download many more trees for the commit so we should download the pack. */ - var commitShaLocal = commitSha; // can't use out parameter in lambda - int missingTreeCount = this.treesWithDownloadedCommits.GetEntries().Where(x => x.Value == commitShaLocal).Count(); + int missingTreeCount = this.missingTreeTracker.GetMissingTreeCount(commitSha); return missingTreeCount > MissingTreeThresholdForDownloadingCommitPack; } @@ -639,7 +639,7 @@ private void UpdateTreesForDownloadedCommits(string objectSha) * as a heuristic to decide whether to batch download all the trees for the commit the * next time a missing one is requested. */ - if (!this.treesWithDownloadedCommits.TryGetValue(objectSha, out var commitSha)) + if (!this.missingTreeTracker.TryGetCommit(objectSha, out var commitSha)) { return; } @@ -654,14 +654,14 @@ private void UpdateTreesForDownloadedCommits(string objectSha) { foreach (var missingSubTree in missingSubTrees) { - this.treesWithDownloadedCommits.Set(missingSubTree, commitSha); + this.missingTreeTracker.AddMissingTree(missingSubTree, commitSha); } } } private void DownloadedCommitPack(string commitSha) { - this.treesWithDownloadedCommits.RemoveAllWithValue(commitSha); + this.missingTreeTracker.RemoveCommit(commitSha); } private void HandlePostFetchJobRequest(NamedPipeMessages.Message message, NamedPipeServer.Connection connection) diff --git a/GVFS/GVFS.UnitTests/Common/LruCacheTests.cs b/GVFS/GVFS.UnitTests/Common/LruCacheTests.cs deleted file mode 100644 index 23882d48e..000000000 --- a/GVFS/GVFS.UnitTests/Common/LruCacheTests.cs +++ /dev/null @@ -1,294 +0,0 @@ -using GVFS.Common; -using GVFS.Tests.Should; -using NUnit.Framework; -using System.Collections.Generic; -using System.Threading; -using System.Threading.Tasks; - -namespace GVFS.UnitTests.Common -{ - [TestFixture] - public class LruCacheTests - { - [TestCase] - public void TryGetValue_ReturnsFalse_WhenEmpty() - { - LruCache cache = new LruCache(5); - - string value; - cache.TryGetValue("missing", out value).ShouldEqual(false); - value.ShouldBeNull(); - } - - [TestCase] - public void Set_And_TryGetValue_ReturnsValue() - { - LruCache cache = new LruCache(5); - - cache.Set("key", "value"); - - string value; - cache.TryGetValue("key", out value).ShouldEqual(true); - value.ShouldEqual("value"); - } - - [TestCase] - public void Count_ReflectsCurrentSize() - { - LruCache cache = new LruCache(5); - - cache.Count.ShouldEqual(0); - cache.Set("a", "1"); - cache.Count.ShouldEqual(1); - cache.Set("b", "2"); - cache.Count.ShouldEqual(2); - cache.Remove("a"); - cache.Count.ShouldEqual(1); - } - - [TestCase] - public void Set_OverwritesExistingKey() - { - LruCache cache = new LruCache(5); - - cache.Set("key", "first"); - cache.Set("key", "second"); - - cache.Count.ShouldEqual(1); - string value; - cache.TryGetValue("key", out value).ShouldEqual(true); - value.ShouldEqual("second"); - } - - [TestCase] - public void Set_EvictsLRU_WhenAtCapacity() - { - LruCache cache = new LruCache(3); - - cache.Set("a", "1"); - cache.Set("b", "2"); - cache.Set("c", "3"); - - // Adding a fourth entry should evict "a" (the oldest/LRU) - cache.Set("d", "4"); - - cache.Count.ShouldEqual(3); - string value; - cache.TryGetValue("a", out value).ShouldEqual(false); - cache.TryGetValue("b", out value).ShouldEqual(true); - cache.TryGetValue("c", out value).ShouldEqual(true); - cache.TryGetValue("d", out value).ShouldEqual(true); - } - - [TestCase] - public void TryGetValue_PromotesToMRU_SoItIsNotNextEvicted() - { - LruCache cache = new LruCache(3); - - cache.Set("a", "1"); - cache.Set("b", "2"); - cache.Set("c", "3"); - - // Access "a" to promote it to MRU — "b" becomes the LRU - string value; - cache.TryGetValue("a", out value); - - // Adding a fourth entry should now evict "b", not "a" - cache.Set("d", "4"); - - cache.Count.ShouldEqual(3); - cache.TryGetValue("a", out value).ShouldEqual(true); - cache.TryGetValue("b", out value).ShouldEqual(false); - cache.TryGetValue("c", out value).ShouldEqual(true); - cache.TryGetValue("d", out value).ShouldEqual(true); - } - - [TestCase] - public void Set_OverwriteExistingKey_DoesNotEvictOtherEntries() - { - LruCache cache = new LruCache(3); - - cache.Set("a", "1"); - cache.Set("b", "2"); - cache.Set("c", "3"); - - // Overwriting an existing key must not count as a new entry and must not trigger eviction - cache.Set("a", "updated"); - - cache.Count.ShouldEqual(3); - string value; - cache.TryGetValue("a", out value).ShouldEqual(true); - value.ShouldEqual("updated"); - cache.TryGetValue("b", out value).ShouldEqual(true); - cache.TryGetValue("c", out value).ShouldEqual(true); - } - - [TestCase] - public void Remove_ReturnsTrueAndRemovesEntry() - { - LruCache cache = new LruCache(5); - cache.Set("key", "value"); - - cache.Remove("key").ShouldEqual(true); - - cache.Count.ShouldEqual(0); - string value; - cache.TryGetValue("key", out value).ShouldEqual(false); - } - - [TestCase] - public void Remove_ReturnsFalse_WhenKeyNotPresent() - { - LruCache cache = new LruCache(5); - - cache.Remove("nonexistent").ShouldEqual(false); - } - - [TestCase] - public void GetEntries_ReturnsSnapshotInMRUOrder() - { - LruCache cache = new LruCache(5); - - cache.Set("a", "1"); - cache.Set("b", "2"); - cache.Set("c", "3"); - - // Inserted a, b, c ? MRU order: c, b, a - // Access "a" to promote it ? MRU order: a, c, b - string value; - cache.TryGetValue("a", out value); - - IList> entries = cache.GetEntries(); - - entries.Count.ShouldEqual(3); - entries[0].Key.ShouldEqual("a"); - entries[1].Key.ShouldEqual("c"); - entries[2].Key.ShouldEqual("b"); - } - - [TestCase] - public void GetEntries_ReturnsSnapshot_IndependentOfSubsequentMutations() - { - LruCache cache = new LruCache(5); - cache.Set("a", "1"); - cache.Set("b", "2"); - - IList> snapshot = cache.GetEntries(); - cache.Set("c", "3"); - cache.Remove("a"); - - // The snapshot must not be affected by mutations after it was taken - snapshot.Count.ShouldEqual(2); - } - - [TestCase] - public void RemoveAllWithValue_RemovesAllMatchingEntries() - { - LruCache cache = new LruCache(10); - - cache.Set("tree1", "commitA"); - cache.Set("tree2", "commitA"); - cache.Set("tree3", "commitB"); - cache.Set("tree4", "commitA"); - - int removed = cache.RemoveAllWithValue("commitA"); - - removed.ShouldEqual(3); - cache.Count.ShouldEqual(1); - string value; - cache.TryGetValue("tree1", out value).ShouldEqual(false); - cache.TryGetValue("tree2", out value).ShouldEqual(false); - cache.TryGetValue("tree4", out value).ShouldEqual(false); - cache.TryGetValue("tree3", out value).ShouldEqual(true); - value.ShouldEqual("commitB"); - } - - [TestCase] - public void RemoveAllWithValue_RetainsNonMatchingEntries() - { - LruCache cache = new LruCache(10); - - cache.Set("tree1", "commitA"); - cache.Set("tree2", "commitB"); - cache.Set("tree3", "commitC"); - - int removed = cache.RemoveAllWithValue("commitB"); - - removed.ShouldEqual(1); - cache.Count.ShouldEqual(2); - string value; - cache.TryGetValue("tree1", out value).ShouldEqual(true); - cache.TryGetValue("tree2", out value).ShouldEqual(false); - cache.TryGetValue("tree3", out value).ShouldEqual(true); - } - - [TestCase] - public void RemoveAllWithValue_ReturnsZero_WhenNoMatch() - { - LruCache cache = new LruCache(5); - cache.Set("tree1", "commitA"); - - int removed = cache.RemoveAllWithValue("commitX"); - - removed.ShouldEqual(0); - cache.Count.ShouldEqual(1); - } - - [TestCase] - public void RemoveAllWithValue_ReturnsZero_WhenEmpty() - { - LruCache cache = new LruCache(5); - - int removed = cache.RemoveAllWithValue("commitA"); - - removed.ShouldEqual(0); - } - - [TestCase] - public void ThreadSafety_ConcurrentSetAndGet_DoesNotThrow() - { - const int threadCount = 8; - const int operationsPerThread = 500; - const int capacity = 20; - - LruCache cache = new LruCache(capacity); - ManualResetEventSlim ready = new ManualResetEventSlim(false); - - Task[] tasks = new Task[threadCount]; - for (int t = 0; t < threadCount; t++) - { - int threadIndex = t; - tasks[t] = Task.Run(() => - { - ready.Wait(); - for (int i = 0; i < operationsPerThread; i++) - { - string key = "key" + ((threadIndex * operationsPerThread + i) % (capacity * 2)); - string value = "val" + i; - - cache.Set(key, value); - - string retrieved; - cache.TryGetValue(key, out retrieved); - - if (i % 10 == 0) - { - cache.Remove(key); - } - - if (i % 20 == 0) - { - cache.RemoveAllWithValue(value); - } - } - }); - } - - ready.Set(); - Task.WaitAll(tasks); - - // No exceptions thrown and count is within valid bounds - cache.Count.ShouldBeAtMost(capacity); - } - } -} diff --git a/GVFS/GVFS.UnitTests/Common/MissingTreeTrackerTests.cs b/GVFS/GVFS.UnitTests/Common/MissingTreeTrackerTests.cs new file mode 100644 index 000000000..c65d102a2 --- /dev/null +++ b/GVFS/GVFS.UnitTests/Common/MissingTreeTrackerTests.cs @@ -0,0 +1,313 @@ +using GVFS.Common; +using GVFS.Tests.Should; +using NUnit.Framework; + +namespace GVFS.UnitTests.Common +{ + [TestFixture] + public class MissingTreeTrackerTests + { + [TestCase] + public void AddMissingTree_SingleTreeAndCommit() + { + MissingTreeTracker tracker = new MissingTreeTracker(capacity: 10); + + tracker.AddMissingTree("tree1", "commit1"); + + tracker.TryGetCommit("tree1", out string commitSha).ShouldEqual(true); + commitSha.ShouldEqual("commit1"); + tracker.GetMissingTreeCount("commit1").ShouldEqual(1); + } + + [TestCase] + public void AddMissingTree_MultipleTreesForSameCommit() + { + MissingTreeTracker tracker = new MissingTreeTracker(capacity: 10); + + tracker.AddMissingTree("tree1", "commit1"); + tracker.AddMissingTree("tree2", "commit1"); + tracker.AddMissingTree("tree3", "commit1"); + + tracker.GetMissingTreeCount("commit1").ShouldEqual(3); + + tracker.TryGetCommit("tree1", out string commit1).ShouldEqual(true); + commit1.ShouldEqual("commit1"); + + tracker.TryGetCommit("tree2", out string commit2).ShouldEqual(true); + commit2.ShouldEqual("commit1"); + + tracker.TryGetCommit("tree3", out string commit3).ShouldEqual(true); + commit3.ShouldEqual("commit1"); + } + + [TestCase] + public void AddMissingTree_SameTreeAddedTwiceToSameCommit() + { + MissingTreeTracker tracker = new MissingTreeTracker(capacity: 10); + + tracker.AddMissingTree("tree1", "commit1"); + tracker.AddMissingTree("tree1", "commit1"); + + tracker.GetMissingTreeCount("commit1").ShouldEqual(1); + } + + [TestCase] + public void AddMissingTree_TreeReassociatedWithDifferentCommit() + { + MissingTreeTracker tracker = new MissingTreeTracker(capacity: 10); + + // Add tree to first commit + tracker.AddMissingTree("tree1", "commit1"); + tracker.GetMissingTreeCount("commit1").ShouldEqual(1); + + // Reassociate same tree with different commit + tracker.AddMissingTree("tree1", "commit2"); + + // Tree should now be associated with commit2 + tracker.TryGetCommit("tree1", out string commitSha).ShouldEqual(true); + commitSha.ShouldEqual("commit2"); + + // commit1 should have 0 trees (and be removed) + tracker.GetMissingTreeCount("commit1").ShouldEqual(0); + + // commit2 should have 1 tree + tracker.GetMissingTreeCount("commit2").ShouldEqual(1); + } + + [TestCase] + public void AddMissingTree_TreeReassociatedWithDifferentCommit_OriginalCommitHasOtherTrees() + { + MissingTreeTracker tracker = new MissingTreeTracker(capacity: 10); + + // Add multiple trees to first commit + tracker.AddMissingTree("tree1", "commit1"); + tracker.AddMissingTree("tree2", "commit1"); + tracker.GetMissingTreeCount("commit1").ShouldEqual(2); + + // Reassociate tree1 with different commit + tracker.AddMissingTree("tree1", "commit2"); + + // Tree1 should now be associated with commit2 + tracker.TryGetCommit("tree1", out string commitSha).ShouldEqual(true); + commitSha.ShouldEqual("commit2"); + + // commit1 should still exist with 1 tree + tracker.GetMissingTreeCount("commit1").ShouldEqual(1); + + // commit2 should have 1 tree + tracker.GetMissingTreeCount("commit2").ShouldEqual(1); + + // tree2 should still be associated with commit1 + tracker.TryGetCommit("tree2", out string tree2Commit).ShouldEqual(true); + tree2Commit.ShouldEqual("commit1"); + } + + [TestCase] + public void TryGetCommit_NonExistentTree() + { + MissingTreeTracker tracker = new MissingTreeTracker(capacity: 10); + + tracker.TryGetCommit("nonexistent", out string commitSha).ShouldEqual(false); + commitSha.ShouldEqual(null); + } + + [TestCase] + public void GetMissingTreeCount_NonExistentCommit() + { + MissingTreeTracker tracker = new MissingTreeTracker(capacity: 10); + + tracker.GetMissingTreeCount("nonexistent").ShouldEqual(0); + } + + [TestCase] + public void RemoveCommit_RemovesAllTrees() + { + MissingTreeTracker tracker = new MissingTreeTracker(capacity: 10); + + tracker.AddMissingTree("tree1", "commit1"); + tracker.AddMissingTree("tree2", "commit1"); + tracker.AddMissingTree("tree3", "commit1"); + + tracker.GetMissingTreeCount("commit1").ShouldEqual(3); + + tracker.RemoveCommit("commit1"); + + tracker.GetMissingTreeCount("commit1").ShouldEqual(0); + tracker.TryGetCommit("tree1", out string _).ShouldEqual(false); + tracker.TryGetCommit("tree2", out string _).ShouldEqual(false); + tracker.TryGetCommit("tree3", out string _).ShouldEqual(false); + } + + [TestCase] + public void RemoveCommit_NonExistentCommit() + { + MissingTreeTracker tracker = new MissingTreeTracker(capacity: 10); + + // Should not throw + tracker.RemoveCommit("nonexistent"); + } + + [TestCase] + public void RemoveCommit_DoesNotAffectOtherCommits() + { + MissingTreeTracker tracker = new MissingTreeTracker(capacity: 10); + + tracker.AddMissingTree("tree1", "commit1"); + tracker.AddMissingTree("tree2", "commit2"); + + tracker.RemoveCommit("commit1"); + + tracker.GetMissingTreeCount("commit1").ShouldEqual(0); + tracker.GetMissingTreeCount("commit2").ShouldEqual(1); + tracker.TryGetCommit("tree2", out string commitSha).ShouldEqual(true); + commitSha.ShouldEqual("commit2"); + } + + [TestCase] + public void LruEviction_EvictsOldestCommit() + { + MissingTreeTracker tracker = new MissingTreeTracker(capacity: 3); + + tracker.AddMissingTree("tree1", "commit1"); + tracker.AddMissingTree("tree2", "commit2"); + tracker.AddMissingTree("tree3", "commit3"); + + // All three commits should exist + tracker.GetMissingTreeCount("commit1").ShouldEqual(1); + tracker.GetMissingTreeCount("commit2").ShouldEqual(1); + tracker.GetMissingTreeCount("commit3").ShouldEqual(1); + + // Adding a fourth commit should evict commit1 (oldest) + tracker.AddMissingTree("tree4", "commit4"); + + tracker.GetMissingTreeCount("commit1").ShouldEqual(0); + tracker.TryGetCommit("tree1", out string _).ShouldEqual(false); + + tracker.GetMissingTreeCount("commit2").ShouldEqual(1); + tracker.GetMissingTreeCount("commit3").ShouldEqual(1); + tracker.GetMissingTreeCount("commit4").ShouldEqual(1); + } + + [TestCase] + public void LruEviction_AddingTreeToExistingCommitUpdatesLru() + { + MissingTreeTracker tracker = new MissingTreeTracker(capacity: 3); + + tracker.AddMissingTree("tree1", "commit1"); + tracker.AddMissingTree("tree2", "commit2"); + tracker.AddMissingTree("tree3", "commit3"); + + // Access commit1 by adding another tree to it (marks it as recently used) + tracker.AddMissingTree("tree1b", "commit1"); + + // Adding a fourth commit should evict commit2 (now oldest) + tracker.AddMissingTree("tree4", "commit4"); + + tracker.GetMissingTreeCount("commit1").ShouldEqual(2); // Still has tree1 and tree1b + tracker.GetMissingTreeCount("commit2").ShouldEqual(0); // Evicted + tracker.GetMissingTreeCount("commit3").ShouldEqual(1); + tracker.GetMissingTreeCount("commit4").ShouldEqual(1); + } + + [TestCase] + public void LruEviction_TryGetCommitUpdatesLru() + { + MissingTreeTracker tracker = new MissingTreeTracker(capacity: 3); + + tracker.AddMissingTree("tree1", "commit1"); + tracker.AddMissingTree("tree2", "commit2"); + tracker.AddMissingTree("tree3", "commit3"); + + // Access commit1 via TryGetCommit (marks it as recently used) + tracker.TryGetCommit("tree1", out string _); + + // Adding a fourth commit should evict commit2 (now oldest) + tracker.AddMissingTree("tree4", "commit4"); + + tracker.GetMissingTreeCount("commit1").ShouldEqual(1); // Still exists + tracker.GetMissingTreeCount("commit2").ShouldEqual(0); // Evicted + tracker.GetMissingTreeCount("commit3").ShouldEqual(1); + tracker.GetMissingTreeCount("commit4").ShouldEqual(1); + } + + [TestCase] + public void LruEviction_EvictsMultipleTrees() + { + MissingTreeTracker tracker = new MissingTreeTracker(capacity: 2); + + tracker.AddMissingTree("tree1", "commit1"); + tracker.AddMissingTree("tree2", "commit1"); + tracker.AddMissingTree("tree3", "commit1"); + + tracker.AddMissingTree("tree4", "commit2"); + + tracker.GetMissingTreeCount("commit1").ShouldEqual(3); + tracker.GetMissingTreeCount("commit2").ShouldEqual(1); + + // Adding a third commit should evict commit1 with all its trees + tracker.AddMissingTree("tree5", "commit3"); + + tracker.GetMissingTreeCount("commit1").ShouldEqual(0); + tracker.TryGetCommit("tree1", out string _).ShouldEqual(false); + tracker.TryGetCommit("tree2", out string _).ShouldEqual(false); + tracker.TryGetCommit("tree3", out string _).ShouldEqual(false); + + tracker.GetMissingTreeCount("commit2").ShouldEqual(1); + tracker.GetMissingTreeCount("commit3").ShouldEqual(1); + } + + [TestCase] + public void Capacity_One() + { + MissingTreeTracker tracker = new MissingTreeTracker(capacity: 1); + + tracker.AddMissingTree("tree1", "commit1"); + tracker.GetMissingTreeCount("commit1").ShouldEqual(1); + + tracker.AddMissingTree("tree2", "commit2"); + + tracker.GetMissingTreeCount("commit1").ShouldEqual(0); + tracker.GetMissingTreeCount("commit2").ShouldEqual(1); + } + + [TestCase] + public void AddMissingTree_MultipleTrees_ChecksCount() + { + MissingTreeTracker tracker = new MissingTreeTracker(capacity: 10); + + tracker.AddMissingTree("tree1", "commit1"); + tracker.GetMissingTreeCount("commit1").ShouldEqual(1); + + tracker.AddMissingTree("tree2", "commit1"); + tracker.GetMissingTreeCount("commit1").ShouldEqual(2); + + tracker.AddMissingTree("tree3", "commit1"); + tracker.GetMissingTreeCount("commit1").ShouldEqual(3); + + tracker.AddMissingTree("tree4", "commit1"); + tracker.AddMissingTree("tree5", "commit1"); + tracker.GetMissingTreeCount("commit1").ShouldEqual(5); + } + + [TestCase] + public void GetMissingTreeCount_DoesNotUpdateLru() + { + MissingTreeTracker tracker = new MissingTreeTracker(capacity: 3); + + tracker.AddMissingTree("tree1", "commit1"); + tracker.AddMissingTree("tree2", "commit2"); + tracker.AddMissingTree("tree3", "commit3"); + + // Query commit1's count (should not update LRU) + tracker.GetMissingTreeCount("commit1"); + + // Adding a fourth commit should still evict commit1 (oldest) + tracker.AddMissingTree("tree4", "commit4"); + + tracker.GetMissingTreeCount("commit1").ShouldEqual(0); + tracker.GetMissingTreeCount("commit2").ShouldEqual(1); + tracker.GetMissingTreeCount("commit3").ShouldEqual(1); + tracker.GetMissingTreeCount("commit4").ShouldEqual(1); + } + } +} From 4c7e7bcc503a2e18abf15d6a5773a623ac73644d Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Thu, 26 Feb 2026 10:08:57 -0800 Subject: [PATCH 3/5] MissingTreeTracker - use Trees for capacity, track multiple commits per tree --- GVFS/GVFS.Common/MissingTreeTracker.cs | 248 +++++-- GVFS/GVFS.Mount/InProcessMount.cs | 26 +- .../Common/MissingTreeTrackerTests.cs | 647 +++++++++++------- 3 files changed, 605 insertions(+), 316 deletions(-) diff --git a/GVFS/GVFS.Common/MissingTreeTracker.cs b/GVFS/GVFS.Common/MissingTreeTracker.cs index b1ee3eac6..7dd0bf526 100644 --- a/GVFS/GVFS.Common/MissingTreeTracker.cs +++ b/GVFS/GVFS.Common/MissingTreeTracker.cs @@ -1,3 +1,4 @@ +using System; using System.Collections.Generic; using System.Linq; @@ -6,134 +7,167 @@ namespace GVFS.Common /// /// Tracks missing trees per commit to support batching tree downloads. /// Maintains LRU eviction based on commits (not individual trees). + /// A single tree SHA may be shared across multiple commits. /// public class MissingTreeTracker { - private readonly int capacity; + private readonly int treeCapacity; private readonly object syncLock = new object(); // Primary storage: commit -> set of missing trees private readonly Dictionary> missingTreesByCommit; - // Reverse lookup: tree -> commit (for fast lookups) - private readonly Dictionary commitByTree; + // Reverse lookup: tree -> set of commits (for fast lookups) + private readonly Dictionary> commitsByTree; // LRU ordering based on commits private readonly LinkedList commitOrder; private readonly Dictionary> commitNodes; - public MissingTreeTracker(int capacity) + public MissingTreeTracker(int treeCapacity) { - this.capacity = capacity; - this.missingTreesByCommit = new Dictionary>(); - this.commitByTree = new Dictionary(); + this.treeCapacity = treeCapacity; + this.missingTreesByCommit = new Dictionary>(StringComparer.OrdinalIgnoreCase); + this.commitsByTree = new Dictionary>(StringComparer.OrdinalIgnoreCase); this.commitOrder = new LinkedList(); - this.commitNodes = new Dictionary>(); + this.commitNodes = new Dictionary>(StringComparer.OrdinalIgnoreCase); } /// - /// Records a missing tree for a commit. Marks the commit as recently used. + /// Records a missing root tree for a commit. Marks the commit as recently used. + /// A tree may be associated with multiple commits. /// - public void AddMissingTree(string treeSha, string commitSha) + public void AddMissingRootTree(string treeSha, string commitSha) { lock (this.syncLock) { - // If tree already tracked for a different commit, remove it first - if (this.commitByTree.TryGetValue(treeSha, out string existingCommit) && existingCommit != commitSha) + this.EnsureCommitTracked(commitSha); + this.AddTreeToCommit(treeSha, commitSha); + } + } + + /// + /// Records missing sub-trees discovered while processing a parent tree. + /// Each sub-tree is associated with all commits currently tracking the parent tree. + /// + public void AddMissingSubTrees(string parentTreeSha, string[] subTreeShas) + { + lock (this.syncLock) + { + if (!this.commitsByTree.TryGetValue(parentTreeSha, out var commits)) { - this.RemoveTreeFromCommit(treeSha, existingCommit); + return; } - // Add or update the commit's missing trees - if (!this.missingTreesByCommit.TryGetValue(commitSha, out var trees)) + // Snapshot the set because AddTreeToCommit may modify commitsByTree indirectly + string[] commitSnapshot = commits.ToArray(); + foreach (string subTreeSha in subTreeShas) { - trees = new HashSet(); - this.missingTreesByCommit[commitSha] = trees; - - // Check capacity and evict LRU commit if needed - if (this.commitNodes.Count >= this.capacity) + foreach (string commitSha in commitSnapshot) { - this.EvictLruCommit(); - } + /* Ensure it wasn't evicted earlier in the loop. */ + if (!this.missingTreesByCommit.ContainsKey(commitSha)) + { + continue; + } - // Add new commit node to the front (MRU) - var node = this.commitOrder.AddFirst(commitSha); - this.commitNodes[commitSha] = node; - } - else - { - // Move existing commit to front (mark as recently used) - this.MarkCommitAsUsed(commitSha); + this.AddTreeToCommit(subTreeSha, commitSha); + } } - - trees.Add(treeSha); - this.commitByTree[treeSha] = commitSha; } } /// - /// Tries to get the commit associated with a tree SHA. + /// Tries to get all commits associated with a tree SHA. + /// Marks all found commits as recently used. /// - public bool TryGetCommit(string treeSha, out string commitSha) + public bool TryGetCommits(string treeSha, out string[] commitShas) { lock (this.syncLock) { - if (this.commitByTree.TryGetValue(treeSha, out commitSha)) + if (this.commitsByTree.TryGetValue(treeSha, out var commits)) { - // Mark the commit as recently used - this.MarkCommitAsUsed(commitSha); + commitShas = commits.ToArray(); + foreach (string commitSha in commitShas) + { + this.MarkCommitAsUsed(commitSha); + } + return true; } - commitSha = null; + commitShas = null; return false; } } /// - /// Gets the count of missing trees for a specific commit. + /// Given a set of commits, finds the one with the most missing trees. /// - public int GetMissingTreeCount(string commitSha) + public int GetHighestMissingTreeCount(string[] commitShas, out string highestCountCommitSha) { lock (this.syncLock) { - if (this.missingTreesByCommit.TryGetValue(commitSha, out var trees)) + highestCountCommitSha = null; + int highestCount = 0; + + foreach (string commitSha in commitShas) { - return trees.Count; + if (this.missingTreesByCommit.TryGetValue(commitSha, out var trees) + && trees.Count > highestCount) + { + highestCount = trees.Count; + highestCountCommitSha = commitSha; + } } - return 0; + return highestCount; } } /// - /// Removes all missing trees associated with a commit (e.g., after downloading the commit pack). + /// Marks a commit as complete (e.g. its pack was downloaded successfully). + /// Because the trees are now available, they are also removed from tracking + /// for any other commits that shared them, and those commits are cleaned up + /// if they become empty. /// - public void RemoveCommit(string commitSha) + public void MarkCommitComplete(string commitSha) { lock (this.syncLock) { - if (!this.missingTreesByCommit.TryGetValue(commitSha, out var trees)) - { - return; - } - - // Remove all tree -> commit reverse lookups - foreach (var tree in trees) - { - this.commitByTree.Remove(tree); - } + this.RemoveCommitWithCascade(commitSha); + } + } - // Remove commit from primary storage - this.missingTreesByCommit.Remove(commitSha); + private void EnsureCommitTracked(string commitSha) + { + if (!this.missingTreesByCommit.TryGetValue(commitSha, out _)) + { + this.missingTreesByCommit[commitSha] = new HashSet(StringComparer.OrdinalIgnoreCase); + var node = this.commitOrder.AddFirst(commitSha); + this.commitNodes[commitSha] = node; + } + else + { + this.MarkCommitAsUsed(commitSha); + } + } - // Remove from LRU order - if (this.commitNodes.TryGetValue(commitSha, out var node)) + private void AddTreeToCommit(string treeSha, string commitSha) + { + if (!this.commitsByTree.ContainsKey(treeSha)) + { + // Evict LRU commits until there is room for the new tree + while (this.commitsByTree.Count >= this.treeCapacity) { - this.commitOrder.Remove(node); - this.commitNodes.Remove(commitSha); + this.EvictLruCommit(); } + + this.commitsByTree[treeSha] = new HashSet(StringComparer.OrdinalIgnoreCase); } + + this.missingTreesByCommit[commitSha].Add(treeSha); + this.commitsByTree[treeSha].Add(commitSha); } private void MarkCommitAsUsed(string commitSha) @@ -151,24 +185,98 @@ private void EvictLruCommit() if (this.commitOrder.Last != null) { string lruCommit = this.commitOrder.Last.Value; - this.RemoveCommit(lruCommit); + this.RemoveCommitNoCache(lruCommit); } } - private void RemoveTreeFromCommit(string treeSha, string commitSha) + /// + /// Removes a commit without cascading tree removal to other commits. + /// Used during LRU eviction: the trees are still missing, so other commits + /// that share those trees should continue to track them. + /// + private void RemoveCommitNoCache(string commitSha) { - if (this.missingTreesByCommit.TryGetValue(commitSha, out var trees)) + if (!this.missingTreesByCommit.TryGetValue(commitSha, out var trees)) { - trees.Remove(treeSha); + return; + } - // If no more trees for this commit, remove the commit entirely - if (trees.Count == 0) + foreach (string treeSha in trees) + { + if (this.commitsByTree.TryGetValue(treeSha, out var commits)) { - this.RemoveCommit(commitSha); + commits.Remove(commitSha); + if (commits.Count == 0) + { + this.commitsByTree.Remove(treeSha); + } } } - this.commitByTree.Remove(treeSha); + this.missingTreesByCommit.Remove(commitSha); + this.RemoveFromLruOrder(commitSha); + } + + /// + /// Removes a commit and cascades: trees that were in this commit's set are + /// also removed from all other commits that shared them. Any commit that + /// becomes empty as a result is also removed (without further cascade). + /// + private void RemoveCommitWithCascade(string commitSha) + { + if (!this.missingTreesByCommit.TryGetValue(commitSha, out var trees)) + { + return; + } + + // Collect commits that may become empty after we remove the shared trees. + // We don't cascade further than one level. + var commitsToCheck = new HashSet(); + + foreach (string treeSha in trees) + { + if (this.commitsByTree.TryGetValue(treeSha, out var sharingCommits)) + { + sharingCommits.Remove(commitSha); + + foreach (string otherCommit in sharingCommits) + { + if (this.missingTreesByCommit.TryGetValue(otherCommit, out var otherTrees)) + { + otherTrees.Remove(treeSha); + if (otherTrees.Count == 0) + { + commitsToCheck.Add(otherCommit); + } + } + } + + sharingCommits.Clear(); + this.commitsByTree.Remove(treeSha); + } + } + + this.missingTreesByCommit.Remove(commitSha); + this.RemoveFromLruOrder(commitSha); + + // Clean up any commits that became empty due to the cascade + foreach (string emptyCommit in commitsToCheck) + { + if (this.missingTreesByCommit.TryGetValue(emptyCommit, out var remaining) && remaining.Count == 0) + { + this.missingTreesByCommit.Remove(emptyCommit); + this.RemoveFromLruOrder(emptyCommit); + } + } + } + + private void RemoveFromLruOrder(string commitSha) + { + if (this.commitNodes.TryGetValue(commitSha, out var node)) + { + this.commitOrder.Remove(node); + this.commitNodes.Remove(commitSha); + } } } } diff --git a/GVFS/GVFS.Mount/InProcessMount.cs b/GVFS/GVFS.Mount/InProcessMount.cs index 87a579e44..ee6f123ef 100644 --- a/GVFS/GVFS.Mount/InProcessMount.cs +++ b/GVFS/GVFS.Mount/InProcessMount.cs @@ -33,9 +33,11 @@ public class InProcessMount // all the trees in a commit to ~2-3 seconds. private const int MissingTreeThresholdForDownloadingCommitPack = 200; - // Number of commits with missing trees to track with LRU eviction. This is to support batching tree - // downloads without using an unbounded amount of memory. - private const int TrackedCommitsCapacity = 20; + // Number of unique missing trees to track with LRU eviction. Eviction is commit-based: + // when capacity is reached, the LRU commit and all its unique trees are dropped to make room. + // Set to 20x the threshold so that enough trees can accumulate for the heuristic to + // reliably trigger a commit pack download. + private const int TrackedTreeCapacity = MissingTreeThresholdForDownloadingCommitPack * 20; private readonly bool showDebugWindow; @@ -56,7 +58,7 @@ public class InProcessMount private HeartbeatThread heartbeat; private ManualResetEvent unmountEvent; - private readonly MissingTreeTracker missingTreeTracker = new MissingTreeTracker(TrackedCommitsCapacity); + private readonly MissingTreeTracker missingTreeTracker = new MissingTreeTracker(TrackedTreeCapacity); // True if InProcessMount is calling git reset as part of processing // a folder dehydrate request @@ -607,7 +609,7 @@ private void HandleDownloadObjectRequest(NamedPipeMessages.Message message, Name * * Save the tree/commit so if more trees are requested we can download all the trees for the commit in a batch. */ - this.missingTreeTracker.AddMissingTree(treeSha, objectSha); + this.missingTreeTracker.AddMissingRootTree(treeSha, objectSha); } } } @@ -617,8 +619,9 @@ private void HandleDownloadObjectRequest(NamedPipeMessages.Message message, Name private bool ShouldDownloadCommitPack(string objectSha, out string commitSha) { - if (!this.missingTreeTracker.TryGetCommit(objectSha, out commitSha)) + if (!this.missingTreeTracker.TryGetCommits(objectSha, out string[] commitShas)) { + commitSha = null; return false; } @@ -627,7 +630,7 @@ private bool ShouldDownloadCommitPack(string objectSha, out string commitSha) * Conversely, if we know (from previously downloaded missing trees) that a commit has a lot of missing * trees left, we'll probably need to download many more trees for the commit so we should download the pack. */ - int missingTreeCount = this.missingTreeTracker.GetMissingTreeCount(commitSha); + int missingTreeCount = this.missingTreeTracker.GetHighestMissingTreeCount(commitShas, out commitSha); return missingTreeCount > MissingTreeThresholdForDownloadingCommitPack; } @@ -639,7 +642,7 @@ private void UpdateTreesForDownloadedCommits(string objectSha) * as a heuristic to decide whether to batch download all the trees for the commit the * next time a missing one is requested. */ - if (!this.missingTreeTracker.TryGetCommit(objectSha, out var commitSha)) + if (!this.missingTreeTracker.TryGetCommits(objectSha, out _)) { return; } @@ -652,16 +655,13 @@ private void UpdateTreesForDownloadedCommits(string objectSha) if (this.context.Repository.TryGetMissingSubTrees(objectSha, out var missingSubTrees)) { - foreach (var missingSubTree in missingSubTrees) - { - this.missingTreeTracker.AddMissingTree(missingSubTree, commitSha); - } + this.missingTreeTracker.AddMissingSubTrees(objectSha, missingSubTrees); } } private void DownloadedCommitPack(string commitSha) { - this.missingTreeTracker.RemoveCommit(commitSha); + this.missingTreeTracker.MarkCommitComplete(commitSha); } private void HandlePostFetchJobRequest(NamedPipeMessages.Message message, NamedPipeServer.Connection connection) diff --git a/GVFS/GVFS.UnitTests/Common/MissingTreeTrackerTests.cs b/GVFS/GVFS.UnitTests/Common/MissingTreeTrackerTests.cs index c65d102a2..944a214b3 100644 --- a/GVFS/GVFS.UnitTests/Common/MissingTreeTrackerTests.cs +++ b/GVFS/GVFS.UnitTests/Common/MissingTreeTrackerTests.cs @@ -7,307 +7,488 @@ namespace GVFS.UnitTests.Common [TestFixture] public class MissingTreeTrackerTests { + // ------------------------------------------------------------------------- + // AddMissingRootTree + // ------------------------------------------------------------------------- + + [TestCase] + public void AddMissingRootTree_SingleTreeAndCommit() + { + MissingTreeTracker tracker = new MissingTreeTracker(treeCapacity: 10); + + tracker.AddMissingRootTree("tree1", "commit1"); + + tracker.TryGetCommits("tree1", out string[] commits).ShouldEqual(true); + commits.Length.ShouldEqual(1); + commits[0].ShouldEqual("commit1"); + tracker.GetHighestMissingTreeCount(commits, out _).ShouldEqual(1); + } + + [TestCase] + public void AddMissingRootTree_MultipleTreesForSameCommit() + { + MissingTreeTracker tracker = new MissingTreeTracker(treeCapacity: 10); + + tracker.AddMissingRootTree("tree1", "commit1"); + tracker.AddMissingRootTree("tree2", "commit1"); + tracker.AddMissingRootTree("tree3", "commit1"); + + tracker.GetHighestMissingTreeCount(new[] { "commit1" }, out _).ShouldEqual(3); + + tracker.TryGetCommits("tree1", out string[] c1).ShouldEqual(true); + c1[0].ShouldEqual("commit1"); + + tracker.TryGetCommits("tree2", out string[] c2).ShouldEqual(true); + c2[0].ShouldEqual("commit1"); + + tracker.TryGetCommits("tree3", out string[] c3).ShouldEqual(true); + c3[0].ShouldEqual("commit1"); + } + + [TestCase] + public void AddMissingRootTree_SameTreeAddedTwiceToSameCommit() + { + MissingTreeTracker tracker = new MissingTreeTracker(treeCapacity: 10); + + tracker.AddMissingRootTree("tree1", "commit1"); + tracker.AddMissingRootTree("tree1", "commit1"); + + tracker.GetHighestMissingTreeCount(new[] { "commit1" }, out _).ShouldEqual(1); + } + + [TestCase] + public void AddMissingRootTree_SameTreeAddedToMultipleCommits() + { + MissingTreeTracker tracker = new MissingTreeTracker(treeCapacity: 10); + + tracker.AddMissingRootTree("tree1", "commit1"); + tracker.AddMissingRootTree("tree1", "commit2"); + + // tree1 is now tracked under both commits + tracker.TryGetCommits("tree1", out string[] commits).ShouldEqual(true); + commits.Length.ShouldEqual(2); + + // Both commits each have 1 tree + tracker.GetHighestMissingTreeCount(new[] { "commit1" }, out _).ShouldEqual(1); + tracker.GetHighestMissingTreeCount(new[] { "commit2" }, out _).ShouldEqual(1); + } + + [TestCase] + public void AddMissingRootTree_MultipleTrees_ChecksCount() + { + MissingTreeTracker tracker = new MissingTreeTracker(treeCapacity: 10); + + tracker.AddMissingRootTree("tree1", "commit1"); + tracker.GetHighestMissingTreeCount(new[] { "commit1" }, out _).ShouldEqual(1); + + tracker.AddMissingRootTree("tree2", "commit1"); + tracker.GetHighestMissingTreeCount(new[] { "commit1" }, out _).ShouldEqual(2); + + tracker.AddMissingRootTree("tree3", "commit1"); + tracker.GetHighestMissingTreeCount(new[] { "commit1" }, out _).ShouldEqual(3); + + tracker.AddMissingRootTree("tree4", "commit1"); + tracker.AddMissingRootTree("tree5", "commit1"); + tracker.GetHighestMissingTreeCount(new[] { "commit1" }, out _).ShouldEqual(5); + } + + // ------------------------------------------------------------------------- + // AddMissingSubTrees + // ------------------------------------------------------------------------- + [TestCase] - public void AddMissingTree_SingleTreeAndCommit() + public void AddMissingSubTrees_AddsSubTreesUnderParentsCommits() { - MissingTreeTracker tracker = new MissingTreeTracker(capacity: 10); - - tracker.AddMissingTree("tree1", "commit1"); - - tracker.TryGetCommit("tree1", out string commitSha).ShouldEqual(true); - commitSha.ShouldEqual("commit1"); - tracker.GetMissingTreeCount("commit1").ShouldEqual(1); + MissingTreeTracker tracker = new MissingTreeTracker(treeCapacity: 10); + + tracker.AddMissingRootTree("rootTree", "commit1"); + tracker.AddMissingSubTrees("rootTree", new[] { "sub1", "sub2" }); + + tracker.GetHighestMissingTreeCount(new[] { "commit1" }, out _).ShouldEqual(3); + + tracker.TryGetCommits("sub1", out string[] c1).ShouldEqual(true); + c1[0].ShouldEqual("commit1"); + + tracker.TryGetCommits("sub2", out string[] c2).ShouldEqual(true); + c2[0].ShouldEqual("commit1"); } [TestCase] - public void AddMissingTree_MultipleTreesForSameCommit() + public void AddMissingSubTrees_PropagatesAcrossAllSharingCommits() { - MissingTreeTracker tracker = new MissingTreeTracker(capacity: 10); - - tracker.AddMissingTree("tree1", "commit1"); - tracker.AddMissingTree("tree2", "commit1"); - tracker.AddMissingTree("tree3", "commit1"); - - tracker.GetMissingTreeCount("commit1").ShouldEqual(3); - - tracker.TryGetCommit("tree1", out string commit1).ShouldEqual(true); - commit1.ShouldEqual("commit1"); - - tracker.TryGetCommit("tree2", out string commit2).ShouldEqual(true); - commit2.ShouldEqual("commit1"); - - tracker.TryGetCommit("tree3", out string commit3).ShouldEqual(true); - commit3.ShouldEqual("commit1"); + MissingTreeTracker tracker = new MissingTreeTracker(treeCapacity: 10); + + // Two commits share the same root tree + tracker.AddMissingRootTree("rootTree", "commit1"); + tracker.AddMissingRootTree("rootTree", "commit2"); + + tracker.AddMissingSubTrees("rootTree", new[] { "sub1" }); + + // sub1 should be tracked under both commits + tracker.TryGetCommits("sub1", out string[] commits).ShouldEqual(true); + commits.Length.ShouldEqual(2); + + tracker.GetHighestMissingTreeCount(new[] { "commit1" }, out _).ShouldEqual(2); + tracker.GetHighestMissingTreeCount(new[] { "commit2" }, out _).ShouldEqual(2); } [TestCase] - public void AddMissingTree_SameTreeAddedTwiceToSameCommit() + public void AddMissingSubTrees_NoOp_WhenParentNotTracked() { - MissingTreeTracker tracker = new MissingTreeTracker(capacity: 10); - - tracker.AddMissingTree("tree1", "commit1"); - tracker.AddMissingTree("tree1", "commit1"); - - tracker.GetMissingTreeCount("commit1").ShouldEqual(1); + MissingTreeTracker tracker = new MissingTreeTracker(treeCapacity: 10); + + // Should not throw; parent is not tracked + tracker.AddMissingSubTrees("unknownParent", new[] { "sub1" }); + + tracker.TryGetCommits("sub1", out _).ShouldEqual(false); } [TestCase] - public void AddMissingTree_TreeReassociatedWithDifferentCommit() + public void AddMissingSubTrees_SkipsCommitEvictedDuringLoop() { - MissingTreeTracker tracker = new MissingTreeTracker(capacity: 10); - - // Add tree to first commit - tracker.AddMissingTree("tree1", "commit1"); - tracker.GetMissingTreeCount("commit1").ShouldEqual(1); - - // Reassociate same tree with different commit - tracker.AddMissingTree("tree1", "commit2"); - - // Tree should now be associated with commit2 - tracker.TryGetCommit("tree1", out string commitSha).ShouldEqual(true); - commitSha.ShouldEqual("commit2"); - - // commit1 should have 0 trees (and be removed) - tracker.GetMissingTreeCount("commit1").ShouldEqual(0); - - // commit2 should have 1 tree - tracker.GetMissingTreeCount("commit2").ShouldEqual(1); + // treeCapacity = 2: rootTree fills slot 1, rootTree2 fills slot 2. + // commit1 and commit2 both share rootTree (1 unique tree so far). + // commit3 holds rootTree2 (2 unique trees, at capacity). + // AddMissingSubTrees(rootTree, [sub1]) must add sub1 to commit1 then commit2. + // Adding sub1 for commit1 fills the 3rd slot, which evicts the LRU commit. + // commit2 is LRU (added to the tracker last among commit1/commit2 and then not used + // again, while commit1 just got used), so it is evicted before we process commit2. + // The loop must skip commit2 rather than crashing. + MissingTreeTracker tracker = new MissingTreeTracker(treeCapacity: 2); + + tracker.AddMissingRootTree("rootTree", "commit1"); + tracker.AddMissingRootTree("rootTree", "commit2"); + + // Does not throw, and sub1 ends up under whichever commit survived eviction + tracker.AddMissingSubTrees("rootTree", new[] { "sub1" }); + + // Exactly one of commit1/commit2 was evicted; sub1 exists under the survivor + bool commit1HasSub1 = tracker.GetHighestMissingTreeCount(new[] { "commit1" }, out _) == 2; + bool commit2HasSub1 = tracker.GetHighestMissingTreeCount(new[] { "commit2" }, out _) == 2; + (commit1HasSub1 || commit2HasSub1).ShouldEqual(true); + (commit1HasSub1 && commit2HasSub1).ShouldEqual(false); + } + + // ------------------------------------------------------------------------- + // TryGetCommits + // ------------------------------------------------------------------------- + + [TestCase] + public void TryGetCommits_NonExistentTree() + { + MissingTreeTracker tracker = new MissingTreeTracker(treeCapacity: 10); + + tracker.TryGetCommits("nonexistent", out string[] commits).ShouldEqual(false); + commits.ShouldBeNull(); + } + + [TestCase] + public void TryGetCommits_MarksAllCommitsAsRecentlyUsed() + { + MissingTreeTracker tracker = new MissingTreeTracker(treeCapacity: 3); + + tracker.AddMissingRootTree("sharedTree", "commit1"); + tracker.AddMissingRootTree("sharedTree", "commit2"); + tracker.AddMissingRootTree("tree3", "commit3"); + + // Access commit1 and commit2 via TryGetCommits + tracker.TryGetCommits("sharedTree", out _); + + // Adding a fourth commit should evict commit3 (oldest unused) + tracker.AddMissingRootTree("tree4", "commit4"); + + tracker.GetHighestMissingTreeCount(new[] { "commit1" }, out _).ShouldEqual(1); + tracker.GetHighestMissingTreeCount(new[] { "commit2" }, out _).ShouldEqual(1); + tracker.GetHighestMissingTreeCount(new[] { "commit3" }, out _).ShouldEqual(0); + tracker.GetHighestMissingTreeCount(new[] { "commit4" }, out _).ShouldEqual(1); } + // ------------------------------------------------------------------------- + // GetHighestMissingTreeCount + // ------------------------------------------------------------------------- + [TestCase] - public void AddMissingTree_TreeReassociatedWithDifferentCommit_OriginalCommitHasOtherTrees() + public void GetHighestMissingTreeCount_NonExistentCommit() { - MissingTreeTracker tracker = new MissingTreeTracker(capacity: 10); - - // Add multiple trees to first commit - tracker.AddMissingTree("tree1", "commit1"); - tracker.AddMissingTree("tree2", "commit1"); - tracker.GetMissingTreeCount("commit1").ShouldEqual(2); - - // Reassociate tree1 with different commit - tracker.AddMissingTree("tree1", "commit2"); - - // Tree1 should now be associated with commit2 - tracker.TryGetCommit("tree1", out string commitSha).ShouldEqual(true); - commitSha.ShouldEqual("commit2"); - - // commit1 should still exist with 1 tree - tracker.GetMissingTreeCount("commit1").ShouldEqual(1); - - // commit2 should have 1 tree - tracker.GetMissingTreeCount("commit2").ShouldEqual(1); - - // tree2 should still be associated with commit1 - tracker.TryGetCommit("tree2", out string tree2Commit).ShouldEqual(true); - tree2Commit.ShouldEqual("commit1"); + MissingTreeTracker tracker = new MissingTreeTracker(treeCapacity: 10); + + tracker.GetHighestMissingTreeCount(new[] { "nonexistent" }, out string highest).ShouldEqual(0); + highest.ShouldBeNull(); } [TestCase] - public void TryGetCommit_NonExistentTree() + public void GetHighestMissingTreeCount_ReturnsCommitWithMostTrees() { - MissingTreeTracker tracker = new MissingTreeTracker(capacity: 10); - - tracker.TryGetCommit("nonexistent", out string commitSha).ShouldEqual(false); - commitSha.ShouldEqual(null); + MissingTreeTracker tracker = new MissingTreeTracker(treeCapacity: 10); + + tracker.AddMissingRootTree("tree1", "commit1"); + tracker.AddMissingRootTree("tree2", "commit1"); + tracker.AddMissingRootTree("tree3", "commit2"); + + int count = tracker.GetHighestMissingTreeCount(new[] { "commit1", "commit2" }, out string highest); + count.ShouldEqual(2); + highest.ShouldEqual("commit1"); } [TestCase] - public void GetMissingTreeCount_NonExistentCommit() + public void GetHighestMissingTreeCount_DoesNotUpdateLru() { - MissingTreeTracker tracker = new MissingTreeTracker(capacity: 10); - - tracker.GetMissingTreeCount("nonexistent").ShouldEqual(0); + MissingTreeTracker tracker = new MissingTreeTracker(treeCapacity: 3); + + tracker.AddMissingRootTree("tree1", "commit1"); + tracker.AddMissingRootTree("tree2", "commit2"); + tracker.AddMissingRootTree("tree3", "commit3"); + + // Query commit1's count (should not update LRU) + tracker.GetHighestMissingTreeCount(new[] { "commit1" }, out _); + + // Adding a fourth commit should still evict commit1 (oldest) + tracker.AddMissingRootTree("tree4", "commit4"); + + tracker.GetHighestMissingTreeCount(new[] { "commit1" }, out _).ShouldEqual(0); + tracker.GetHighestMissingTreeCount(new[] { "commit2" }, out _).ShouldEqual(1); + tracker.GetHighestMissingTreeCount(new[] { "commit3" }, out _).ShouldEqual(1); + tracker.GetHighestMissingTreeCount(new[] { "commit4" }, out _).ShouldEqual(1); } + // ------------------------------------------------------------------------- + // MarkCommitComplete (cascade removal) + // ------------------------------------------------------------------------- + [TestCase] - public void RemoveCommit_RemovesAllTrees() + public void MarkCommitComplete_RemovesAllTreesForCommit() { - MissingTreeTracker tracker = new MissingTreeTracker(capacity: 10); - - tracker.AddMissingTree("tree1", "commit1"); - tracker.AddMissingTree("tree2", "commit1"); - tracker.AddMissingTree("tree3", "commit1"); - - tracker.GetMissingTreeCount("commit1").ShouldEqual(3); - - tracker.RemoveCommit("commit1"); - - tracker.GetMissingTreeCount("commit1").ShouldEqual(0); - tracker.TryGetCommit("tree1", out string _).ShouldEqual(false); - tracker.TryGetCommit("tree2", out string _).ShouldEqual(false); - tracker.TryGetCommit("tree3", out string _).ShouldEqual(false); + MissingTreeTracker tracker = new MissingTreeTracker(treeCapacity: 10); + + tracker.AddMissingRootTree("tree1", "commit1"); + tracker.AddMissingRootTree("tree2", "commit1"); + tracker.AddMissingRootTree("tree3", "commit1"); + + tracker.GetHighestMissingTreeCount(new[] { "commit1" }, out _).ShouldEqual(3); + + tracker.MarkCommitComplete("commit1"); + + tracker.GetHighestMissingTreeCount(new[] { "commit1" }, out _).ShouldEqual(0); + tracker.TryGetCommits("tree1", out _).ShouldEqual(false); + tracker.TryGetCommits("tree2", out _).ShouldEqual(false); + tracker.TryGetCommits("tree3", out _).ShouldEqual(false); } [TestCase] - public void RemoveCommit_NonExistentCommit() + public void MarkCommitComplete_NonExistentCommit() { - MissingTreeTracker tracker = new MissingTreeTracker(capacity: 10); - + MissingTreeTracker tracker = new MissingTreeTracker(treeCapacity: 10); + // Should not throw - tracker.RemoveCommit("nonexistent"); + tracker.MarkCommitComplete("nonexistent"); } [TestCase] - public void RemoveCommit_DoesNotAffectOtherCommits() + public void MarkCommitComplete_CascadesSharedTreesToOtherCommits() { - MissingTreeTracker tracker = new MissingTreeTracker(capacity: 10); - - tracker.AddMissingTree("tree1", "commit1"); - tracker.AddMissingTree("tree2", "commit2"); - - tracker.RemoveCommit("commit1"); - - tracker.GetMissingTreeCount("commit1").ShouldEqual(0); - tracker.GetMissingTreeCount("commit2").ShouldEqual(1); - tracker.TryGetCommit("tree2", out string commitSha).ShouldEqual(true); - commitSha.ShouldEqual("commit2"); + MissingTreeTracker tracker = new MissingTreeTracker(treeCapacity: 10); + + // commit1 and commit2 share tree1; commit2 also has tree2 + tracker.AddMissingRootTree("tree1", "commit1"); + tracker.AddMissingRootTree("tree1", "commit2"); + tracker.AddMissingRootTree("tree2", "commit2"); + + tracker.MarkCommitComplete("commit1"); + + // tree1 was in commit1, so it should be removed from commit2 as well + tracker.TryGetCommits("tree1", out _).ShouldEqual(false); + + // tree2 is unrelated to commit1, so commit2 still has it + tracker.GetHighestMissingTreeCount(new[] { "commit2" }, out _).ShouldEqual(1); + tracker.TryGetCommits("tree2", out string[] c2).ShouldEqual(true); + c2[0].ShouldEqual("commit2"); + } + + [TestCase] + public void MarkCommitComplete_RemovesOtherCommitWhenItBecomesEmpty() + { + MissingTreeTracker tracker = new MissingTreeTracker(treeCapacity: 10); + + // commit2's only tree is shared with commit1 + tracker.AddMissingRootTree("tree1", "commit1"); + tracker.AddMissingRootTree("tree1", "commit2"); + + tracker.MarkCommitComplete("commit1"); + + // commit2 had only tree1, which was cascaded away, so commit2 should be gone too + tracker.GetHighestMissingTreeCount(new[] { "commit2" }, out _).ShouldEqual(0); + tracker.TryGetCommits("tree1", out _).ShouldEqual(false); + } + + [TestCase] + public void MarkCommitComplete_DoesNotAffectUnrelatedCommits() + { + MissingTreeTracker tracker = new MissingTreeTracker(treeCapacity: 10); + + tracker.AddMissingRootTree("tree1", "commit1"); + tracker.AddMissingRootTree("tree2", "commit2"); + + tracker.MarkCommitComplete("commit1"); + + tracker.GetHighestMissingTreeCount(new[] { "commit1" }, out _).ShouldEqual(0); + tracker.GetHighestMissingTreeCount(new[] { "commit2" }, out _).ShouldEqual(1); + tracker.TryGetCommits("tree2", out string[] c).ShouldEqual(true); + c[0].ShouldEqual("commit2"); } + // ------------------------------------------------------------------------- + // LRU eviction (no cascade) + // ------------------------------------------------------------------------- + [TestCase] public void LruEviction_EvictsOldestCommit() { - MissingTreeTracker tracker = new MissingTreeTracker(capacity: 3); - - tracker.AddMissingTree("tree1", "commit1"); - tracker.AddMissingTree("tree2", "commit2"); - tracker.AddMissingTree("tree3", "commit3"); - - // All three commits should exist - tracker.GetMissingTreeCount("commit1").ShouldEqual(1); - tracker.GetMissingTreeCount("commit2").ShouldEqual(1); - tracker.GetMissingTreeCount("commit3").ShouldEqual(1); - - // Adding a fourth commit should evict commit1 (oldest) - tracker.AddMissingTree("tree4", "commit4"); - - tracker.GetMissingTreeCount("commit1").ShouldEqual(0); - tracker.TryGetCommit("tree1", out string _).ShouldEqual(false); - - tracker.GetMissingTreeCount("commit2").ShouldEqual(1); - tracker.GetMissingTreeCount("commit3").ShouldEqual(1); - tracker.GetMissingTreeCount("commit4").ShouldEqual(1); + // treeCapacity = 3 trees; one tree per commit + MissingTreeTracker tracker = new MissingTreeTracker(treeCapacity: 3); + + tracker.AddMissingRootTree("tree1", "commit1"); + tracker.AddMissingRootTree("tree2", "commit2"); + tracker.AddMissingRootTree("tree3", "commit3"); + + // Adding a fourth tree exceeds treeCapacity, so commit1 (LRU) is evicted + tracker.AddMissingRootTree("tree4", "commit4"); + + tracker.GetHighestMissingTreeCount(new[] { "commit1" }, out _).ShouldEqual(0); + tracker.TryGetCommits("tree1", out _).ShouldEqual(false); + + tracker.GetHighestMissingTreeCount(new[] { "commit2" }, out _).ShouldEqual(1); + tracker.GetHighestMissingTreeCount(new[] { "commit3" }, out _).ShouldEqual(1); + tracker.GetHighestMissingTreeCount(new[] { "commit4" }, out _).ShouldEqual(1); } [TestCase] - public void LruEviction_AddingTreeToExistingCommitUpdatesLru() + public void LruEviction_DoesNotCascadeSharedTreesToOtherCommits() { - MissingTreeTracker tracker = new MissingTreeTracker(capacity: 3); - - tracker.AddMissingTree("tree1", "commit1"); - tracker.AddMissingTree("tree2", "commit2"); - tracker.AddMissingTree("tree3", "commit3"); - - // Access commit1 by adding another tree to it (marks it as recently used) - tracker.AddMissingTree("tree1b", "commit1"); - - // Adding a fourth commit should evict commit2 (now oldest) - tracker.AddMissingTree("tree4", "commit4"); - - tracker.GetMissingTreeCount("commit1").ShouldEqual(2); // Still has tree1 and tree1b - tracker.GetMissingTreeCount("commit2").ShouldEqual(0); // Evicted - tracker.GetMissingTreeCount("commit3").ShouldEqual(1); - tracker.GetMissingTreeCount("commit4").ShouldEqual(1); + // treeCapacity = 3 trees; tree1 is shared so only 2 unique trees + tree3 = 3 total + MissingTreeTracker tracker = new MissingTreeTracker(treeCapacity: 3); + + // tree1 is shared between commit1 and commit2 (counts as 1 unique tree) + tracker.AddMissingRootTree("tree1", "commit1"); + tracker.AddMissingRootTree("tree1", "commit2"); + tracker.AddMissingRootTree("tree3", "commit3"); + + // tree4 is the 4th unique tree, exceeding treeCapacity; evicts commit1 (LRU) + tracker.AddMissingRootTree("tree4", "commit4"); + + tracker.GetHighestMissingTreeCount(new[] { "commit1" }, out _).ShouldEqual(0); + + // tree1 is still missing (not yet downloaded), so commit2 retains it + tracker.TryGetCommits("tree1", out string[] commits).ShouldEqual(true); + commits.Length.ShouldEqual(1); + commits[0].ShouldEqual("commit2"); + tracker.GetHighestMissingTreeCount(new[] { "commit2" }, out _).ShouldEqual(1); } [TestCase] - public void LruEviction_TryGetCommitUpdatesLru() + public void LruEviction_AddingTreeToExistingCommitUpdatesLru() { - MissingTreeTracker tracker = new MissingTreeTracker(capacity: 3); - - tracker.AddMissingTree("tree1", "commit1"); - tracker.AddMissingTree("tree2", "commit2"); - tracker.AddMissingTree("tree3", "commit3"); - - // Access commit1 via TryGetCommit (marks it as recently used) - tracker.TryGetCommit("tree1", out string _); - - // Adding a fourth commit should evict commit2 (now oldest) - tracker.AddMissingTree("tree4", "commit4"); - - tracker.GetMissingTreeCount("commit1").ShouldEqual(1); // Still exists - tracker.GetMissingTreeCount("commit2").ShouldEqual(0); // Evicted - tracker.GetMissingTreeCount("commit3").ShouldEqual(1); - tracker.GetMissingTreeCount("commit4").ShouldEqual(1); + // treeCapacity = 4 trees; tree1, tree2, tree3 fill it, then tree1b re-uses commit1 + MissingTreeTracker tracker = new MissingTreeTracker(treeCapacity: 4); + + tracker.AddMissingRootTree("tree1", "commit1"); + tracker.AddMissingRootTree("tree2", "commit2"); + tracker.AddMissingRootTree("tree3", "commit3"); + + // Adding tree1b to commit1 marks commit1 as recently used (it's a new unique tree) + tracker.AddMissingRootTree("tree1b", "commit1"); + + // tree4 is the 5th unique tree, exceeding treeCapacity; commit2 is now LRU + tracker.AddMissingRootTree("tree4", "commit4"); + + tracker.GetHighestMissingTreeCount(new[] { "commit1" }, out _).ShouldEqual(2); + tracker.GetHighestMissingTreeCount(new[] { "commit2" }, out _).ShouldEqual(0); + tracker.GetHighestMissingTreeCount(new[] { "commit3" }, out _).ShouldEqual(1); + tracker.GetHighestMissingTreeCount(new[] { "commit4" }, out _).ShouldEqual(1); } [TestCase] - public void LruEviction_EvictsMultipleTrees() + public void LruEviction_MultipleTreesPerCommit_EvictsEntireCommit() { - MissingTreeTracker tracker = new MissingTreeTracker(capacity: 2); - - tracker.AddMissingTree("tree1", "commit1"); - tracker.AddMissingTree("tree2", "commit1"); - tracker.AddMissingTree("tree3", "commit1"); - - tracker.AddMissingTree("tree4", "commit2"); - - tracker.GetMissingTreeCount("commit1").ShouldEqual(3); - tracker.GetMissingTreeCount("commit2").ShouldEqual(1); - - // Adding a third commit should evict commit1 with all its trees - tracker.AddMissingTree("tree5", "commit3"); - - tracker.GetMissingTreeCount("commit1").ShouldEqual(0); - tracker.TryGetCommit("tree1", out string _).ShouldEqual(false); - tracker.TryGetCommit("tree2", out string _).ShouldEqual(false); - tracker.TryGetCommit("tree3", out string _).ShouldEqual(false); - - tracker.GetMissingTreeCount("commit2").ShouldEqual(1); - tracker.GetMissingTreeCount("commit3").ShouldEqual(1); + // treeCapacity = 4 trees; commit1 holds 3, commit2 holds 1 + MissingTreeTracker tracker = new MissingTreeTracker(treeCapacity: 4); + + tracker.AddMissingRootTree("tree1", "commit1"); + tracker.AddMissingRootTree("tree2", "commit1"); + tracker.AddMissingRootTree("tree3", "commit1"); + tracker.AddMissingRootTree("tree4", "commit2"); + + tracker.GetHighestMissingTreeCount(new[] { "commit1" }, out _).ShouldEqual(3); + tracker.GetHighestMissingTreeCount(new[] { "commit2" }, out _).ShouldEqual(1); + + // tree5 is the 5th unique tree; evict LRU (commit1) freeing 3 slots, then add tree5 + tracker.AddMissingRootTree("tree5", "commit3"); + + tracker.GetHighestMissingTreeCount(new[] { "commit1" }, out _).ShouldEqual(0); + tracker.TryGetCommits("tree1", out _).ShouldEqual(false); + tracker.TryGetCommits("tree2", out _).ShouldEqual(false); + tracker.TryGetCommits("tree3", out _).ShouldEqual(false); + + tracker.GetHighestMissingTreeCount(new[] { "commit2" }, out _).ShouldEqual(1); + tracker.GetHighestMissingTreeCount(new[] { "commit3" }, out _).ShouldEqual(1); } [TestCase] - public void Capacity_One() + public void LruEviction_CapacityOne() { - MissingTreeTracker tracker = new MissingTreeTracker(capacity: 1); - - tracker.AddMissingTree("tree1", "commit1"); - tracker.GetMissingTreeCount("commit1").ShouldEqual(1); - - tracker.AddMissingTree("tree2", "commit2"); - - tracker.GetMissingTreeCount("commit1").ShouldEqual(0); - tracker.GetMissingTreeCount("commit2").ShouldEqual(1); + MissingTreeTracker tracker = new MissingTreeTracker(treeCapacity: 1); + + tracker.AddMissingRootTree("tree1", "commit1"); + tracker.GetHighestMissingTreeCount(new[] { "commit1" }, out _).ShouldEqual(1); + + tracker.AddMissingRootTree("tree2", "commit2"); + + tracker.GetHighestMissingTreeCount(new[] { "commit1" }, out _).ShouldEqual(0); + tracker.GetHighestMissingTreeCount(new[] { "commit2" }, out _).ShouldEqual(1); } [TestCase] - public void AddMissingTree_MultipleTrees_ChecksCount() + public void LruEviction_ManyTreesOneCommit_ExceedsCapacity() { - MissingTreeTracker tracker = new MissingTreeTracker(capacity: 10); - - tracker.AddMissingTree("tree1", "commit1"); - tracker.GetMissingTreeCount("commit1").ShouldEqual(1); - - tracker.AddMissingTree("tree2", "commit1"); - tracker.GetMissingTreeCount("commit1").ShouldEqual(2); - - tracker.AddMissingTree("tree3", "commit1"); - tracker.GetMissingTreeCount("commit1").ShouldEqual(3); - - tracker.AddMissingTree("tree4", "commit1"); - tracker.AddMissingTree("tree5", "commit1"); - tracker.GetMissingTreeCount("commit1").ShouldEqual(5); + // treeCapacity = 3 trees; all trees belong to commit1 + // Adding a 4th tree must evict commit1 (the only commit) to make room + MissingTreeTracker tracker = new MissingTreeTracker(treeCapacity: 3); + + tracker.AddMissingRootTree("tree1", "commit1"); + tracker.AddMissingRootTree("tree2", "commit1"); + tracker.AddMissingRootTree("tree3", "commit1"); + + // tree4 exceeds the tree treeCapacity; the LRU commit (commit1) is evicted + // and then commit2 with tree4 is added fresh + tracker.AddMissingRootTree("tree4", "commit2"); + + tracker.GetHighestMissingTreeCount(new[] { "commit1" }, out _).ShouldEqual(0); + tracker.TryGetCommits("tree1", out _).ShouldEqual(false); + tracker.TryGetCommits("tree2", out _).ShouldEqual(false); + tracker.TryGetCommits("tree3", out _).ShouldEqual(false); + + tracker.GetHighestMissingTreeCount(new[] { "commit2" }, out _).ShouldEqual(1); } [TestCase] - public void GetMissingTreeCount_DoesNotUpdateLru() + public void LruEviction_TryGetCommitsUpdatesLru() { - MissingTreeTracker tracker = new MissingTreeTracker(capacity: 3); - - tracker.AddMissingTree("tree1", "commit1"); - tracker.AddMissingTree("tree2", "commit2"); - tracker.AddMissingTree("tree3", "commit3"); - - // Query commit1's count (should not update LRU) - tracker.GetMissingTreeCount("commit1"); - - // Adding a fourth commit should still evict commit1 (oldest) - tracker.AddMissingTree("tree4", "commit4"); - - tracker.GetMissingTreeCount("commit1").ShouldEqual(0); - tracker.GetMissingTreeCount("commit2").ShouldEqual(1); - tracker.GetMissingTreeCount("commit3").ShouldEqual(1); - tracker.GetMissingTreeCount("commit4").ShouldEqual(1); + // treeCapacity = 3 trees, one per commit + MissingTreeTracker tracker = new MissingTreeTracker(treeCapacity: 3); + + tracker.AddMissingRootTree("tree1", "commit1"); + tracker.AddMissingRootTree("tree2", "commit2"); + tracker.AddMissingRootTree("tree3", "commit3"); + + // Access commit1 via TryGetCommits (marks it as recently used) + tracker.TryGetCommits("tree1", out _); + + // tree4 exceeds treeCapacity; commit2 is now LRU + tracker.AddMissingRootTree("tree4", "commit4"); + + tracker.GetHighestMissingTreeCount(new[] { "commit1" }, out _).ShouldEqual(1); + tracker.GetHighestMissingTreeCount(new[] { "commit2" }, out _).ShouldEqual(0); + tracker.GetHighestMissingTreeCount(new[] { "commit3" }, out _).ShouldEqual(1); + tracker.GetHighestMissingTreeCount(new[] { "commit4" }, out _).ShouldEqual(1); } } } From 2b7fdcb0bac1752ee00e89d3b58eecf4d4a52f6e Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Thu, 26 Feb 2026 10:41:30 -0800 Subject: [PATCH 4/5] Fix tests --- GVFS/GVFS.Common/MissingTreeTracker.cs | 16 +++++++++---- GVFS/GVFS.Mount/InProcessMount.cs | 2 +- .../Common/MissingTreeTrackerTests.cs | 23 ++++++++++++++++--- 3 files changed, 33 insertions(+), 8 deletions(-) diff --git a/GVFS/GVFS.Common/MissingTreeTracker.cs b/GVFS/GVFS.Common/MissingTreeTracker.cs index 7dd0bf526..0781c1774 100644 --- a/GVFS/GVFS.Common/MissingTreeTracker.cs +++ b/GVFS/GVFS.Common/MissingTreeTracker.cs @@ -70,7 +70,8 @@ public void AddMissingSubTrees(string parentTreeSha, string[] subTreeShas) { continue; } - + /* Ensure we don't evict this commit while trying to add a tree to it. */ + this.MarkCommitAsUsed(commitSha); this.AddTreeToCommit(subTreeSha, commitSha); } } @@ -160,7 +161,11 @@ private void AddTreeToCommit(string treeSha, string commitSha) // Evict LRU commits until there is room for the new tree while (this.commitsByTree.Count >= this.treeCapacity) { - this.EvictLruCommit(); + // If evict fails it means we only have one commit left. + if (!this.EvictLruCommit()) + { + break; + } } this.commitsByTree[treeSha] = new HashSet(StringComparer.OrdinalIgnoreCase); @@ -180,13 +185,16 @@ private void MarkCommitAsUsed(string commitSha) } } - private void EvictLruCommit() + private bool EvictLruCommit() { - if (this.commitOrder.Last != null) + var last = this.commitOrder.Last; + if (last != null && last.Value != this.commitOrder.First.Value) { string lruCommit = this.commitOrder.Last.Value; this.RemoveCommitNoCache(lruCommit); + return true; } + return false; } /// diff --git a/GVFS/GVFS.Mount/InProcessMount.cs b/GVFS/GVFS.Mount/InProcessMount.cs index ee6f123ef..43a40b228 100644 --- a/GVFS/GVFS.Mount/InProcessMount.cs +++ b/GVFS/GVFS.Mount/InProcessMount.cs @@ -609,7 +609,7 @@ private void HandleDownloadObjectRequest(NamedPipeMessages.Message message, Name * * Save the tree/commit so if more trees are requested we can download all the trees for the commit in a batch. */ - this.missingTreeTracker.AddMissingRootTree(treeSha, objectSha); + this.missingTreeTracker.AddMissingRootTree(treeSha: treeSha, commitSha: objectSha); } } } diff --git a/GVFS/GVFS.UnitTests/Common/MissingTreeTrackerTests.cs b/GVFS/GVFS.UnitTests/Common/MissingTreeTrackerTests.cs index 944a214b3..a7c3e4b6b 100644 --- a/GVFS/GVFS.UnitTests/Common/MissingTreeTrackerTests.cs +++ b/GVFS/GVFS.UnitTests/Common/MissingTreeTrackerTests.cs @@ -158,6 +158,7 @@ public void AddMissingSubTrees_SkipsCommitEvictedDuringLoop() tracker.AddMissingRootTree("rootTree", "commit1"); tracker.AddMissingRootTree("rootTree", "commit2"); + tracker.AddMissingRootTree("rootTree2", "commit3"); // Does not throw, and sub1 ends up under whichever commit survived eviction tracker.AddMissingSubTrees("rootTree", new[] { "sub1" }); @@ -169,6 +170,18 @@ public void AddMissingSubTrees_SkipsCommitEvictedDuringLoop() (commit1HasSub1 && commit2HasSub1).ShouldEqual(false); } + [TestCase] + public void AddMissingSubTrees_DoesNotEvictIfOnlyOneCommit() + { + /* This shouldn't be possible if user has a proper threshold and is marking commits + * as completed, but test to be safe. */ + MissingTreeTracker tracker = new MissingTreeTracker(treeCapacity: 2); + tracker.AddMissingRootTree("rootTree", "commit1"); + tracker.AddMissingSubTrees("rootTree", new[] { "sub1" }); + tracker.AddMissingSubTrees("rootTree", new[] { "sub2" }); + tracker.GetHighestMissingTreeCount(new[] { "commit1" }, out _).ShouldEqual(3); + } + // ------------------------------------------------------------------------- // TryGetCommits // ------------------------------------------------------------------------- @@ -189,18 +202,20 @@ public void TryGetCommits_MarksAllCommitsAsRecentlyUsed() tracker.AddMissingRootTree("sharedTree", "commit1"); tracker.AddMissingRootTree("sharedTree", "commit2"); - tracker.AddMissingRootTree("tree3", "commit3"); + tracker.AddMissingRootTree("tree2", "commit3"); + tracker.AddMissingRootTree("tree3", "commit4"); // Access commit1 and commit2 via TryGetCommits tracker.TryGetCommits("sharedTree", out _); - // Adding a fourth commit should evict commit3 (oldest unused) - tracker.AddMissingRootTree("tree4", "commit4"); + // Adding a fourth tree should evict commit3 (oldest unused) + tracker.AddMissingRootTree("tree4", "commit5"); tracker.GetHighestMissingTreeCount(new[] { "commit1" }, out _).ShouldEqual(1); tracker.GetHighestMissingTreeCount(new[] { "commit2" }, out _).ShouldEqual(1); tracker.GetHighestMissingTreeCount(new[] { "commit3" }, out _).ShouldEqual(0); tracker.GetHighestMissingTreeCount(new[] { "commit4" }, out _).ShouldEqual(1); + tracker.GetHighestMissingTreeCount(new[] { "commit5" }, out _).ShouldEqual(1); } // ------------------------------------------------------------------------- @@ -369,10 +384,12 @@ public void LruEviction_DoesNotCascadeSharedTreesToOtherCommits() // tree1 is shared between commit1 and commit2 (counts as 1 unique tree) tracker.AddMissingRootTree("tree1", "commit1"); + tracker.AddMissingRootTree("tree2", "commit1"); tracker.AddMissingRootTree("tree1", "commit2"); tracker.AddMissingRootTree("tree3", "commit3"); // tree4 is the 4th unique tree, exceeding treeCapacity; evicts commit1 (LRU) + // which removes tree2, freeing up capacity. tracker.AddMissingRootTree("tree4", "commit4"); tracker.GetHighestMissingTreeCount(new[] { "commit1" }, out _).ShouldEqual(0); From a90175ff5f16b52796326101e163aef77f98254a Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Thu, 26 Feb 2026 10:51:53 -0800 Subject: [PATCH 5/5] Add telemetry --- GVFS/GVFS.Common/MissingTreeTracker.cs | 32 +++++++++- GVFS/GVFS.Mount/InProcessMount.cs | 3 +- .../Common/MissingTreeTrackerTests.cs | 60 ++++++++++--------- 3 files changed, 65 insertions(+), 30 deletions(-) diff --git a/GVFS/GVFS.Common/MissingTreeTracker.cs b/GVFS/GVFS.Common/MissingTreeTracker.cs index 0781c1774..3d5ca78a1 100644 --- a/GVFS/GVFS.Common/MissingTreeTracker.cs +++ b/GVFS/GVFS.Common/MissingTreeTracker.cs @@ -1,6 +1,7 @@ using System; using System.Collections.Generic; using System.Linq; +using GVFS.Common.Tracing; namespace GVFS.Common { @@ -11,7 +12,10 @@ namespace GVFS.Common /// public class MissingTreeTracker { + private const string EtwArea = nameof(MissingTreeTracker); + private readonly int treeCapacity; + private readonly ITracer tracer; private readonly object syncLock = new object(); // Primary storage: commit -> set of missing trees @@ -24,8 +28,9 @@ public class MissingTreeTracker private readonly LinkedList commitOrder; private readonly Dictionary> commitNodes; - public MissingTreeTracker(int treeCapacity) + public MissingTreeTracker(ITracer tracer, int treeCapacity) { + this.tracer = tracer; this.treeCapacity = treeCapacity; this.missingTreesByCommit = new Dictionary>(StringComparer.OrdinalIgnoreCase); this.commitsByTree = new Dictionary>(StringComparer.OrdinalIgnoreCase); @@ -137,6 +142,13 @@ public void MarkCommitComplete(string commitSha) lock (this.syncLock) { this.RemoveCommitWithCascade(commitSha); + + EventMetadata metadata = new EventMetadata(); + metadata.Add("Area", EtwArea); + metadata.Add("CompletedCommit", commitSha); + metadata.Add("RemainingCommits", this.commitNodes.Count); + metadata.Add("RemainingTrees", this.commitsByTree.Count); + this.tracer.RelatedEvent(EventLevel.Informational, nameof(this.MarkCommitComplete), metadata, Keywords.Telemetry); } } @@ -190,10 +202,26 @@ private bool EvictLruCommit() var last = this.commitOrder.Last; if (last != null && last.Value != this.commitOrder.First.Value) { - string lruCommit = this.commitOrder.Last.Value; + string lruCommit = last.Value; + var treeCountBefore = this.commitsByTree.Count; this.RemoveCommitNoCache(lruCommit); + + EventMetadata metadata = new EventMetadata(); + metadata.Add("Area", EtwArea); + metadata.Add("EvictedCommit", lruCommit); + metadata.Add("TreeCountBefore", treeCountBefore); + metadata.Add("TreeCountAfter", this.commitsByTree.Count); + this.tracer.RelatedEvent(EventLevel.Informational, nameof(this.EvictLruCommit), metadata, Keywords.Telemetry); + return true; } + + EventMetadata warnMetadata = new EventMetadata(); + warnMetadata.Add("Area", EtwArea); + warnMetadata.Add("TreeCount", this.commitsByTree.Count); + warnMetadata.Add("CommitCount", this.commitNodes.Count); + this.tracer.RelatedEvent(EventLevel.Warning, $"{nameof(this.EvictLruCommit)}CouldNotEvict", warnMetadata, Keywords.Telemetry); + return false; } diff --git a/GVFS/GVFS.Mount/InProcessMount.cs b/GVFS/GVFS.Mount/InProcessMount.cs index 43a40b228..c5126bac0 100644 --- a/GVFS/GVFS.Mount/InProcessMount.cs +++ b/GVFS/GVFS.Mount/InProcessMount.cs @@ -58,7 +58,7 @@ public class InProcessMount private HeartbeatThread heartbeat; private ManualResetEvent unmountEvent; - private readonly MissingTreeTracker missingTreeTracker = new MissingTreeTracker(TrackedTreeCapacity); + private readonly MissingTreeTracker missingTreeTracker; // True if InProcessMount is calling git reset as part of processing // a folder dehydrate request @@ -73,6 +73,7 @@ public InProcessMount(ITracer tracer, GVFSEnlistment enlistment, CacheServerInfo this.enlistment = enlistment; this.showDebugWindow = showDebugWindow; this.unmountEvent = new ManualResetEvent(false); + this.missingTreeTracker = new MissingTreeTracker(tracer, TrackedTreeCapacity); } private enum MountState diff --git a/GVFS/GVFS.UnitTests/Common/MissingTreeTrackerTests.cs b/GVFS/GVFS.UnitTests/Common/MissingTreeTrackerTests.cs index a7c3e4b6b..a13a34554 100644 --- a/GVFS/GVFS.UnitTests/Common/MissingTreeTrackerTests.cs +++ b/GVFS/GVFS.UnitTests/Common/MissingTreeTrackerTests.cs @@ -1,5 +1,6 @@ using GVFS.Common; using GVFS.Tests.Should; +using GVFS.UnitTests.Mock.Common; using NUnit.Framework; namespace GVFS.UnitTests.Common @@ -7,6 +8,11 @@ namespace GVFS.UnitTests.Common [TestFixture] public class MissingTreeTrackerTests { + private static MissingTreeTracker CreateTracker(int treeCapacity) + { + return new MissingTreeTracker(new MockTracer(), treeCapacity); + } + // ------------------------------------------------------------------------- // AddMissingRootTree // ------------------------------------------------------------------------- @@ -14,7 +20,7 @@ public class MissingTreeTrackerTests [TestCase] public void AddMissingRootTree_SingleTreeAndCommit() { - MissingTreeTracker tracker = new MissingTreeTracker(treeCapacity: 10); + MissingTreeTracker tracker = CreateTracker(treeCapacity: 10); tracker.AddMissingRootTree("tree1", "commit1"); @@ -27,7 +33,7 @@ public void AddMissingRootTree_SingleTreeAndCommit() [TestCase] public void AddMissingRootTree_MultipleTreesForSameCommit() { - MissingTreeTracker tracker = new MissingTreeTracker(treeCapacity: 10); + MissingTreeTracker tracker = CreateTracker(treeCapacity: 10); tracker.AddMissingRootTree("tree1", "commit1"); tracker.AddMissingRootTree("tree2", "commit1"); @@ -48,7 +54,7 @@ public void AddMissingRootTree_MultipleTreesForSameCommit() [TestCase] public void AddMissingRootTree_SameTreeAddedTwiceToSameCommit() { - MissingTreeTracker tracker = new MissingTreeTracker(treeCapacity: 10); + MissingTreeTracker tracker = CreateTracker(treeCapacity: 10); tracker.AddMissingRootTree("tree1", "commit1"); tracker.AddMissingRootTree("tree1", "commit1"); @@ -59,7 +65,7 @@ public void AddMissingRootTree_SameTreeAddedTwiceToSameCommit() [TestCase] public void AddMissingRootTree_SameTreeAddedToMultipleCommits() { - MissingTreeTracker tracker = new MissingTreeTracker(treeCapacity: 10); + MissingTreeTracker tracker = CreateTracker(treeCapacity: 10); tracker.AddMissingRootTree("tree1", "commit1"); tracker.AddMissingRootTree("tree1", "commit2"); @@ -76,7 +82,7 @@ public void AddMissingRootTree_SameTreeAddedToMultipleCommits() [TestCase] public void AddMissingRootTree_MultipleTrees_ChecksCount() { - MissingTreeTracker tracker = new MissingTreeTracker(treeCapacity: 10); + MissingTreeTracker tracker = CreateTracker(treeCapacity: 10); tracker.AddMissingRootTree("tree1", "commit1"); tracker.GetHighestMissingTreeCount(new[] { "commit1" }, out _).ShouldEqual(1); @@ -99,7 +105,7 @@ public void AddMissingRootTree_MultipleTrees_ChecksCount() [TestCase] public void AddMissingSubTrees_AddsSubTreesUnderParentsCommits() { - MissingTreeTracker tracker = new MissingTreeTracker(treeCapacity: 10); + MissingTreeTracker tracker = CreateTracker(treeCapacity: 10); tracker.AddMissingRootTree("rootTree", "commit1"); tracker.AddMissingSubTrees("rootTree", new[] { "sub1", "sub2" }); @@ -116,7 +122,7 @@ public void AddMissingSubTrees_AddsSubTreesUnderParentsCommits() [TestCase] public void AddMissingSubTrees_PropagatesAcrossAllSharingCommits() { - MissingTreeTracker tracker = new MissingTreeTracker(treeCapacity: 10); + MissingTreeTracker tracker = CreateTracker(treeCapacity: 10); // Two commits share the same root tree tracker.AddMissingRootTree("rootTree", "commit1"); @@ -135,7 +141,7 @@ public void AddMissingSubTrees_PropagatesAcrossAllSharingCommits() [TestCase] public void AddMissingSubTrees_NoOp_WhenParentNotTracked() { - MissingTreeTracker tracker = new MissingTreeTracker(treeCapacity: 10); + MissingTreeTracker tracker = CreateTracker(treeCapacity: 10); // Should not throw; parent is not tracked tracker.AddMissingSubTrees("unknownParent", new[] { "sub1" }); @@ -154,7 +160,7 @@ public void AddMissingSubTrees_SkipsCommitEvictedDuringLoop() // commit2 is LRU (added to the tracker last among commit1/commit2 and then not used // again, while commit1 just got used), so it is evicted before we process commit2. // The loop must skip commit2 rather than crashing. - MissingTreeTracker tracker = new MissingTreeTracker(treeCapacity: 2); + MissingTreeTracker tracker = CreateTracker(treeCapacity: 2); tracker.AddMissingRootTree("rootTree", "commit1"); tracker.AddMissingRootTree("rootTree", "commit2"); @@ -175,7 +181,7 @@ public void AddMissingSubTrees_DoesNotEvictIfOnlyOneCommit() { /* This shouldn't be possible if user has a proper threshold and is marking commits * as completed, but test to be safe. */ - MissingTreeTracker tracker = new MissingTreeTracker(treeCapacity: 2); + MissingTreeTracker tracker = CreateTracker(treeCapacity: 2); tracker.AddMissingRootTree("rootTree", "commit1"); tracker.AddMissingSubTrees("rootTree", new[] { "sub1" }); tracker.AddMissingSubTrees("rootTree", new[] { "sub2" }); @@ -189,7 +195,7 @@ public void AddMissingSubTrees_DoesNotEvictIfOnlyOneCommit() [TestCase] public void TryGetCommits_NonExistentTree() { - MissingTreeTracker tracker = new MissingTreeTracker(treeCapacity: 10); + MissingTreeTracker tracker = CreateTracker(treeCapacity: 10); tracker.TryGetCommits("nonexistent", out string[] commits).ShouldEqual(false); commits.ShouldBeNull(); @@ -198,7 +204,7 @@ public void TryGetCommits_NonExistentTree() [TestCase] public void TryGetCommits_MarksAllCommitsAsRecentlyUsed() { - MissingTreeTracker tracker = new MissingTreeTracker(treeCapacity: 3); + MissingTreeTracker tracker = CreateTracker(treeCapacity: 3); tracker.AddMissingRootTree("sharedTree", "commit1"); tracker.AddMissingRootTree("sharedTree", "commit2"); @@ -225,7 +231,7 @@ public void TryGetCommits_MarksAllCommitsAsRecentlyUsed() [TestCase] public void GetHighestMissingTreeCount_NonExistentCommit() { - MissingTreeTracker tracker = new MissingTreeTracker(treeCapacity: 10); + MissingTreeTracker tracker = CreateTracker(treeCapacity: 10); tracker.GetHighestMissingTreeCount(new[] { "nonexistent" }, out string highest).ShouldEqual(0); highest.ShouldBeNull(); @@ -234,7 +240,7 @@ public void GetHighestMissingTreeCount_NonExistentCommit() [TestCase] public void GetHighestMissingTreeCount_ReturnsCommitWithMostTrees() { - MissingTreeTracker tracker = new MissingTreeTracker(treeCapacity: 10); + MissingTreeTracker tracker = CreateTracker(treeCapacity: 10); tracker.AddMissingRootTree("tree1", "commit1"); tracker.AddMissingRootTree("tree2", "commit1"); @@ -248,7 +254,7 @@ public void GetHighestMissingTreeCount_ReturnsCommitWithMostTrees() [TestCase] public void GetHighestMissingTreeCount_DoesNotUpdateLru() { - MissingTreeTracker tracker = new MissingTreeTracker(treeCapacity: 3); + MissingTreeTracker tracker = CreateTracker(treeCapacity: 3); tracker.AddMissingRootTree("tree1", "commit1"); tracker.AddMissingRootTree("tree2", "commit2"); @@ -273,7 +279,7 @@ public void GetHighestMissingTreeCount_DoesNotUpdateLru() [TestCase] public void MarkCommitComplete_RemovesAllTreesForCommit() { - MissingTreeTracker tracker = new MissingTreeTracker(treeCapacity: 10); + MissingTreeTracker tracker = CreateTracker(treeCapacity: 10); tracker.AddMissingRootTree("tree1", "commit1"); tracker.AddMissingRootTree("tree2", "commit1"); @@ -292,7 +298,7 @@ public void MarkCommitComplete_RemovesAllTreesForCommit() [TestCase] public void MarkCommitComplete_NonExistentCommit() { - MissingTreeTracker tracker = new MissingTreeTracker(treeCapacity: 10); + MissingTreeTracker tracker = CreateTracker(treeCapacity: 10); // Should not throw tracker.MarkCommitComplete("nonexistent"); @@ -301,7 +307,7 @@ public void MarkCommitComplete_NonExistentCommit() [TestCase] public void MarkCommitComplete_CascadesSharedTreesToOtherCommits() { - MissingTreeTracker tracker = new MissingTreeTracker(treeCapacity: 10); + MissingTreeTracker tracker = CreateTracker(treeCapacity: 10); // commit1 and commit2 share tree1; commit2 also has tree2 tracker.AddMissingRootTree("tree1", "commit1"); @@ -322,7 +328,7 @@ public void MarkCommitComplete_CascadesSharedTreesToOtherCommits() [TestCase] public void MarkCommitComplete_RemovesOtherCommitWhenItBecomesEmpty() { - MissingTreeTracker tracker = new MissingTreeTracker(treeCapacity: 10); + MissingTreeTracker tracker = CreateTracker(treeCapacity: 10); // commit2's only tree is shared with commit1 tracker.AddMissingRootTree("tree1", "commit1"); @@ -338,7 +344,7 @@ public void MarkCommitComplete_RemovesOtherCommitWhenItBecomesEmpty() [TestCase] public void MarkCommitComplete_DoesNotAffectUnrelatedCommits() { - MissingTreeTracker tracker = new MissingTreeTracker(treeCapacity: 10); + MissingTreeTracker tracker = CreateTracker(treeCapacity: 10); tracker.AddMissingRootTree("tree1", "commit1"); tracker.AddMissingRootTree("tree2", "commit2"); @@ -359,7 +365,7 @@ public void MarkCommitComplete_DoesNotAffectUnrelatedCommits() public void LruEviction_EvictsOldestCommit() { // treeCapacity = 3 trees; one tree per commit - MissingTreeTracker tracker = new MissingTreeTracker(treeCapacity: 3); + MissingTreeTracker tracker = CreateTracker(treeCapacity: 3); tracker.AddMissingRootTree("tree1", "commit1"); tracker.AddMissingRootTree("tree2", "commit2"); @@ -380,7 +386,7 @@ public void LruEviction_EvictsOldestCommit() public void LruEviction_DoesNotCascadeSharedTreesToOtherCommits() { // treeCapacity = 3 trees; tree1 is shared so only 2 unique trees + tree3 = 3 total - MissingTreeTracker tracker = new MissingTreeTracker(treeCapacity: 3); + MissingTreeTracker tracker = CreateTracker(treeCapacity: 3); // tree1 is shared between commit1 and commit2 (counts as 1 unique tree) tracker.AddMissingRootTree("tree1", "commit1"); @@ -405,7 +411,7 @@ public void LruEviction_DoesNotCascadeSharedTreesToOtherCommits() public void LruEviction_AddingTreeToExistingCommitUpdatesLru() { // treeCapacity = 4 trees; tree1, tree2, tree3 fill it, then tree1b re-uses commit1 - MissingTreeTracker tracker = new MissingTreeTracker(treeCapacity: 4); + MissingTreeTracker tracker = CreateTracker(treeCapacity: 4); tracker.AddMissingRootTree("tree1", "commit1"); tracker.AddMissingRootTree("tree2", "commit2"); @@ -427,7 +433,7 @@ public void LruEviction_AddingTreeToExistingCommitUpdatesLru() public void LruEviction_MultipleTreesPerCommit_EvictsEntireCommit() { // treeCapacity = 4 trees; commit1 holds 3, commit2 holds 1 - MissingTreeTracker tracker = new MissingTreeTracker(treeCapacity: 4); + MissingTreeTracker tracker = CreateTracker(treeCapacity: 4); tracker.AddMissingRootTree("tree1", "commit1"); tracker.AddMissingRootTree("tree2", "commit1"); @@ -452,7 +458,7 @@ public void LruEviction_MultipleTreesPerCommit_EvictsEntireCommit() [TestCase] public void LruEviction_CapacityOne() { - MissingTreeTracker tracker = new MissingTreeTracker(treeCapacity: 1); + MissingTreeTracker tracker = CreateTracker(treeCapacity: 1); tracker.AddMissingRootTree("tree1", "commit1"); tracker.GetHighestMissingTreeCount(new[] { "commit1" }, out _).ShouldEqual(1); @@ -468,7 +474,7 @@ public void LruEviction_ManyTreesOneCommit_ExceedsCapacity() { // treeCapacity = 3 trees; all trees belong to commit1 // Adding a 4th tree must evict commit1 (the only commit) to make room - MissingTreeTracker tracker = new MissingTreeTracker(treeCapacity: 3); + MissingTreeTracker tracker = CreateTracker(treeCapacity: 3); tracker.AddMissingRootTree("tree1", "commit1"); tracker.AddMissingRootTree("tree2", "commit1"); @@ -490,7 +496,7 @@ public void LruEviction_ManyTreesOneCommit_ExceedsCapacity() public void LruEviction_TryGetCommitsUpdatesLru() { // treeCapacity = 3 trees, one per commit - MissingTreeTracker tracker = new MissingTreeTracker(treeCapacity: 3); + MissingTreeTracker tracker = CreateTracker(treeCapacity: 3); tracker.AddMissingRootTree("tree1", "commit1"); tracker.AddMissingRootTree("tree2", "commit2");