Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix performance regression in LibGit2RepoInvoker #1278

Merged
merged 7 commits into from
Jun 17, 2019
Merged

Fix performance regression in LibGit2RepoInvoker #1278

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
50ffac1
LibGit2Repo: Do not dispose on a timer
derrickstolee Jun 13, 2019
c544b1a
LibGit2RepoInvoker: Run a simple query to preload data
derrickstolee Jun 13, 2019
3e3279d
LibGit2RepoInvoker: cleanups from feedback
derrickstolee Jun 13, 2019
c3213b7
LibGit2RepoInvoker: Remove unused Timer
derrickstolee Jun 14, 2019
6e00a0f
LibGit2RepoInvoker: Test a real SHA1 when initializing
derrickstolee Jun 14, 2019
a8d174d
PrefetchStep: Close libgit2 repo before deleting .idx files
derrickstolee Jun 17, 2019
d1054d3
PrefetchTests: Unmount before deleting packs
derrickstolee Jun 17, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions GVFS/GVFS.Common/Git/GitRepo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,14 +44,22 @@ private enum LooseBlobState
Unknown,
}

public bool HasActiveLibGit2Repo => this.libgit2RepoInvoker?.IsActive == true;

public GVFSLock GVFSLock
{
get;
private set;
}

public void CloseActiveRepo()
{
this.libgit2RepoInvoker?.DisposeSharedRepo();
}

public void OpenRepo()
{
this.libgit2RepoInvoker?.InitializedSharedRepo();
}

public bool TryGetIsBlob(string sha, out bool isBlob)
{
return this.libgit2RepoInvoker.TryInvoke(repo => repo.IsBlob(sha), out isBlob);
Expand Down
59 changes: 23 additions & 36 deletions GVFS/GVFS.Common/Git/LibGit2RepoInvoker.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,40 +6,22 @@ namespace GVFS.Common.Git
{
public class LibGit2RepoInvoker : IDisposable
{
private static readonly TimeSpan DefaultRepositoryDisposalPeriod = TimeSpan.FromMinutes(15);

private readonly Func<LibGit2Repo> createRepo;
private readonly ITracer tracer;
private readonly object sharedRepoLock = new object();
private readonly TimeSpan sharedRepositoryDisposalPeriod;
private volatile bool disposing;
private volatile int activeCallers;
private LibGit2Repo sharedRepo;
private Timer sharedRepoDisposalTimer;

public LibGit2RepoInvoker(ITracer tracer, Func<LibGit2Repo> createRepo, TimeSpan? disposalPeriod = null)
public LibGit2RepoInvoker(ITracer tracer, Func<LibGit2Repo> createRepo)
{
this.tracer = tracer;
this.createRepo = createRepo;

if (!disposalPeriod.HasValue || disposalPeriod.Value <= TimeSpan.Zero)
{
this.sharedRepositoryDisposalPeriod = DefaultRepositoryDisposalPeriod;
}
else
{
this.sharedRepositoryDisposalPeriod = disposalPeriod.Value;
}

this.sharedRepoDisposalTimer = new Timer(
(state) => this.DisposeSharedRepo(),
state: null,
dueTime: this.sharedRepositoryDisposalPeriod,
period: this.sharedRepositoryDisposalPeriod);
this.InitializedSharedRepo();
}

public bool IsActive => this.sharedRepo != null;

public void Dispose()
{
this.disposing = true;
Expand Down Expand Up @@ -81,37 +63,42 @@ public bool TryInvoke<TResult>(Func<LibGit2Repo, TResult> function, out TResult
}
}

private LibGit2Repo GetSharedRepo()
public void DisposeSharedRepo()
{
lock (this.sharedRepoLock)
{
if (this.disposing)
{
return null;
}

this.sharedRepoDisposalTimer?.Change(this.sharedRepositoryDisposalPeriod, this.sharedRepositoryDisposalPeriod);

if (this.sharedRepo == null)
if (this.disposing || this.activeCallers > 0)
{
this.sharedRepo = this.createRepo();
return;
}

return this.sharedRepo;
this.sharedRepo?.Dispose();
this.sharedRepo = null;
}
}

private void DisposeSharedRepo()
public void InitializedSharedRepo()
{
// Run a test on the shared repo to ensure the object store
// is loaded, as that is what takes a long time with many packs.
this.GetSharedRepo().ObjectExists(GVFSConstants.AllZeroSha);
}

private LibGit2Repo GetSharedRepo()
{
lock (this.sharedRepoLock)
{
if (this.disposing || this.activeCallers > 0)
if (this.disposing)
{
return;
return null;
}

this.sharedRepo?.Dispose();
this.sharedRepo = null;
if (this.sharedRepo == null)
{
this.sharedRepo = this.createRepo();
}

return this.sharedRepo;
}
}
}
Expand Down
30 changes: 17 additions & 13 deletions GVFS/GVFS.Common/Maintenance/PackfileMaintenanceStep.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,18 +128,6 @@ protected override void PerformMaintenance()
activity.RelatedWarning($"Skipping {nameof(PackfileMaintenanceStep)} due to git pids {string.Join(",", processIds)}", Keywords.Telemetry);
return;
}

// If a LibGit2Repo is active, then it may hold handles to the .idx and .pack files we want
// to delete during the 'git multi-pack-index expire' step. If one starts during the step,
// then it can still block those deletions, but we will clean them up in the next run. By
// checking HasActiveLibGit2Repo here, we ensure that we do not run twice with the same
// LibGit2Repo active across two calls. A "new" repo should not hold handles to .idx files
// that do not have corresponding .pack files, so we will clean them up in CleanStaleIdxFiles().
if (this.Context.Repository.HasActiveLibGit2Repo)
{
activity.RelatedWarning($"Skipping {nameof(PackfileMaintenanceStep)} due to active libgit2 repo", Keywords.Telemetry);
return;
}
}

