threads / patch / 16421

patchmmap implementation for mingw.

Subject: [PATCH] mmap implementation for mingw.

## tl;dr

7 messages between Nov 21, 2008 and Nov 21, 2008. Diffs are folded; open one to read it.

replies: 6people: 4as markdown or json

Vasyl Vavrychuk· Nov 21, 2008, 02:50 UTC · lore

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(-)
Show changes to 3 files +35 −0

compat/mingw.c, compat/mingw.h, git-compat-util.h

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
Johannes Sixt· Nov 21, 2008, 07:59 UTC · re: Vasyl Vavrychuk · lore

Re: [PATCH] mmap implementation for mingw.

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.
Show 14 quoted lines
> --- 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.

Show 12 quoted lines
> +
> +	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.
Show 17 quoted lines
> --- 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
Vasyl' Vavrychuk· Nov 21, 2008, 08:57 UTC · re: Johannes Sixt · lore

Re: [PATCH] mmap implementation for mingw.

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
Johannes Sixt· Nov 21, 2008, 09:44 UTC · re: Vasyl' Vavrychuk · lore

Re: [PATCH] mmap implementation for mingw.

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
Johannes Schindelin· Nov 21, 2008, 10:48 UTC · re: Vasyl' Vavrychuk · lore

Re: [PATCH] mmap implementation for mingw.

Hi,
[re-Cc:ed j6t]
On Fri, 21 Nov 2008, Vasyl' Vavrychuk wrote:
Show 6 quoted lines
> [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

Bryan Donlan· Nov 21, 2008, 20:25 UTC · re: Johannes Schindelin · lore

Re: [PATCH] mmap implementation for mingw.

On Fri, Nov 21, 2008 at 5:48 AM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 16 quoted lines
> 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
Johannes Schindelin· Nov 21, 2008, 20:51 UTC · re: Bryan Donlan · lore

Re: [PATCH] mmap implementation for mingw.

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

← back to recent threads