{"thread":{"id":"18793","subject":"[PATCH] MinGW readdir reimplementation to support d_type","startedAt":"2009-04-08T21:01:47Z","lastAt":"2009-05-08T05:45:17Z","messageCount":6,"participants":["Marius Storm-Olsen","Johannes Sixt","Heiko Voigt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"110871","messageId":"1239224507-5372-1-git-send-email-marius@trolltech.com","threadId":"18793","inReplyTo":null,"subject":"[PATCH] MinGW readdir reimplementation to support d_type","fromName":"Marius Storm-Olsen","fromEmail":"marius@trolltech.com","sentAt":"2009-04-08T21:01:47Z","receivedAt":"2009-04-08T21:01:47Z","isPatch":true,"sender":{"key":"marius@trolltech.com","avatar":"https://gravatar.com/avatar/a40071d8f651862c6ab10bd7996f0ad84d94f06c0399de9e3fa4f06beb390a71?d=mp&s=160"},"body":"The original readdir implementation was fast, but didn't\nsupport the d_type. This means that git would do additional\nlstats for each entry, to figure out if the entry was a\ndirectory or not. This unneedingly slowed down many\noperations, since Windows API provides this information\ndirectly when walking the directories.\n\nBy running this implementation on Moe's repo structure:\n  mkdir bummer && cd bummer; for ((i=0;i<100;i++)); do\n    mkdir $i && pushd $i;\n      for ((j=0;j<1000;j++)); do echo \"$j\" >$j; done;\n    popd;\n  done\n\nWe see the following speedups:\n  git add .\n  -------------------\n  old: 00:00:23(.087)\n  new: 00:00:21(.512) 1.07x\n\n  git status\n  -------------------\n  old: 00:00:03(.306)\n  new: 00:00:01(.684) 1.96x\n\n  git clean -dxf\n  -------------------\n  old: 00:00:01(.918)\n  new: 00:00:00(.295) 6.50x\n\nSigned-off-by: Marius Storm-Olsen <marius@trolltech.com>\n---\n It would be nice if MinGW/Windows people would give this a thorough\n testing to ensure that's it's pristine. It seems fine, and I've not\n stumbled over anything myself.\n\n Of course, if you have status.showUntrackedFiles = no, then you'll\n not get any speedups, since the read_directory_recursive loop is\n never entered. People with a standard setup, however, should\n experience a significant speedup.\n\n compat/mingw.c |   59 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n compat/mingw.h |   28 ++++++++++++++++++++++++++\n 2 files changed, 87 insertions(+), 0 deletions(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 2839d9d..f52de3e 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -1139,3 +1139,62 @@ int link(const char *oldpath, const char *newpath)\n \t}\n \treturn 0;\n }\n+\n+#ifndef NO_MINGW_REPLACE_READDIR\n+/* MinGW readdir implementation to avoid extra lstats for Git */\n+struct mingw_DIR\n+{\n+\tstruct _finddata_t\tdd_dta;\t\t/* disk transfer area for this dir */\n+\tstruct mingw_dirent\tdd_dir;\t\t/* Our own implementation, including d_type */\n+\tlong\t\t\tdd_handle;\t/* _findnext handle */\n+\tint\t\t\tdd_stat; \t/* 0 = next entry to read is first entry, -1 = off the end, positive = 0 based index of next entry */\n+\tchar\t\t\tdd_name[1]; \t/* given path for dir with search pattern (struct is extended) */\n+};\n+\n+struct dirent *mingw_readdir(DIR *dir)\n+{\n+\tWIN32_FIND_DATAA buf;\n+\tHANDLE handle;\n+\tstruct mingw_DIR *mdir = (struct mingw_DIR*)dir;\n+\n+\tif (!dir->dd_handle) {\n+\t\terrno = EBADF; /* No set_errno for mingw */\n+\t\treturn NULL;\n+\t}\n+\n+\tif (dir->dd_handle == (long)INVALID_HANDLE_VALUE && dir->dd_stat == 0)\n+\t{\n+\t\thandle = FindFirstFileA(dir->dd_name, &buf);\n+\t\tDWORD lasterr = GetLastError();\n+\t\tdir->dd_handle = (long)handle;\n+\t\tif (handle == INVALID_HANDLE_VALUE && (lasterr != ERROR_NO_MORE_FILES)) {\n+\t\t\terrno = err_win_to_posix(lasterr);\n+\t\t\treturn NULL;\n+\t\t}\n+\t} else if (dir->dd_handle == (long)INVALID_HANDLE_VALUE) {\n+\t\treturn NULL;\n+\t} else if (!FindNextFileA((HANDLE)dir->dd_handle, &buf)) {\n+\t\tDWORD lasterr = GetLastError();\n+\t\tFindClose((HANDLE)dir->dd_handle);\n+\t\tdir->dd_handle = (long)INVALID_HANDLE_VALUE;\n+\t\t/* POSIX says you shouldn't set errno when readdir can't\n+  \t\t   find any more files; so, if another error we leave it set. */\n+\t\tif (lasterr != ERROR_NO_MORE_FILES)\n+\t\t\terrno = err_win_to_posix(lasterr);\n+\t\treturn NULL;\n+\t}\n+\n+\t/* We get here if `buf' contains valid data.  */\n+\tstrcpy(dir->dd_dir.d_name, buf.cFileName);\n+\t++dir->dd_stat;\n+\n+\t/* Set file type, based on WIN32_FIND_DATA */\n+\tmdir->dd_dir.d_type = 0;\n+\tif (buf.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY)\n+\t\tmdir->dd_dir.d_type |= DT_DIR;\n+\telse\n+\t\tmdir->dd_dir.d_type |= DT_REG;\n+\n+\treturn (struct dirent*)&dir->dd_dir;\n+}\n+#endif // !NO_MINGW_REPLACE_READDIR\ndiff --git a/compat/mingw.h b/compat/mingw.h\nindex 762eb14..104b310 100644\n--- a/compat/mingw.h\n+++ b/compat/mingw.h\n@@ -233,3 +233,31 @@ int main(int argc, const char **argv) \\\n \treturn mingw_main(argc, argv); \\\n } \\\n static int mingw_main(c,v)\n+\n+#ifndef NO_MINGW_REPLACE_READDIR\n+/*\n+ * A replacement of readdir, to ensure that it reads the file type at\n+ * the same time. This avoid extra unneeded lstats in git on MinGW\n+ */\n+#undef DT_UNKNOWN\n+#undef DT_DIR\n+#undef DT_REG\n+#undef DT_LNK\n+#define DT_UNKNOWN\t0\n+#define DT_DIR\t\t1\n+#define DT_REG\t\t2\n+#define DT_LNK\t\t3\n+\n+struct mingw_dirent\n+{\n+\tlong\t\td_ino;\t\t\t/* Always zero. */\n+\tunion {\n+\t\tunsigned short\td_reclen;\t/* Always zero. */\n+\t\tunsigned char   d_type;\t\t/* Reimplementation adds this */\n+\t};\n+\tunsigned short\td_namlen;\t\t/* Length of name in d_name. */\n+\tchar\t\td_name[FILENAME_MAX];\t/* File name. */\n+};\n+#define dirent mingw_dirent\n+#define readdir(x) mingw_readdir(x)\n+#endif // !NO_MINGW_REPLACE_READDIR\n--\n1.6.2.2.472.gf61f7.dirty\n"},{"id":"110955","messageId":"49DE5BDE.9050709@kdbg.org","threadId":"18793","inReplyTo":"1239224507-5372-1-git-send-email-marius@trolltech.com","subject":"Re: [msysGit] [PATCH] MinGW readdir reimplementation to support d_type","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2009-04-09T20:34:38Z","receivedAt":"2009-04-09T20:34:38Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Marius Storm-Olsen schrieb:\n> The original readdir implementation was fast, but didn't\n> support the d_type. This means that git would do additional\n> lstats for each entry, to figure out if the entry was a\n> directory or not. This unneedingly slowed down many\n> operations, since Windows API provides this information\n> directly when walking the directories.\n> \n> By running this implementation on Moe's repo structure:\n>   mkdir bummer && cd bummer; for ((i=0;i<100;i++)); do\n>     mkdir $i && pushd $i;\n>       for ((j=0;j<1000;j++)); do echo \"$j\" >$j; done;\n>     popd;\n>   done\n> \n> We see the following speedups:\n>   git add .\n>   -------------------\n>   old: 00:00:23(.087)\n>   new: 00:00:21(.512) 1.07x\n> \n>   git status\n>   -------------------\n>   old: 00:00:03(.306)\n>   new: 00:00:01(.684) 1.96x\n> \n>   git clean -dxf\n>   -------------------\n>   old: 00:00:01(.918)\n>   new: 00:00:00(.295) 6.50x\n\nWell done!\n\n> +struct mingw_dirent\n> +{\n> +\tlong\t\td_ino;\t\t\t/* Always zero. */\n> +\tunion {\n> +\t\tunsigned short\td_reclen;\t/* Always zero. */\n> +\t\tunsigned char   d_type;\t\t/* Reimplementation adds this */\n> +\t};\n\nVERY sneaky! I was wondering why you could get away without replacing \nopendir and closedir, and why you still defined a replacement mingw_DIR \nthat contains the replacement mingw_dirent, until I noticed this unnamed \nunion.\n\nSince we don't use d_reclen anywhere in the code, wouldn't you get away with\n\n#define d_type d_reclen\n\nunless the type (short vs. char) makes a difference. Or would you say that \ndoing that would be even more sneaky?\n\n> +\tunsigned short\td_namlen;\t\t/* Length of name in d_name. */\n> +\tchar\t\td_name[FILENAME_MAX];\t/* File name. */\n> +};\n> +#define dirent mingw_dirent\n> +#define readdir(x) mingw_readdir(x)\n> +#endif // !NO_MINGW_REPLACE_READDIR\n\n-- Hannes\n"},{"id":"110993","messageId":"49DEFA30.1000101@gmail.com","threadId":"18793","inReplyTo":"49DE5BDE.9050709@kdbg.org","subject":"Re: [PATCH] MinGW readdir reimplementation to support d_type","fromName":"Marius Storm-Olsen","fromEmail":"marius@storm-olsen.com","sentAt":"2009-04-10T07:50:08Z","receivedAt":"2009-04-10T07:50:08Z","isPatch":true,"sender":{"key":"marius@storm-olsen.com","avatar":"https://avatars.githubusercontent.com/u/1500?v=4"},"body":"\nJohannes Sixt said the following on 09.04.2009 22:34:\n> Marius Storm-Olsen schrieb:\n>> +struct mingw_dirent\n>> +{\n>> +\tlong\t\td_ino;\t\t\t/* Always zero. */\n>> +\tunion {\n>> +\t\tunsigned short\td_reclen;\t/* Always zero. */\n>> +\t\tunsigned char   d_type;\t\t/* Reimplementation adds this */\n>> +\t};\n> \n> VERY sneaky! I was wondering why you could get away without replacing\n> opendir and closedir, and why you still defined a replacement\n> mingw_DIR that contains the replacement mingw_dirent, until I noticed\n> this unnamed union.\n> \n> Since we don't use d_reclen anywhere in the code, wouldn't you get\n> away with\n> \n> #define d_type d_reclen\n> \n> unless the type (short vs. char) makes a difference. Or would you say\n> that doing that would be even more sneaky?\n\nI'm sure it could be done just with a define. However, given the \nremaining unused variables, I was wondering about also packing in \npermission bits and file modification time in there, to optimize the \nstatus checking even further. That way, on Windows, we would only need \none 'readdir' pass to check the whole repository, with no lstats \nwhatsoever. So, this was patch was a 'primer' for that, hence the union \nwith a proper uchar for the d_type.\n\nHowever, that would also mean a significant change in the status \nchecking code, as it first lstat's ever file in the index, then uses \nread_directory + lstat's for others. I guess that'll be too big of a \nchange in core code, so the vision is moot?\n\nI'd be ok to just use the define, provided that it compiles cleanly of \ncourse, if the above seems too ambitious. :-) I kinda feel like the \ncurrent code is more clean though :)\n\n--\n.marius\n"},{"id":"111101","messageId":"49E10F5A.9010400@kdbg.org","threadId":"18793","inReplyTo":"49DEFA30.1000101@gmail.com","subject":"Re: [PATCH] MinGW readdir reimplementation to support d_type","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2009-04-11T21:44:58Z","receivedAt":"2009-04-11T21:44:58Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"\nMarius Storm-Olsen schrieb:\n> Johannes Sixt said the following on 09.04.2009 22:34:\n>> Marius Storm-Olsen schrieb:\n>>> +struct mingw_dirent\n>>> +{\n>>> +    long        d_ino;            /* Always zero. */\n>>> +    union {\n>>> +        unsigned short    d_reclen;    /* Always zero. */\n>>> +        unsigned char   d_type;        /* Reimplementation adds this */\n>>> +    };\n>>\n>> VERY sneaky! I was wondering why you could get away without replacing\n>> opendir and closedir, and why you still defined a replacement\n>> mingw_DIR that contains the replacement mingw_dirent, until I noticed\n>> this unnamed union.\n>>\n>> Since we don't use d_reclen anywhere in the code, wouldn't you get\n>> away with\n>>\n>> #define d_type d_reclen\n>>\n>> unless the type (short vs. char) makes a difference. Or would you say\n>> that doing that would be even more sneaky?\n> \n> I'm sure it could be done just with a define. However, given the \n> remaining unused variables, I was wondering about also packing in \n> permission bits and file modification time in there, to optimize the \n> status checking even further. That way, on Windows, we would only need \n> one 'readdir' pass to check the whole repository, with no lstats \n> whatsoever. So, this was patch was a 'primer' for that, hence the union \n> with a proper uchar for the d_type.\n> \n> However, that would also mean a significant change in the status \n> checking code, as it first lstat's ever file in the index, then uses \n> read_directory + lstat's for others. I guess that'll be too big of a \n> change in core code, so the vision is moot?\n> \n> I'd be ok to just use the define, provided that it compiles cleanly of \n> course, if the above seems too ambitious. :-) I kinda feel like the \n> current code is more clean though :)\n\nWith a comment in the commit message, it would have been clear, perhaps.\n\nI'll carry this in my (private) tree for a while with the below squashed \nin to avoid a lot of warnings.\n\n-- Hannes\n\ndiff --git a/compat/mingw.h b/compat/mingw.h\nindex 104b310..16ec76b 100644\n--- a/compat/mingw.h\n+++ b/compat/mingw.h\n@@ -260,4 +260,5 @@ struct mingw_dirent\n  };\n  #define dirent mingw_dirent\n  #define readdir(x) mingw_readdir(x)\n+struct dirent *mingw_readdir(DIR *dir);\n  #endif // !NO_MINGW_REPLACE_READDIR\n"},{"id":"113264","messageId":"20090507212629.GC6751@macbook.lan","threadId":"18793","inReplyTo":"49E10F5A.9010400@kdbg.org","subject":"Re: [PATCH] MinGW readdir reimplementation to support d_type","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2009-05-07T21:26:29Z","receivedAt":"2009-05-07T21:26:29Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Sat, Apr 11, 2009 at 11:44:58PM +0200, Johannes Sixt wrote:\n> With a comment in the commit message, it would have been clear, perhaps.\n>\n> I'll carry this in my (private) tree for a while with the below squashed  \n> in to avoid a lot of warnings.\n\nWhat happened to this patch? Is there any reason it can not be included?\nI can confirm the factor 2 speedup which is very noticeable. Especially\nwhen working with \"git gui\" which does git status very often.\n\ncheers Heiko\n"},{"id":"113305","messageId":"4A03C6ED.5050904@trolltech.com","threadId":"18793","inReplyTo":"20090507212629.GC6751@macbook.lan","subject":"Re: [PATCH] MinGW readdir reimplementation to support d_type","fromName":"Marius Storm-Olsen","fromEmail":"marius@trolltech.com","sentAt":"2009-05-08T05:45:17Z","receivedAt":"2009-05-08T05:45:17Z","isPatch":true,"sender":{"key":"marius@trolltech.com","avatar":"https://gravatar.com/avatar/a40071d8f651862c6ab10bd7996f0ad84d94f06c0399de9e3fa4f06beb390a71?d=mp&s=160"},"body":"Heiko Voigt said the following on 07.05.2009 23:26:\n> On Sat, Apr 11, 2009 at 11:44:58PM +0200, Johannes Sixt wrote:\n>> With a comment in the commit message, it would have been clear,\n>> perhaps.\n>> \n>> I'll carry this in my (private) tree for a while with the below\n>> squashed in to avoid a lot of warnings.\n> \n> What happened to this patch? Is there any reason it can not be\n> included? I can confirm the factor 2 speedup which is very\n> noticeable. Especially when working with \"git gui\" which does git\n> status very often.\n\nMsysgit 1.6.3 (http://code.google.com/p/msysgit/downloads/list) contains \nthis patch ontop of baseline 1.6.3. It hasn't made it into the mainline \nyet though. All in good time :-)\n\n--\n.marius\n"}]}