[PATCH] mmap implementation for mingw.

[PATCH] mmap implementation for mingw.

[Vasyl Vavrychuk]

Here is simple and restricted implementation of mmap using CreateFileMapping, 
MapViewOfFile.

---
 compat/mingw.c    |   27 +++++++++++++++++++++++++++
 compat/mingw.h    |    6 ++++++
 git-compat-util.h |    2 ++
 3 files changed, 35 insertions(+), 0 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index b534a8a..a6a5081 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -994,3 +994,30 @@ void mingw_open_html(const char *unixpath)
 	printf("Launching default browser to display HTML ...\n");
 	ShellExecute(NULL, "open", htmlpath, NULL, "\\", 0);
 }
+
+void *mingw_mmap(void *start, size_t length, int prot, int flags, int fd, 
off_t offset)
+{
+	HANDLE handle;
+
+	if (start != NULL || !(flags & MAP_PRIVATE))
+		die("Invalid usage of mingw_mmap");
+
+	if (offset % getpagesize() != 0)
+		die("Offset does not match the memory allocation granularity");
+
+	handle = CreateFileMapping((HANDLE)_get_osfhandle(fd), NULL, 
PAGE_WRITECOPY,
+			0, 0, NULL);
+
+	if (handle != NULL) {
+		start = MapViewOfFile(handle, FILE_MAP_COPY, 0, offset, 
length);
+		CloseHandle(handle);
+	}
+
+	return start;
+}
+
+int mingw_munmap(void *start, size_t length)
+{
+	UnmapViewOfFile(start);
+	return 0;
+}
diff --git a/compat/mingw.h b/compat/mingw.h
index 4f275cb..1b2a098 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -183,6 +183,12 @@ static inline unsigned int git_ntohl(unsigned int x)
 sig_handler_t mingw_signal(int sig, sig_handler_t handler);
 #define signal mingw_signal
 
+void *mingw_mmap(void *start, size_t length, int prot, int flags, int fd, 
off_t offset);
+#define mmap mingw_mmap
+
+int mingw_munmap(void *start, size_t length);
+#define munmap mingw_munmap
+
 /*
  * ANSI emulation wrappers
  */
diff --git a/git-compat-util.h b/git-compat-util.h
index e20b1e8..8f3d070 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -175,10 +175,12 @@ static inline const char *skip_prefix(const char *str, 
const char *prefix)
 #define MAP_FAILED ((void*)-1)
 #endif
 
+#ifndef __MINGW32__
 #define mmap git_mmap
 #define munmap git_munmap
 extern void *git_mmap(void *start, size_t length, int prot, int flags, int fd, 
off_t offset);
 extern int git_munmap(void *start, size_t length);
+#endif
 
 /* This value must be multiple of (pagesize * 2) */
 #define DEFAULT_PACKED_GIT_WINDOW_SIZE (1 * 1024 * 1024)
-- 
1.5.6.1.1071.g76fb

Re: [PATCH] mmap implementation for mingw.

[Johannes Sixt]

Vasyl Vavrychuk schrieb:
> Here is simple and restricted implementation of mmap using CreateFileMapping, 
> MapViewOfFile.

Thanks. Sign-off?

Did you notice any differences with this? Or is this change just
because-we-can?

It doesn't pass the test suite, for example t5301-sliding-window.sh fails.

> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -994,3 +994,30 @@ void mingw_open_html(const char *unixpath)
>  	printf("Launching default browser to display HTML ...\n");
>  	ShellExecute(NULL, "open", htmlpath, NULL, "\\", 0);
>  }
> +
> +void *mingw_mmap(void *start, size_t length, int prot, int flags, int fd, 
> off_t offset)
> +{
> +	HANDLE handle;
> +
> +	if (start != NULL || !(flags & MAP_PRIVATE))
> +		die("Invalid usage of mingw_mmap");

I tend to use this idiom:

		return errno = EINVAL,
			 error("Invalid usage of mingw_mmap");

> +	if (offset % getpagesize() != 0)
> +		die("Offset does not match the memory allocation granularity");

This is dangerous. Because on MinGW getpagesize() is hard-coded to 0x1000.
getpagesize() does not consult GetSystemInfo(). Just skip the check;
MapViewOfFile() will report the error later anyway. Or better carefully
compute a suitable offset and adjust the length accordingly.

> +
> +	handle = CreateFileMapping((HANDLE)_get_osfhandle(fd), NULL, 
> PAGE_WRITECOPY,
> +			0, 0, NULL);
> +
> +	if (handle != NULL) {
> +		start = MapViewOfFile(handle, FILE_MAP_COPY, 0, offset, 
> length);
> +		CloseHandle(handle);
> +	}
> +
> +	return start;

Upon failure you should return MAP_FAILED, not NULL.

> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -175,10 +175,12 @@ static inline const char *skip_prefix(const char *str, 
> const char *prefix)
>  #define MAP_FAILED ((void*)-1)
>  #endif
>  
> +#ifndef __MINGW32__
>  #define mmap git_mmap
>  #define munmap git_munmap
>  extern void *git_mmap(void *start, size_t length, int prot, int flags, int fd, 
> off_t offset);
>  extern int git_munmap(void *start, size_t length);
> +#endif
>  
>  /* This value must be multiple of (pagesize * 2) */
>  #define DEFAULT_PACKED_GIT_WINDOW_SIZE (1 * 1024 * 1024)

This is inside #ifdef NO_MMAP ... #else section. Isn't that a bit strange?
I.e. we say NO_MMAP, but then we do have mmap() now?

-- Hannes

Re: [PATCH] mmap implementation for mingw.

[Vasyl' Vavrychuk]

Hello Johannes,

Thank you for your comments. It's my first patch on git so please be kind:)
At the weekend I hope I will find time to work on this.

 > Vasyl Vavrychuk schrieb:
 >
 >> Here is simple and restricted implementation of mmap using
 >> CreateFileMapping, MapViewOfFile.
 >>
 > Thanks. Sign-off?
Will be.

 > Did you notice any differences with this? Or is this change just
 > because-we-can?
Not yet.

 > It doesn't pass the test suite, for example t5301-sliding-window.sh
 > fails.
I will investigate.

 >
 >> --- a/compat/mingw.c
 >> +++ b/compat/mingw.c
 >> @@ -994,3 +994,30 @@ void mingw_open_html(const char *unixpath)
 >> printf("Launching default browser to display HTML ...\n");
 >> ShellExecute(NULL, "open", htmlpath, NULL, "\\", 0);
 >> }
 >> +
 >> +void *mingw_mmap(void *start, size_t length, int prot, int flags,
 >> int fd,
 >> off_t offset)
 >> +{
 >> +	HANDLE handle;
 >> +
 >> +	if (start != NULL || !(flags & MAP_PRIVATE))
 >> +		die("Invalid usage of mingw_mmap");
 > I tend to use this idiom:
 >
 > return errno = EINVAL,
 > error("Invalid usage of mingw_mmap");
Interesting.

 >> +	if (offset % getpagesize() != 0)
 >> +		die("Offset does not match the memory allocation granularity");
 > This is dangerous. Because on MinGW getpagesize() is hard-coded to
 > 0x1000. getpagesize() does not consult GetSystemInfo(). Just skip the
 > check; MapViewOfFile() will report the error later anyway. Or better
 > carefully compute a suitable offset and adjust the length accordingly.
I haven't known about hardcoded getpagesize(). Ajusting will be good but 
it will not solve problem with hardcoded
getpagesize(). Maybe write own getpagesize() based on GetSystemInfo()?

 >
 >> +
 >> +	handle = CreateFileMapping((HANDLE)_get_osfhandle(fd), NULL,
 >> PAGE_WRITECOPY,
 >> +			0, 0, NULL);
 >> +
 >> +	if (handle != NULL) {
 >> +		start = MapViewOfFile(handle, FILE_MAP_COPY, 0, offset,
 >> length);
 >> +		CloseHandle(handle);
 >> +	}
 >> +
 >> +	return start;
 > Upon failure you should return MAP_FAILED, not NULL.
OK

 >> --- a/git-compat-util.h
 >> +++ b/git-compat-util.h
 >> @@ -175,10 +175,12 @@ static inline const char *skip_prefix(const
 >> char *str,
 >> const char *prefix)
 >> #define MAP_FAILED ((void*)-1)
 >> #endif
 >> +#ifndef __MINGW32__
 >> #define mmap git_mmap
 >> #define munmap git_munmap
 >> extern void *git_mmap(void *start, size_t length, int prot, int
 >> flags, int fd,
 >> off_t offset);
 >> extern int git_munmap(void *start, size_t length);
 >> +#endif
 >> /* This value must be multiple of (pagesize * 2) */ #define
 >> DEFAULT_PACKED_GIT_WINDOW_SIZE (1 * 1024 * 1024)
 >>
 > This is inside #ifdef NO_MMAP ... #else section. Isn't that a bit
 > strange? I.e. we say NO_MMAP, but then we do have mmap() now?
 >
Agree

Re: [PATCH] mmap implementation for mingw.

[Johannes Sixt]

Please "reply to all" to keep the Cc list.

Vasyl' Vavrychuk schrieb:
>> Did you notice any differences with this? Or is this change just
>> because-we-can?
> Not yet.

This patch makes sense only if there is a noticable speed improvement. I
doubt that the gain is measurable in the warm-cache case, but in the
cold-cache case we can perhaps indeed save a number of disk accesses if
whole pack files need not be slurped in (which the current mmap
replacements do).

> Maybe write own getpagesize() based on GetSystemInfo()?

It's probably worth it (but only if mmap() makes sense at all).
getpagesize() is used in the "sliding window" code to access pack files,
and then the offset is non-zero and must be aligned properly. Make this a
separate commit.

-- Hannes

Re: [PATCH] mmap implementation for mingw.

[Johannes Schindelin]

Hi,

[re-Cc:ed j6t]

On Fri, 21 Nov 2008, Vasyl' Vavrychuk wrote:

> [Dscho knows that j6t wrote this:]
> 
> > It doesn't pass the test suite, for example t5301-sliding-window.sh 
> > fails.
>
> I will investigate.

Note that I quickly discarded the idea of mmap() on MinGW because at least 
in the past, we used to rename files that were mmap()ed.  That is a no-go 
with CreateFile().

And I'd really like to see speed comparisons; AFAIR we had major issues 
with mmap() performance on MacOSX, and use lseek() && pread() for 
that particular code instead now.

Note that I find your work valuable, even if it should turn out that 
CreateFile() is slower, because at least we will know.

Thanks,
Dscho

Re: [PATCH] mmap implementation for mingw.

[Bryan Donlan]

On Fri, Nov 21, 2008 at 5:48 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> [re-Cc:ed j6t]
>
> On Fri, 21 Nov 2008, Vasyl' Vavrychuk wrote:
>
>> [Dscho knows that j6t wrote this:]
>>
>> > It doesn't pass the test suite, for example t5301-sliding-window.sh
>> > fails.
>>
>> I will investigate.
>
> Note that I quickly discarded the idea of mmap() on MinGW because at least
> in the past, we used to rename files that were mmap()ed.  That is a no-go
> with CreateFile().

Hi,

I'm not overly familiar with the windows API, but wouldn't passing
FILE_SHARE_DELETE | FILE_SHARE_READ in the dwShareMode argument of
CreateFile() be enough to allow rename/deletion of the file in
question while it is mapped?

Thanks,

Bryan Donlan

Re: [PATCH] mmap implementation for mingw.

[Johannes Schindelin]

Hi,

On Fri, 21 Nov 2008, Bryan Donlan wrote:

> I'm not overly familiar with the windows API, but wouldn't passing 
> FILE_SHARE_DELETE | FILE_SHARE_READ in the dwShareMode argument of 
> CreateFile() be enough to allow rename/deletion of the file in question 
> while it is mapped?

A very quick Google research already revealed that your suggestion has any 
meaning _only_ for NTFS.

So that leaves FAT behind.

I could also imagine that Windows 2K does not even bother heeding those 
flags (it speaks volumes about Microsoft's documentation that you cannot 
find out if that is the case spending less than 15 minutes, and possibly 
more than that).

So I think your objection is irrelevant, sorry,
Dscho