Streaming Filter Support #1030

whoisj · 2015-04-23T22:17:02Z

Completes the work started by @ammeep for getting filters working in Libgit2Sharp (thanks for the awesome starting point!). Makes use of the streaming filter infrastructure added by @ethomson in Libgit2 to minimize the memory footprint. The goal is to support the Git-LFS filter, so one has to assume the streams won't fit in memory.

Fairly simple design. Added a new GitWriteStream to wrap the native git_writestream, then implements all logic for doing the actual stream work in the Filter base class. The rest of the work was done by @ammeep 🆒

Fixes #1025

nulltoken · 2015-04-24T07:13:03Z

LibGit2Sharp/Core/GitWriteStream.cs

+        [MarshalAs(UnmanagedType.FunctionPtr)]
+        public free_fn free;        
+
+        public delegate int write_fn(IntPtr stream, IntPtr buffer, uint len);


I think we should rather marshal size_t into a UIntPtr

nulltoken · 2015-04-24T07:29:53Z

Wow! That's a pretty nice interop job. Thanks for giving some love to those filters.

/cc @shiftkey @ammeep @ethomson @jamill @Therzok Could you please also take a peek at this PR?

ethomson · 2015-04-24T12:55:55Z

I think this looks great. Thanks @whoisj

jamill · 2015-04-24T14:00:08Z

LibGit2Sharp/FilterAttributeEntry.cs