this.GetPackFilesInfo(out int beforeCount, out long beforeSize, out bool hasKeep);
Expand All @@ -150,7 +138,23 @@ protected override void PerformMaintenance()
return;
}

GitProcess.Result expireResult = this.RunGitCommand((process) => process.MultiPackIndexExpire(this.Context.Enlistment.GitObjectsRoot), nameof(GitProcess.MultiPackIndexExpire));
try
{
// If a LibGit2Repo is active, then it may hold handles to the .idx and .pack files we want
// to delete during the 'git multi-pack-index expire' step. If one starts during the step,
// then it can still block those deletions, but we will clean them up in the next run. By
// running CloseActiveRepos() here, we ensure that we do not run twice with the same
// LibGit2Repo active across two calls. A "new" repo should not hold handles to .idx files
// that do not have corresponding .pack files, so we will clean them up in CleanStaleIdxFiles().
this.Context.Repository.CloseActiveRepo();

GitProcess.Result expireResult = this.RunGitCommand((process) => process.MultiPackIndexExpire(this.Context.Enlistment.GitObjectsRoot), nameof(GitProcess.MultiPackIndexExpire));
}
finally
{
this.Context.Repository.OpenRepo();
}

List<string> staleIdxFiles = this.CleanStaleIdxFiles(out int numDeletionBlocked);
this.GetPackFilesInfo(out int expireCount, out long expireSize, out hasKeep);

Expand Down
83 changes: 50 additions & 33 deletions GVFS/GVFS.UnitTests/Common/LibGit2RepoInvokerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
using GVFS.Tests.Should;
using GVFS.UnitTests.Mock.Common;
using NUnit.Framework;
using System;
using System.Collections.Concurrent;
using System.Threading;

Expand All @@ -11,7 +10,6 @@ namespace GVFS.UnitTests.Common
[TestFixture]
public class LibGit2RepoInvokerTests
{
private readonly TimeSpan disposalPeriod = TimeSpan.FromMilliseconds(1);
private MockTracer tracer;
private LibGit2RepoInvoker invoker;
private int numConstructors;
Expand All @@ -25,32 +23,65 @@ public void Setup()
this.invoker?.Dispose();

this.tracer = new MockTracer();
this.invoker = new LibGit2RepoInvoker(this.tracer, this.CreateRepo, this.disposalPeriod);
this.numConstructors = 0;
this.numDisposals = 0;
this.DisposalTriggers = new BlockingCollection<object>();

this.invoker = new LibGit2RepoInvoker(this.tracer, this.CreateRepo);
}

[TestCase]
public void DoesNotCreateRepoOnConstruction()
public void DoesCreateRepoOnConstruction()
{
this.numConstructors.ShouldEqual(0);
this.numConstructors.ShouldEqual(1);
}

