Nerdbank.GitVersioning: IOException when building from mapping pack files into memory

Seems to be working well for us on all platforms, 👍🏻 Thanks again

This is now available in https://github.com/dotnet/Nerdbank.GitVersioning/releases/tag/v3.4.228

Thanks @AArnott , sorry I didn’t get around to give feedback in time. Will get on the new version tomorrow and give it a whirl.

Guessing that’s a limitation of a 32-bit process not being able to spawn a 64-bit one, but this certainly isn’t my area of expertise.

There’s never a problem with 32-bit processes spawning 64-bit ones. It’s just that that’s not what msbuild in VS was designed to do.

@benmccallum If you have the time to dig into this, could you give https://github.com/dotnet/Nerdbank.GitVersioning/pull/626 a try and let us know if this fixes the problem for you? There may be a perf regression n macOS, but since you mention VS, I guess you’re on Windows so you should be good.

Alternatively, set the NBGV_GitEngine environment variable to LibGit2 to sidestep the managed implementation altogether.

All help is welcome, so I’d be more than happy if @filipnavara wants to invest some time in this 😄 .

Not sure what is the motivation for MemoryMappedStream compared to the built-in MemoryMappedViewStream

I think the main reason was .Dispose behavior. Pooling streams had a significant impact on performance. There was an attempt to use the built-in version (0c5a573) which was reverted (61e877a, https://github.com/dotnet/Nerdbank.GitVersioning/pull/521#issuecomment-719987210).

In the end I have a feeling that it may be more reasonable to map only the index files and not the pack files.

Yes, that may be the case. If I remember correctly, the stream pooling and usage of memory mapped files was introduced at about the same time. There’s probably a real benefit for index files, since we do a binary search there.

It is possible that the memory mapping doesn’t have much of a perf impact on pack files, and the real gain comes from stream pooling.

There are a couple of perf tests which run as part of CI, so it should be fairly easy to tell whether changes regressed performance or not 😃

Here’s an experiment with mapping only small chunks of the pack file: https://github.com/dotnet/Nerdbank.GitVersioning/compare/master...filipnavara:big-pack?expand=1

The approach is not perfect but it highlights few things:

• There’s unused code for GitPackPooledStream.
• It’s likely going to have performance impact so it may be worth the trouble to choose different strategy for small pack files, big pack files and/or 32-bit/64-bit processes.
• Not sure what is the motivation for MemoryMappedStream compared to the built-in MemoryMappedViewStream. It could be performance or different behaviour when disposing. Either way it doesn’t handle non-aligned offsets. The memory mappings have to be aligned to page size boundaries on the OS level APIs. .NET tries to hide this restriction by aligning the mapped block to nearest smaller offset and then making the window large enough. Then it makes the stream point to the actual offset within the first page of the view.
• Mapping small chunks may turn out to be less efficient in worst case. The mapping is always aligned to OS page size so if we create a view for each individual object then two small adjacent objects will end up with two bigger views. Moreover, to read the object size we have to first map a bit of the file to read the object header. I optimistically start with an 8Kb view, read the header and if the object is bigger, then remap to a bigger view. This is somewhat suboptimal.

In the end I have a feeling that it may be more reasonable to map only the index files and not the pack files. If it’s a problem performance wise it’s probably fine to do that only as a fallback when certain parameters are exceeded (file size threshold, 32-bit process, the mapping fails, etc.). .NET 6 will have better APIs to handle this kind of random reads (https://github.com/dotnet/runtime/pull/53669) but unfortunately that’s still some months away.

Here’s one more data point:

filipnavara@172-4-1-17 emclient % ls -l .git/objects/pack
total 8302608
-r--r--r--  1 filipnavara  staff    14635208 Jun 11 10:00 pack-80c7ee391de74c1da6faf79c8b49c68d8c2dacf6.idx
-r--r--r--  1 filipnavara  staff  4236296121 Jun 11 10:00 pack-80c7ee391de74c1da6faf79c8b49c68d8c2dacf6.pack

Mapping the index files is fine but mapping the whole pack files is definitely problematic.