{"thread":{"id":"16421","subject":"[PATCH] mmap implementation for mingw.","startedAt":"2008-11-21T02:50:22Z","lastAt":"2008-11-21T20:51:22Z","messageCount":7,"participants":["Vasyl Vavrychuk","Johannes Sixt","Vasyl' Vavrychuk","Johannes Schindelin","Bryan Donlan"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"96306","messageId":"loom.20081121T024302-370@post.gmane.org","threadId":"16421","inReplyTo":null,"subject":"[PATCH] mmap implementation for mingw.","fromName":"Vasyl Vavrychuk","fromEmail":"vvavrychuk@gmail.com","sentAt":"2008-11-21T02:50:22Z","receivedAt":"2008-11-21T02:50:22Z","isPatch":true,"sender":{"key":"vvavrychuk@gmail.com","avatar":"https://avatars.githubusercontent.com/u/152563?v=4"},"body":"Here is simple and restricted implementation of mmap using CreateFileMapping, \nMapViewOfFile.\n\n---\n compat/mingw.c    |   27 +++++++++++++++++++++++++++\n compat/mingw.h    |    6 ++++++\n git-compat-util.h |    2 ++\n 3 files changed, 35 insertions(+), 0 deletions(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex b534a8a..a6a5081 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -994,3 +994,30 @@ void mingw_open_html(const char *unixpath)\n \tprintf(\"Launching default browser to display HTML ...\\n\");\n \tShellExecute(NULL, \"open\", htmlpath, NULL, \"\\\\\", 0);\n }\n+\n+void *mingw_mmap(void *start, size_t length, int prot, int flags, int fd, \noff_t offset)\n+{\n+\tHANDLE handle;\n+\n+\tif (start != NULL || !(flags & MAP_PRIVATE))\n+\t\tdie(\"Invalid usage of mingw_mmap\");\n+\n+\tif (offset % getpagesize() != 0)\n+\t\tdie(\"Offset does not match the memory allocation granularity\");\n+\n+\thandle = CreateFileMapping((HANDLE)_get_osfhandle(fd), NULL, \nPAGE_WRITECOPY,\n+\t\t\t0, 0, NULL);\n+\n+\tif (handle != NULL) {\n+\t\tstart = MapViewOfFile(handle, FILE_MAP_COPY, 0, offset, \nlength);\n+\t\tCloseHandle(handle);\n+\t}\n+\n+\treturn start;\n+}\n+\n+int mingw_munmap(void *start, size_t length)\n+{\n+\tUnmapViewOfFile(start);\n+\treturn 0;\n+}\ndiff --git a/compat/mingw.h b/compat/mingw.h\nindex 4f275cb..1b2a098 100644\n--- a/compat/mingw.h\n+++ b/compat/mingw.h\n@@ -183,6 +183,12 @@ static inline unsigned int git_ntohl(unsigned int x)\n sig_handler_t mingw_signal(int sig, sig_handler_t handler);\n #define signal mingw_signal\n \n+void *mingw_mmap(void *start, size_t length, int prot, int flags, int fd, \noff_t offset);\n+#define mmap mingw_mmap\n+\n+int mingw_munmap(void *start, size_t length);\n+#define munmap mingw_munmap\n+\n /*\n  * ANSI emulation wrappers\n  */\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex e20b1e8..8f3d070 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -175,10 +175,12 @@ static inline const char *skip_prefix(const char *str, \nconst char *prefix)\n #define MAP_FAILED ((void*)-1)\n #endif\n \n+#ifndef __MINGW32__\n #define mmap git_mmap\n #define munmap git_munmap\n extern void *git_mmap(void *start, size_t length, int prot, int flags, int fd, \noff_t offset);\n extern int git_munmap(void *start, size_t length);\n+#endif\n \n /* This value must be multiple of (pagesize * 2) */\n #define DEFAULT_PACKED_GIT_WINDOW_SIZE (1 * 1024 * 1024)\n-- \n1.5.6.1.1071.g76fb\n"},{"id":"96312","messageId":"49266A59.4010404@viscovery.net","threadId":"16421","inReplyTo":"loom.20081121T024302-370@post.gmane.org","subject":"Re: [PATCH] mmap implementation for mingw.","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-11-21T07:59:21Z","receivedAt":"2008-11-21T07:59:21Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Vasyl Vavrychuk schrieb:\n> Here is simple and restricted implementation of mmap using CreateFileMapping, \n> MapViewOfFile.\n\nThanks. Sign-off?\n\nDid you notice any differences with this? Or is this change just\nbecause-we-can?\n\nIt doesn't pass the test suite, for example t5301-sliding-window.sh fails.\n\n> --- a/compat/mingw.c\n> +++ b/compat/mingw.c\n> @@ -994,3 +994,30 @@ void mingw_open_html(const char *unixpath)\n>  \tprintf(\"Launching default browser to display HTML ...\\n\");\n>  \tShellExecute(NULL, \"open\", htmlpath, NULL, \"\\\\\", 0);\n>  }\n> +\n> +void *mingw_mmap(void *start, size_t length, int prot, int flags, int fd, \n> off_t offset)\n> +{\n> +\tHANDLE handle;\n> +\n> +\tif (start != NULL || !(flags & MAP_PRIVATE))\n> +\t\tdie(\"Invalid usage of mingw_mmap\");\n\nI tend to use this idiom:\n\n\t\treturn errno = EINVAL,\n\t\t\t error(\"Invalid usage of mingw_mmap\");\n\n> +\tif (offset % getpagesize() != 0)\n> +\t\tdie(\"Offset does not match the memory allocation granularity\");\n\nThis is dangerous. Because on MinGW getpagesize() is hard-coded to 0x1000.\ngetpagesize() does not consult GetSystemInfo(). Just skip the check;\nMapViewOfFile() will report the error later anyway. Or better carefully\ncompute a suitable offset and adjust the length accordingly.\n\n> +\n> +\thandle = CreateFileMapping((HANDLE)_get_osfhandle(fd), NULL, \n> PAGE_WRITECOPY,\n> +\t\t\t0, 0, NULL);\n> +\n> +\tif (handle != NULL) {\n> +\t\tstart = MapViewOfFile(handle, FILE_MAP_COPY, 0, offset, \n> length);\n> +\t\tCloseHandle(handle);\n> +\t}\n> +\n> +\treturn start;\n\nUpon failure you should return MAP_FAILED, not NULL.\n\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -175,10 +175,12 @@ static inline const char *skip_prefix(const char *str, \n> const char *prefix)\n>  #define MAP_FAILED ((void*)-1)\n>  #endif\n>  \n> +#ifndef __MINGW32__\n>  #define mmap git_mmap\n>  #define munmap git_munmap\n>  extern void *git_mmap(void *start, size_t length, int prot, int flags, int fd, \n> off_t offset);\n>  extern int git_munmap(void *start, size_t length);\n> +#endif\n>  \n>  /* This value must be multiple of (pagesize * 2) */\n>  #define DEFAULT_PACKED_GIT_WINDOW_SIZE (1 * 1024 * 1024)\n\nThis is inside #ifdef NO_MMAP ... #else section. Isn't that a bit strange?\nI.e. we say NO_MMAP, but then we do have mmap() now?\n\n-- Hannes\n"},{"id":"96314","messageId":"gg5t5s$qc8$1@ger.gmane.org","threadId":"16421","inReplyTo":"49266A59.4010404@viscovery.net","subject":"Re: [PATCH] mmap implementation for mingw.","fromName":"Vasyl' Vavrychuk","fromEmail":"vvavrychuk@gmail.com","sentAt":"2008-11-21T08:57:26Z","receivedAt":"2008-11-21T08:57:26Z","isPatch":true,"sender":{"key":"vvavrychuk@gmail.com","avatar":"https://avatars.githubusercontent.com/u/152563?v=4"},"body":"Hello Johannes,\n\nThank you for your comments. It's my first patch on git so please be kind:)\nAt the weekend I hope I will find time to work on this.\n\n > Vasyl Vavrychuk schrieb:\n >\n >> Here is simple and restricted implementation of mmap using\n >> CreateFileMapping, MapViewOfFile.\n >>\n > Thanks. Sign-off?\nWill be.\n\n > Did you notice any differences with this? Or is this change just\n > because-we-can?\nNot yet.\n\n > It doesn't pass the test suite, for example t5301-sliding-window.sh\n > fails.\nI will investigate.\n\n >\n >> --- a/compat/mingw.c\n >> +++ b/compat/mingw.c\n >> @@ -994,3 +994,30 @@ void mingw_open_html(const char *unixpath)\n >> printf(\"Launching default browser to display HTML ...\\n\");\n >> ShellExecute(NULL, \"open\", htmlpath, NULL, \"\\\\\", 0);\n >> }\n >> +\n >> +void *mingw_mmap(void *start, size_t length, int prot, int flags,\n >> int fd,\n >> off_t offset)\n >> +{\n >> +\tHANDLE handle;\n >> +\n >> +\tif (start != NULL || !(flags & MAP_PRIVATE))\n >> +\t\tdie(\"Invalid usage of mingw_mmap\");\n > I tend to use this idiom:\n >\n > return errno = EINVAL,\n > error(\"Invalid usage of mingw_mmap\");\nInteresting.\n\n >> +\tif (offset % getpagesize() != 0)\n >> +\t\tdie(\"Offset does not match the memory allocation granularity\");\n > This is dangerous. Because on MinGW getpagesize() is hard-coded to\n > 0x1000. getpagesize() does not consult GetSystemInfo(). Just skip the\n > check; MapViewOfFile() will report the error later anyway. Or better\n > carefully compute a suitable offset and adjust the length accordingly.\nI haven't known about hardcoded getpagesize(). Ajusting will be good but \nit will not solve problem with hardcoded\ngetpagesize(). Maybe write own getpagesize() based on GetSystemInfo()?\n\n >\n >> +\n >> +\thandle = CreateFileMapping((HANDLE)_get_osfhandle(fd), NULL,\n >> PAGE_WRITECOPY,\n >> +\t\t\t0, 0, NULL);\n >> +\n >> +\tif (handle != NULL) {\n >> +\t\tstart = MapViewOfFile(handle, FILE_MAP_COPY, 0, offset,\n >> length);\n >> +\t\tCloseHandle(handle);\n >> +\t}\n >> +\n >> +\treturn start;\n > Upon failure you should return MAP_FAILED, not NULL.\nOK\n\n >> --- a/git-compat-util.h\n >> +++ b/git-compat-util.h\n >> @@ -175,10 +175,12 @@ static inline const char *skip_prefix(const\n >> char *str,\n >> const char *prefix)\n >> #define MAP_FAILED ((void*)-1)\n >> #endif\n >> +#ifndef __MINGW32__\n >> #define mmap git_mmap\n >> #define munmap git_munmap\n >> extern void *git_mmap(void *start, size_t length, int prot, int\n >> flags, int fd,\n >> off_t offset);\n >> extern int git_munmap(void *start, size_t length);\n >> +#endif\n >> /* This value must be multiple of (pagesize * 2) */ #define\n >> DEFAULT_PACKED_GIT_WINDOW_SIZE (1 * 1024 * 1024)\n >>\n > This is inside #ifdef NO_MMAP ... #else section. Isn't that a bit\n > strange? I.e. we say NO_MMAP, but then we do have mmap() now?\n >\nAgree\n"},{"id":"96316","messageId":"492682E9.3030302@viscovery.net","threadId":"16421","inReplyTo":"gg5t5s$qc8$1@ger.gmane.org","subject":"Re: [PATCH] mmap implementation for mingw.","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-11-21T09:44:09Z","receivedAt":"2008-11-21T09:44:09Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Please \"reply to all\" to keep the Cc list.\n\nVasyl' Vavrychuk schrieb:\n>> Did you notice any differences with this? Or is this change just\n>> because-we-can?\n> Not yet.\n\nThis patch makes sense only if there is a noticable speed improvement. I\ndoubt that the gain is measurable in the warm-cache case, but in the\ncold-cache case we can perhaps indeed save a number of disk accesses if\nwhole pack files need not be slurped in (which the current mmap\nreplacements do).\n\n> Maybe write own getpagesize() based on GetSystemInfo()?\n\nIt's probably worth it (but only if mmap() makes sense at all).\ngetpagesize() is used in the \"sliding window\" code to access pack files,\nand then the offset is non-zero and must be aligned properly. Make this a\nseparate commit.\n\n-- Hannes\n"},{"id":"96320","messageId":"alpine.DEB.1.00.0811211143550.30769@pacific.mpi-cbg.de","threadId":"16421","inReplyTo":"gg5t5s$qc8$1@ger.gmane.org","subject":"Re: [PATCH] mmap implementation for mingw.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-11-21T10:48:42Z","receivedAt":"2008-11-21T10:48:42Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\n[re-Cc:ed j6t]\n\nOn Fri, 21 Nov 2008, Vasyl' Vavrychuk wrote:\n\n> [Dscho knows that j6t wrote this:]\n> \n> > It doesn't pass the test suite, for example t5301-sliding-window.sh \n> > fails.\n>\n> I will investigate.\n\nNote that I quickly discarded the idea of mmap() on MinGW because at least \nin the past, we used to rename files that were mmap()ed.  That is a no-go \nwith CreateFile().\n\nAnd I'd really like to see speed comparisons; AFAIR we had major issues \nwith mmap() performance on MacOSX, and use lseek() && pread() for \nthat particular code instead now.\n\nNote that I find your work valuable, even if it should turn out that \nCreateFile() is slower, because at least we will know.\n\nThanks,\nDscho\n"},{"id":"96352","messageId":"3e8340490811211225g6e479dcbyeeb793eb0f825441@mail.gmail.com","threadId":"16421","inReplyTo":"alpine.DEB.1.00.0811211143550.30769@pacific.mpi-cbg.de","subject":"Re: [PATCH] mmap implementation for mingw.","fromName":"Bryan Donlan","fromEmail":"bdonlan@gmail.com","sentAt":"2008-11-21T20:25:58Z","receivedAt":"2008-11-21T20:25:58Z","isPatch":true,"sender":{"key":"bdonlan@gmail.com","avatar":null},"body":"On Fri, Nov 21, 2008 at 5:48 AM, Johannes Schindelin\n<Johannes.Schindelin@gmx.de> wrote:\n> Hi,\n>\n> [re-Cc:ed j6t]\n>\n> On Fri, 21 Nov 2008, Vasyl' Vavrychuk wrote:\n>\n>> [Dscho knows that j6t wrote this:]\n>>\n>> > It doesn't pass the test suite, for example t5301-sliding-window.sh\n>> > fails.\n>>\n>> I will investigate.\n>\n> Note that I quickly discarded the idea of mmap() on MinGW because at least\n> in the past, we used to rename files that were mmap()ed.  That is a no-go\n> with CreateFile().\n\nHi,\n\nI'm not overly familiar with the windows API, but wouldn't passing\nFILE_SHARE_DELETE | FILE_SHARE_READ in the dwShareMode argument of\nCreateFile() be enough to allow rename/deletion of the file in\nquestion while it is mapped?\n\nThanks,\n\nBryan Donlan\n"},{"id":"96353","messageId":"alpine.DEB.1.00.0811212148550.30769@pacific.mpi-cbg.de","threadId":"16421","inReplyTo":"3e8340490811211225g6e479dcbyeeb793eb0f825441@mail.gmail.com","subject":"Re: [PATCH] mmap implementation for mingw.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-11-21T20:51:22Z","receivedAt":"2008-11-21T20:51:22Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 21 Nov 2008, Bryan Donlan wrote:\n\n> I'm not overly familiar with the windows API, but wouldn't passing \n> FILE_SHARE_DELETE | FILE_SHARE_READ in the dwShareMode argument of \n> CreateFile() be enough to allow rename/deletion of the file in question \n> while it is mapped?\n\nA very quick Google research already revealed that your suggestion has any \nmeaning _only_ for NTFS.\n\nSo that leaves FAT behind.\n\nI could also imagine that Windows 2K does not even bother heeding those \nflags (it speaks volumes about Microsoft's documentation that you cannot \nfind out if that is the case spending less than 15 minutes, and possibly \nmore than that).\n\nSo I think your objection is irrelevant, sorry,\nDscho\n"}]}