[TestCase]
public void CreatesRepoOnTryInvoke()
public void CreatesOnRequestAfterClosed()
{
this.numConstructors.ShouldEqual(0);
this.numConstructors.ShouldEqual(1);

this.invoker.DisposeSharedRepo();

this.numDisposals.ShouldEqual(1);
this.numConstructors.ShouldEqual(1);

this.invoker.InitializedSharedRepo();

this.numDisposals.ShouldEqual(1);
this.numConstructors.ShouldEqual(2);

this.invoker.TryInvoke(repo => { return true; }, out bool result);
result.ShouldEqual(true);

this.numDisposals.ShouldEqual(1);
this.numConstructors.ShouldEqual(2);
}

[TestCase]
public void CreatesOnInvokeAfterClosed()
{
this.numConstructors.ShouldEqual(1);

this.invoker.DisposeSharedRepo();

this.numDisposals.ShouldEqual(1);
this.numConstructors.ShouldEqual(1);

this.invoker.TryInvoke(repo => { return true; }, out bool result);

this.numDisposals.ShouldEqual(1);
this.numConstructors.ShouldEqual(2);

this.invoker.InitializedSharedRepo();

this.numDisposals.ShouldEqual(1);
this.numConstructors.ShouldEqual(2);
}

[TestCase]
public void DoesNotCreateMultipleRepos()
{
this.numConstructors.ShouldEqual(0);
this.numConstructors.ShouldEqual(1);

this.invoker.TryInvoke(repo => { return true; }, out bool result);
result.ShouldEqual(true);
Expand All @@ -60,25 +91,24 @@ public void DoesNotCreateMultipleRepos()
result.ShouldEqual(true);
this.numConstructors.ShouldEqual(1);

this.invoker.TryInvoke(repo => { return true; }, out result);
result.ShouldEqual(true);
this.invoker.InitializedSharedRepo();
this.numConstructors.ShouldEqual(1);
}

[TestCase]
public void DoesNotCreateRepoAfterDisposal()
{
this.numConstructors.ShouldEqual(0);
this.numConstructors.ShouldEqual(1);
this.invoker.Dispose();
this.invoker.TryInvoke(repo => { return true; }, out bool result);
result.ShouldEqual(false);
this.numConstructors.ShouldEqual(0);
this.numConstructors.ShouldEqual(1);
}

[TestCase]
public void DisposesSharedRepo()
{
this.numConstructors.ShouldEqual(0);
this.numConstructors.ShouldEqual(1);
this.numDisposals.ShouldEqual(0);

this.invoker.TryInvoke(repo => { return true; }, out bool result);
Expand All @@ -93,7 +123,7 @@ public void DisposesSharedRepo()
[TestCase]
public void UsesOnlyOneRepoMultipleThreads()
{
this.numConstructors.ShouldEqual(0);
this.numConstructors.ShouldEqual(1);

Thread[] threads = new Thread[10];
BlockingCollection<object> threadStarted = new BlockingCollection<object>();
Expand Down Expand Up @@ -138,24 +168,6 @@ public void UsesOnlyOneRepoMultipleThreads()
this.numConstructors.ShouldEqual(1);
}

[TestCase]
public void AutomaticallyDisposesAfterNoUse()
{
this.numConstructors.ShouldEqual(0);

bool tryInvokeResult1 = this.invoker.TryInvoke(repo => { return true; }, out bool result);
result.ShouldEqual(true, string.Format("Unexcepted function result from first call to TryInvoke: {0} ReturnCode: {1}", result, tryInvokeResult1));
this.numConstructors.ShouldEqual(1);

this.DisposalTriggers.TryTake(out object _, (int)this.disposalPeriod.TotalMilliseconds * 500).ShouldBeTrue("Did not dispose object in time");
this.numDisposals.ShouldEqual(1);
this.numConstructors.ShouldEqual(1);

bool tryInvokeResult2 = this.invoker.TryInvoke(repo => { return true; }, out result);
result.ShouldEqual(true, string.Format("Unexcepted function result from second call to TryInvoke: {0} ReturnCode: {1}", result, tryInvokeResult2));
this.numConstructors.ShouldEqual(2);
}

private LibGit2Repo CreateRepo()
{
Interlocked.Increment(ref this.numConstructors);
Expand All @@ -171,6 +183,11 @@ public MockLibGit2Repo(LibGit2RepoInvokerTests parent)
this.parent = parent;
}

public override bool ObjectExists(string sha)
{
return false;
}

protected override void Dispose(bool disposing)
{
if (disposing)
Expand Down