Skip to content

Filters stream rebound #948

New issue

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

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

Already on GitHub? Sign in to your account

Merged
merged 21 commits into from
Feb 13, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Binary file added Lib/NativeBinaries/amd64/git2-a2012c4.dll
Binary file not shown.
Binary file not shown.
Binary file removed Lib/NativeBinaries/amd64/git2-e0902fb.dll
Binary file not shown.
Binary file added Lib/NativeBinaries/x86/git2-a2012c4.dll
Binary file not shown.
Binary file not shown.
Binary file removed Lib/NativeBinaries/x86/git2-e0902fb.dll
Binary file not shown.
48 changes: 48 additions & 0 deletions LibGit2Sharp.Tests/FilterFixture.cs
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,54 @@ public void WhenCheckingOutAFileFileSmudgeWritesCorrectFileToWorkingDirectory()
GlobalSettings.DeregisterFilter(filter);
}

[Fact]
public void FilterStreamsAreCoherent()
{
string repoPath = InitNewRepository();

bool? inputCanWrite = null, inputCanRead = null, inputCanSeek = null;
bool? outputCanWrite = null, outputCanRead = null, outputCanSeek = null;

Func<Stream, Stream, int> assertor = (input, output) =>
{
inputCanRead = input.CanRead;
inputCanWrite = input.CanWrite;
inputCanSeek = input.CanSeek;

outputCanRead = output.CanRead;
outputCanWrite = output.CanWrite;
outputCanSeek = output.CanSeek;

return GitPassThrough;
};

var filter = new FakeFilter(FilterName + 18, Attribute, checkSuccess, assertor, assertor);

GlobalSettings.RegisterFilter(filter);

using (var repo = CreateTestRepository(repoPath))
{
StageNewFile(repo);
}

GlobalSettings.DeregisterFilter(filter);

Assert.True(inputCanRead.HasValue);
Assert.True(inputCanWrite.HasValue);
Assert.True(inputCanSeek.HasValue);
Assert.True(outputCanRead.HasValue);
Assert.True(outputCanWrite.HasValue);
Assert.True(outputCanSeek.HasValue);

Assert.True(inputCanRead.Value);
Assert.False(inputCanWrite.Value);
Assert.False(inputCanSeek.Value);

Assert.False(outputCanRead.Value);
Assert.True(outputCanWrite.Value);
Assert.False(outputCanSeek.Value);
}

private FileInfo CheckoutFileForSmudge(string repoPath, string branchName, string content)
{
FileInfo expectedPath;
Expand Down
2 changes: 1 addition & 1 deletion LibGit2Sharp.Tests/SubstitutionCipherFilterFixture.cs
Original file line number Diff line number Diff line change
Expand Up @@ -93,4 +93,4 @@ private static Blob CommitOnBranchAndReturnDatabaseBlob(Repository repo, string
return blob;
}
}
}
}
3 changes: 1 addition & 2 deletions LibGit2Sharp.Tests/TestHelpers/SubstitutionCipherFilter.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Text;

namespace LibGit2Sharp.Tests.TestHelpers
Expand Down Expand Up @@ -59,4 +58,4 @@ public static int RotateByThirteenPlaces(Stream input, Stream output)
}
}
}
}
}
24 changes: 11 additions & 13 deletions LibGit2Sharp/Core/GitBufReadStream.cs
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,6 @@ namespace LibGit2Sharp.Core
/// </summary>
internal class GitBufReadStream : UnmanagedMemoryStream
{
private readonly GitBuf gitBuf;

internal GitBufReadStream(IntPtr gitBufPointer)
: this(gitBufPointer.MarshalAs<GitBuf>())
{ }
Expand All @@ -21,17 +19,7 @@ private unsafe GitBufReadStream(GitBuf gitBuf)
ConvertToLong(gitBuf.size),
ConvertToLong(gitBuf.asize),
FileAccess.Read)
{
this.gitBuf = gitBuf;
}

protected override void Dispose(bool disposing)
{
base.Dispose(disposing);

if (disposing && gitBuf != default(GitBuf))
gitBuf.Dispose();
}
{ }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume the GitBuf is owned by libgit2, so we shouldn't touch it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. See #946 (comment)


private static long ConvertToLong(UIntPtr len)
{
Expand All @@ -46,5 +34,15 @@ private static long ConvertToLong(UIntPtr len)

return (long)len.ToUInt64();
}

public override long Seek(long offset, SeekOrigin loc)
{
throw new NotSupportedException();
}

public override bool CanSeek
{
get { return false; }
}
}
}
88 changes: 63 additions & 25 deletions LibGit2Sharp/Core/GitBufWriteStream.cs
Original file line number Diff line number Diff line change
@@ -1,53 +1,91 @@
using System;
using System.IO;
using System.Runtime.InteropServices;
using LibGit2Sharp.Core.Handles;