+        {
+            Ensure.ArgumentNotNullOrEmptyString(filterName, "filterName");
+
+            if (!filterName.Contains(AttributeFilterDefinition))


Is Contains(...) the right check here? I was kinda expecting a StartsWith(...) with an explicit string comparer...

Agreed, I'll change this to be .StartsWith(..., Ordinal)

shiftkey · 2015-04-24T18:26:52Z

👀 will test this out against GHfW

whoisj · 2015-04-24T18:43:17Z

@jamill Yes, agreed - literals (aka magic numbers) should be moved into constants. My bad, tired brains take shortcuts. I'll fix this up ASAP. As for the exception, I'm using it as flow control. I suppose I should use something cheaper like a continue or goto and keep the exception for exceptional circumstances.

In general, is it better in a native-managed-native call stack to just avoid exceptions all together? Every project seems to have its own set of rules about this. I don't see anything in the .md files about this. What is expected in Libgit2Sharp?

/cc @nulltoken , @ethomson , @shiftkey

whoisj · 2015-04-24T19:02:04Z

Thanks for all of the great feedback. Fixes pushed for all with the exception of the discussion @ethomson and @jamill are having with regards to requiring "filter=". I'm not sure if that should be part of this PR or not.

I agree with the explicit requirement, not sure what kinds of errors it should produce, etc.

jamill · 2015-04-24T20:45:38Z

@ethomson and @jamill having with regards to requiring "filter=". I'm not sure if that should be part of this PR or not.

I think @ethomson and I are in agreement on always adding the filter= prefix to the filter name, and expect that clients will never specify the filter= prefix. I would prefer to have this behavior in the original version, if it is not too much work (unless @ammeep or others have objections), but not too strongly. I could see this going in in a separate PR.

ammeep · 2015-04-25T00:26:49Z

@whoisj this is wonderful! Thank you so much for taking this on, I have been swamped. 😍

We ended up making a fork of libgit2sharp without the api changes in libgit2, which GitHub for Windows is currently using. https://github.com/github/libgit2sharp/tree/ghfw-p-flow-plus-filters

It includes one change that didn't make it into the original libgit2sharp PR github@dbea75e which allows us to pass the working directory of the repository to the clean and smudge overloads on the filter. This turns out to be important because most filter CLI tools (or at least the Git LFS CLI) require the working directory of the repository.

Do you have any objections to adding this to the API contract?

whoisj · 2015-04-25T00:59:14Z

@ammeep I think that makes sense, no objections here.

whoisj · 2015-04-27T16:43:16Z

Updated the PR with changes requested by @ammeep as well as added a necessary, missing callback on filter close to allow the filter to do any final, necessary work before being closed.

shiftkey · 2015-04-27T19:31:58Z

Just ran this through our existing test suite in GitHub for Windows - I'm 👍 on this PR. Thanks for the hard work @whoisj!

nulltoken · 2015-06-10T18:25:05Z

Dammit! The PR isn't mergeable any longer. Could you please rebase it?

Added complete callback to facilitate long running or transforming filters like Git-LFS. Added repository's working directory to callbacks to support extern process like Git-LFS, where knowledge of a repository's root maybe necissary but unobtainable in another fasion. Removed unused NativeMethods

@whoisj

The filters are run in-line with the rest of the system, and libgit2 is set up to have its last filter/writestream write to the filesystem. Instead of using an intermediate buffer/file/pipe, we can make the writes from the user's Smude() or Clean() filter into their output stream propagate immediately into the next filter, leading up the chain to the filesystem, removing any intermediate buffering other than that which is necessary for the filters themselves to work. In the general case, we only have the one clean or smudge filter, which means that even in the case of out-of-band storage for large files, where a small blob can cause very large outputs, we would still write directly to the filesystem. by J Wyman (@whoisj): Buffered writes to next filter to minimize managed to native marshaling for performance optimizations by J Wyman (@whoisj): Split `WriteStream` into its own file and cleaned it up (checks, style, etc)

whoisj · 2015-06-10T19:41:36Z

@nulltoken oh that was a fun merge (in the text sense) 😑

Streaming Filter Support

nulltoken · 2015-06-10T19:57:08Z

@whoisj @ammeep

whoisj · 2015-06-10T19:57:55Z

🎉 🎈 🍻 😌

dahlbyk · 2015-06-10T20:45:17Z

Epic.

ammeep · 2015-06-10T22:13:32Z

🎉 Nice work @whoisj!!

shiftkey · 2015-06-10T22:42:14Z

Nice work @ammeep and @whoisj!

whoisj · 2015-06-10T22:47:12Z

Thanks to @ethomson for the base work in Libgit2, @ammeep for the base work in this library, and to @carlosmn for being awesome and putting my head on straight there at the end. 🙇

nulltoken · 2015-06-11T13:54:16Z

@whoisj This AppVeyor job shows some strange failures related this PR. Any idea?

LibGit2Sharp.Tests.FilterFixture.CanFilterLargeFiles [FAIL]
   System.NullReferenceException : Object reference not set to an instance of an object.

LibGit2Sharp.Tests.FilterFixture.WhenStagingFileApplyIsCalledWithCleanForCorrectPath [FAIL]
   System.NullReferenceException : Object reference not set to an instance of an object.

LibGit2Sharp.Tests.FilterFixture.CleanFilterWritesOutputToObjectTree [FAIL]
   System.NullReferenceException : Object reference not set to an instance of an object.

LibGit2Sharp.Tests.FilterFixture.WhenCheckingOutAFileFileSmudgeWritesCorrectFileToWorkingDirectory [FAIL]
   System.NullReferenceException : Object reference not set to an instance of an object.

nulltoken · 2015-06-11T14:06:32Z

Some more detail in this other one

LibGit2Sharp.Tests.FilterFixture.CanFilterLargeFiles [FAIL]
   System.NullReferenceException : Object reference not set to an instance of an object.

LibGit2Sharp.Tests.FilterFixture.CleanFilterWritesOutputToObjectTree [FAIL]
   System.NullReferenceException : Object reference not set to an instance of an object.

LibGit2Sharp.Tests.FilterFixture.WhenCheckingOutAFileFileSmudgeWritesCorrectFileToWorkingDirectory [FAIL]
   LibGit2Sharp.LibGit2SharpException : Unexpected IntPtr value
   Parameter name: stream
   Stack Trace:
      c:\projects\libgit2sharp\LibGit2Sharp\Core\Ensure.cs(154,0): at LibGit2Sharp.Core.Ensure.HandleError(Int32 result)
      c:\projects\libgit2sharp\LibGit2Sharp\Repository.cs(1722,0): at LibGit2Sharp.Repository.Stage(IEnumerable`1 paths, StageOptions stageOptions)
      c:\projects\libgit2sharp\LibGit2Sharp.Tests\FilterFixture.cs(319,0): at LibGit2Sharp.Tests.FilterFixture.StageNewFile(IRepository repo, String contents)
      c:\projects\libgit2sharp\LibGit2Sharp.Tests\FilterFixture.cs(293,0): at LibGit2Sharp.Tests.FilterFixture.CheckoutFileForSmudge(String repoPath, String branchName, String content)
      c:\projects\libgit2sharp\LibGit2Sharp.Tests\FilterFixture.cs(170,0): at LibGit2Sharp.Tests.FilterFixture.WhenCheckingOutAFileFileSmudgeWritesCorrectFileToWorkingDirectory()

LibGit2Sharp.Tests.FilterFixture.WhenStagingFileApplyIsCalledWithCleanForCorrectPath [FAIL]
   System.NullReferenceException : Object reference not set to an instance of an object.

whoisj · 2015-06-11T14:30:27Z

@nulltoken it's very possible something went wrong in the series of rebases the PR took just before merge. I'm not able to reproduce these failures locally. Do they only happen in AppVeyor or can you reproduce them locally?

nulltoken · 2015-06-11T14:37:37Z

I'm not able to reproduce these failures locally.

I'm not either.

Weirder, see https://ci.appveyor.com/project/libgit2/libgit2sharp/history

Build 934 fails -> Failure to create branch #859
Build 935 passes -> Ensure that conflicts are staged and never ignored #1062
Build 936 fails -> Failure to create branch #859

Just restarted #859 build to see if this is consistent (although I'm not sure to understand why). Maybe AppVeyor prefers @ethomson's code over @jamill's code.

nulltoken · 2015-06-11T14:51:06Z

Very strange. #859 is now passing.

This kind of random issues makes me feel very uneasy especially when the tests do not rely on external dependencies (eg. network).

whoisj · 2015-06-11T14:56:03Z

@nulltoken I see trancient test failures commonly in the BlobFixture, the FilterFixture, the ArchiveFixture, and the CheckoutFixture several times a day and a re-run of the tests nearly always resolves the issue. The errors are always the same: either NullReferenceException or DirectoryNotFound exceptions - at least for me.

This has been happening since before I started on the project and since they were always transient and always resolved by a re-run I ignored them.

nulltoken · 2015-06-11T15:01:01Z

@whoisj Wow. That is very weird. I've never seen BlobFixture/ArchiveFixture/CheckoutFixture failing. Neither locally, nor in the CI logs.

With regards to your failures, are you still running the tests against a RAM disk? Could we be experiencing IO concurrency issues?

whoisj · 2015-06-11T15:19:36Z

@nulltoken I am using the RAM disk locally, but the transient failures have been happening since before I switched from SSD to RAM. I do not think they're related. One piece of information I do have is it feels like it's more common to see transient failures immediately after building. In fact, I do not believe I've ever seen them when using an existing DLL. JIT issues?

nulltoken · 2015-06-11T15:42:38Z

Regarding #1030 (comment) @jeffhostetler was hinting at a possible GC related reclaim.

dahlbyk · 2015-06-18T21:38:28Z

CI FilterFixture failure: https://ci.appveyor.com/project/libgit2/libgit2sharp/build/995/job/qse6k53u5v040045#L205

carlosmn · 2015-06-19T09:46:56Z

One of those errors are from writing to the next stream, which sets its own error message. It should provide more information about the underlying cause if WriteStream.Write() were to use Ensure.ZeroResult() as it could simply be that we're running out of disk space, but in any case it should tell us why libgit2 returned an error.

nulltoken reviewed Apr 24, 2015
View reviewed changes

jamill reviewed Apr 24, 2015
View reviewed changes

ammeep and others added 4 commits June 10, 2015 12:20

Added custom filter support

eb442fc

Added LFS like test to FilterFixture

71dc2c1

nulltoken added a commit that referenced this pull request Jun 10, 2015

Merge pull request #1030 from whoisj/streaming-filter-support

431e9c6

Streaming Filter Support

nulltoken merged commit 431e9c6 into libgit2:vNext Jun 10, 2015

nulltoken added this to the v0.22 milestone Jun 10, 2015

whoisj deleted the streaming-filter-support branch June 10, 2015 19:58

nulltoken mentioned this pull request Jun 10, 2015

Register custom filters #854

Closed

5 tasks

nulltoken mentioned this pull request Jun 12, 2015

Assess streaming filter support behavior in a memory constrained environment #1091

Closed

carlosmn mentioned this pull request Jun 19, 2015

WriteStream: use libgit2's error when we fail to write to the next stream #1107

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Streaming Filter Support #1030

Streaming Filter Support #1030

whoisj commented Apr 23, 2015

nulltoken Apr 24, 2015

nulltoken commented Apr 24, 2015

ethomson commented Apr 24, 2015

jamill Apr 24, 2015

whoisj Apr 24, 2015

shiftkey commented Apr 24, 2015

whoisj commented Apr 24, 2015

whoisj commented Apr 24, 2015

jamill commented Apr 24, 2015

ammeep commented Apr 25, 2015

whoisj commented Apr 25, 2015

whoisj commented Apr 27, 2015

shiftkey commented Apr 27, 2015

nulltoken commented Jun 10, 2015

whoisj commented Jun 10, 2015

nulltoken commented Jun 10, 2015

whoisj commented Jun 10, 2015

dahlbyk commented Jun 10, 2015

ammeep commented Jun 10, 2015

shiftkey commented Jun 10, 2015

whoisj commented Jun 10, 2015

nulltoken commented Jun 11, 2015

nulltoken commented Jun 11, 2015

whoisj commented Jun 11, 2015

nulltoken commented Jun 11, 2015

nulltoken commented Jun 11, 2015

whoisj commented Jun 11, 2015

nulltoken commented Jun 11, 2015

whoisj commented Jun 11, 2015

nulltoken commented Jun 11, 2015

dahlbyk commented Jun 18, 2015

carlosmn commented Jun 19, 2015

Streaming Filter Support #1030

Streaming Filter Support #1030

Conversation

whoisj commented Apr 23, 2015

nulltoken Apr 24, 2015

Choose a reason for hiding this comment

nulltoken commented Apr 24, 2015

ethomson commented Apr 24, 2015

jamill Apr 24, 2015

Choose a reason for hiding this comment

whoisj Apr 24, 2015

Choose a reason for hiding this comment

shiftkey commented Apr 24, 2015

whoisj commented Apr 24, 2015

whoisj commented Apr 24, 2015

jamill commented Apr 24, 2015

ammeep commented Apr 25, 2015

whoisj commented Apr 25, 2015

whoisj commented Apr 27, 2015

shiftkey commented Apr 27, 2015

nulltoken commented Jun 10, 2015

whoisj commented Jun 10, 2015

nulltoken commented Jun 10, 2015

whoisj commented Jun 10, 2015

dahlbyk commented Jun 10, 2015

ammeep commented Jun 10, 2015

shiftkey commented Jun 10, 2015

whoisj commented Jun 10, 2015

nulltoken commented Jun 11, 2015

nulltoken commented Jun 11, 2015

whoisj commented Jun 11, 2015

nulltoken commented Jun 11, 2015

nulltoken commented Jun 11, 2015

whoisj commented Jun 11, 2015

nulltoken commented Jun 11, 2015

whoisj commented Jun 11, 2015

nulltoken commented Jun 11, 2015

dahlbyk commented Jun 18, 2015

carlosmn commented Jun 19, 2015