Re: [PATCH] MinGW: implement mmap

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Mar 18, 2009, 11:18 UTC
Message-ID: <alpine.DEB.1.00.0903181213050.10279@pacific.mpi-cbg.de>
In-Reply-To: <49C0A462.3060902@kdbg.org>

Hi,

On Wed, 18 Mar 2009, Johannes Sixt wrote:

Show 25 quoted lines

> From: Janos Laube <janos.dev@gmail.com>
> Date: Fri, 13 Mar 2009 16:50:45 +0100
> 
> Add USE_WIN32_MMAP which triggers the use of windows' native
> file memory mapping functionality in git_mmap()/git_munmap() functions.
> 
> As git functions currently use mmap with MAP_PRIVATE set only, this
> implementation supports only that mode for now.
> 
> On Windows, offsets for memory mapped files need to match the allocation
> granularity. Take this into account when calculating the packed git-
> windowsize and file offsets. At the moment, the only function which makes
> use of offsets in conjunction with mmap is use_pack() in sha1-file.c.
> 
> Git fast-import's code path tries to map a portion of the temporary
> packfile that exceeds the current filesize, i.e. offset+length is
> greater than the filesize. The NO_MMAP code worked with that since pread()
> just reads the file content until EOF and returns gracefully, while
> MapViewOfFile() aborts the mapping and returns 'Access Denied'.
> Working around that by determining the filesize and adjusting the length
> parameter.
> 
> Signed-off-by: Janos Laube <janos.dev@gmail.com>
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---

In case anybody wonders why this is not integrated into mingw.[ch]: I asked for it, as it is Windows-specific and could benefit Cygwin (or MSVC, should someone attempt that), too.

And I think I missed this style in the patch:

> +void *git_mmap
> +(void *start, size_t length, int prot, int flags, int fd, off_t offset)
> +{

... which I would like to see fixed, of course... :-)

Unfortunately, there is also a long line, which is corrupted due to wrapping:

> +	hmap = CreateFileMapping((HANDLE)_get_osfhandle(fd), 0,
> PAGE_WRITECOPY,
> +		0, 0, 0);

It is easily fixed by moving the PAGE_WRITECOPY to the next line.

These are only style issues, though.

The more important part is: how much does this buy us (or is it more expensive, as we saw one MacOSX at one stage). Janos was nice enough to perform some benchmark tests, and saw a ~40% improvement on rev-list --objects --all on a sizeable project (GCC 'twas, IIRC). Funnily, the numbers got better when reducing the window, up until 1MB, and they got worse when enlarging the window.

So I guess there will be a follow-up patch at some stage which changes the default pack window size on Windows to something smaller than 64MB.

Ciao, Dscho