namespace LibGit2Sharp.Core
{
/// <summary>
/// Writes data to a <see cref="GitBuf"/> pointer
/// </summary>
internal class GitBufWriteStream : MemoryStream
internal class GitBufWriteStream : Stream
{
private readonly IntPtr gitBufPointer;

internal GitBufWriteStream(IntPtr gitBufPointer)
{
this.gitBufPointer = gitBufPointer;

//Preallocate the buffer
Proxy.git_buf_grow(gitBufPointer, 1024);
}

public override void Flush()
{
}

protected override void Dispose(bool disposing)
public override long Seek(long offset, SeekOrigin origin)
{
if (base.CanSeek) // False if stream has already been written/closed
throw new NotSupportedException();
}

public override void SetLength(long value)
{
throw new NotSupportedException();
}

public override int Read(byte[] buffer, int offset, int count)
{
throw new NotSupportedException();
}

public override void Write(byte[] buffer, int offset, int count)
{
AutoGrowBuffer(count);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? git_buf knows how to grow itself (that is much of the point for it existing).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cmn @ammeep How about preallocating 4kb?

@cmn Some local tests show that this (basic) auto grow mechanism leads to fewer reallocs when compared to bare git_buf_put() usage.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Allocating a page would be better yes (assuming you do mean 4kB). The difference in allocations is surprising, since you're also growing by 1.5 and the size is ensured anyhow. The one difference seems to be that you assume there is going to be a next request and preallocate for it. How large is the difference in reallocs? What's the difference in memory consumption?


Proxy.git_buf_put(gitBufPointer, buffer, offset, count);
}

private void AutoGrowBuffer(int count)
{
var gitBuf = gitBufPointer.MarshalAs<GitBuf>();

var asize = (uint)gitBuf.asize;
var size = (uint)gitBuf.size;

var isBufferLargeEnoughToHoldTheNewData = (asize - size) > count;
var filledBufferPercentage = (100.0 * size / asize);

if (isBufferLargeEnoughToHoldTheNewData && filledBufferPercentage < 90)
{
using (var gitBuf = gitBufPointer.MarshalAs<GitBuf>())
{
WriteTo(gitBuf);
}
return;
}

base.Dispose(disposing);
var targetSize = (uint)(1.5 * (asize + count));

Proxy.git_buf_grow(gitBufPointer, targetSize);
}

private void WriteTo(GitBuf gitBuf)
public override bool CanRead
{
Seek(0, SeekOrigin.Begin);
get { return false; }
}

var length = (int)Length;
var bytes = new byte[length];
Read(bytes, 0, length);
public override bool CanSeek
{
get { return false; }
}

IntPtr reverseBytesPointer = Marshal.AllocHGlobal(length);
Marshal.Copy(bytes, 0, reverseBytesPointer, bytes.Length);
public override bool CanWrite
{
get { return true; }
}

var size = (UIntPtr)length;
var allocatedSize = (UIntPtr)length;
NativeMethods.git_buf_set(gitBuf, reverseBytesPointer, size);
gitBuf.size = size;
gitBuf.asize = allocatedSize;
public override long Length
{
get { throw new NotSupportedException(); }
}

Marshal.StructureToPtr(gitBuf, gitBufPointer, true);
public override long Position
{
get { throw new NotSupportedException(); }
set { throw new NotSupportedException(); }
}
}
}
2 changes: 1 addition & 1 deletion LibGit2Sharp/Core/NativeDllName.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,6 @@ namespace LibGit2Sharp.Core
{
internal static class NativeDllName
{
public const string Name = "git2-e0902fb";
public const string Name = "git2-a2012c4";
}
}
4 changes: 2 additions & 2 deletions LibGit2Sharp/Core/NativeMethods.cs
Original file line number Diff line number Diff line change
Expand Up @@ -222,10 +222,10 @@ internal static extern int git_branch_remote_name(
[MarshalAs(UnmanagedType.CustomMarshaler, MarshalCookie = UniqueId.UniqueIdentifier, MarshalTypeRef = typeof(StrictUtf8Marshaler))] string canonical_branch_name);

[DllImport(libgit2)]
internal static extern int git_buf_grow(GitBuf buffer, UIntPtr targetSize);
internal static extern int git_buf_grow(IntPtr buffer, UIntPtr targetSize);

[DllImport(libgit2)]
internal static extern int git_buf_set(GitBuf buffer, IntPtr data, UIntPtr targetSize);
internal static extern int git_buf_put(IntPtr buffer, IntPtr data, UIntPtr len);

[DllImport(libgit2)]
internal static extern int git_remote_rename(
Expand Down
27 changes: 27 additions & 0 deletions LibGit2Sharp/Core/Proxy.cs
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,33 @@ public static string git_branch_upstream_name(RepositorySafeHandle handle, strin

#region git_buf_

public static void git_buf_grow(IntPtr gitBufPointer, uint target_size)
{
using (ThreadAffinity())
{
var res = NativeMethods.git_buf_grow(gitBufPointer, (UIntPtr)target_size);
Ensure.ZeroResult(res);
}
}

public static void git_buf_put(IntPtr gitBufPointer, byte[] data, int offset, int count)
{
using (ThreadAffinity())
{
unsafe
{
int res;

fixed (byte* ptr = data)
{
res = NativeMethods.git_buf_put(gitBufPointer, (IntPtr)ptr, (UIntPtr)count);
}

Ensure.ZeroResult(res);
}
}
}

public static void git_buf_free(GitBuf buf)
{
NativeMethods.git_buf_free(buf);
Expand Down
34 changes: 17 additions & 17 deletions LibGit2Sharp/Filter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ public abstract class Filter : IEquatable<Filter>

/// <summary>
/// Initializes a new instance of the <see cref="Filter"/> class.
/// And allocates the filter natively.
/// And allocates the filter natively.
/// <param name="name">The unique name with which this filtered is registered with</param>
/// <param name="attributes">A list of filterForAttributes which this filter applies to</param>
/// </summary>
Expand All @@ -33,7 +33,7 @@ protected Filter(string name, IEnumerable<string> attributes)

/// <summary>
/// Initializes a new instance of the <see cref="Filter"/> class.
/// And allocates the filter natively.
/// And allocates the filter natively.
/// <param name="name">The unique name with which this filtered is registered with</param>
/// <param name="attributes">Either a single attribute, or a comma separated list of filterForAttributes for which this filter applies to</param>
/// </summary>
Expand Down Expand Up @@ -87,10 +87,10 @@ internal GitFilter ManagedFilter

/// <summary>
/// Initialize callback on filter
///
///
/// Specified as `filter.initialize`, this is an optional callback invoked
/// before a filter is first used. It will be called once at most.
///
///
/// If non-NULL, the filter's `initialize` callback will be invoked right
/// before the first use of the filter, so you can defer expensive
/// initialization operations (in case libgit2 is being used in a way that doesn't need the filter).
Expand Down Expand Up @@ -191,10 +191,10 @@ public override int GetHashCode()

/// <summary>
/// Initialize callback on filter
///
///
/// Specified as `filter.initialize`, this is an optional callback invoked
/// before a filter is first used. It will be called once at most.
///
///
/// If non-NULL, the filter's `initialize` callback will be invoked right
/// before the first use of the filter, so you can defer expensive
/// initialization operations (in case libgit2 is being used in a way that doesn't need the filter).
Expand All @@ -207,15 +207,15 @@ int InitializeCallback(IntPtr gitFilter)
/// <summary>
/// Callback to decide if a given source needs this filter
/// Specified as `filter.check`, this is an optional callback that checks if filtering is needed for a given source.
///
/// It should return 0 if the filter should be applied (i.e. success), GIT_PASSTHROUGH if the filter should
///
/// It should return 0 if the filter should be applied (i.e. success), GIT_PASSTHROUGH if the filter should
/// not be applied, or an error code to fail out of the filter processing pipeline and return to the caller.
///
///
/// The `attr_values` will be set to the values of any filterForAttributes given in the filter definition. See `git_filter` below for more detail.
///
/// The `payload` will be a pointer to a reference payload for the filter. This will start as NULL, but `check` can assign to this
///
/// The `payload` will be a pointer to a reference payload for the filter. This will start as NULL, but `check` can assign to this
/// pointer for later use by the `apply` callback. Note that the value should be heap allocated (not stack), so that it doesn't go
/// away before the `apply` callback can use it. If a filter allocates and assigns a value to the `payload`, it will need a `cleanup`
/// away before the `apply` callback can use it. If a filter allocates and assigns a value to the `payload`, it will need a `cleanup`
/// callback to free the payload.
/// </summary>
/// <returns></returns>
Expand All @@ -230,12 +230,12 @@ int CheckCallback(GitFilter gitFilter, IntPtr payload, IntPtr filterSourcePtr, I

/// <summary>
/// Callback to actually perform the data filtering
///
/// Specified as `filter.apply`, this is the callback that actually filters data.
///
/// Specified as `filter.apply`, this is the callback that actually filters data.
/// If it successfully writes the output, it should return 0. Like `check`,
/// it can return GIT_PASSTHROUGH to indicate that the filter doesn't want to run.
/// it can return GIT_PASSTHROUGH to indicate that the filter doesn't want to run.
/// Other error codes will stop filter processing and return to the caller.
///
///
/// The `payload` value will refer to any payload that was set by the `check` callback. It may be read from or written to as needed.
/// </summary>
int ApplyCallback(GitFilter gitFilter, IntPtr payload,
Expand All @@ -251,4 +251,4 @@ int ApplyCallback(GitFilter gitFilter, IntPtr payload,
}
}
}
}
}
2 changes: 1 addition & 1 deletion LibGit2Sharp/libgit2_hash.txt
Original file line number Diff line number Diff line change
@@ -1 +1 @@
e0902fbce7d14631bd02091c1c70cde3e68f78ab
a2012c43899e6616366b47d7741b3f035e825e84
Loading