{"thread":{"id":"35846","subject":"Make the git codebase thread-safe","startedAt":"2014-02-12T01:54:51Z","lastAt":"2019-04-02T19:07:11Z","messageCount":55,"participants":["Stefan Zager","Robin H. Johnson","Duy Nguyen","Karsten Blees","Erik Faye-Lund","Matthieu Moy","David Kastrup","Junio C Hamano","Mike Hommey","brian m. carlson","Johannes Sixt","Zachary Turner","Matheus Tavares","Matheus Tavares Bernardino"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"234633","messageId":"CA+TurHgyUK5sfCKrK+3xY8AeOg0t66vEvFxX=JiA9wXww7eZXQ@mail.gmail.com","threadId":"35846","inReplyTo":null,"subject":"Make the git codebase thread-safe","fromName":"Stefan Zager","fromEmail":"szager@chromium.org","sentAt":"2014-02-12T01:54:51Z","receivedAt":"2014-02-12T01:54:51Z","isPatch":false,"sender":{"key":"szager@chromium.org","avatar":null},"body":"We in the chromium project have a keen interest in adding threading to\ngit in the pursuit of performance for lengthy operations (checkout,\nstatus, blame, ...). Our motivation comes from hitting some\nperformance walls when working with repositories the size of chromium\nand blink:\n\nhttps://chromium.googlesource.com/chromium/src\nhttps://chromium.googlesource.com/chromium/blink\n\nWe are particularly concerned with the performance of msysgit, and we\nhave already chalked up a significant performance gain by turning on\nthe threading code in pack-objects (which was already enabled for\nposix platforms, but not on msysgit, owing to the lack of a correct\npread implementation).\n\nTo this end, I'd like to start submitting patches that make the code\nbase generally more thread-safe and thread-friendly. Right after this\nemail, I'm going to send the first such patch, which makes the global\nlist of pack files (packed_git) internal to sha1_file.c.\n\nI realize this may be a contentious topic, and I'd like to get\nfeedback on the general effort to add more threading to git. I'd\nappreciate any feedback you'd like to give up front.\n\nThanks!\n\nStefan Zager\n"},{"id":"234636","messageId":"robbat2-20140212T015847-248245854Z@orbis-terrarum.net","threadId":"35846","inReplyTo":"CA+TurHgyUK5sfCKrK+3xY8AeOg0t66vEvFxX=JiA9wXww7eZXQ@mail.gmail.com","subject":"Re: Make the git codebase thread-safe","fromName":"Robin H. Johnson","fromEmail":"robbat2@gentoo.org","sentAt":"2014-02-12T02:02:06Z","receivedAt":"2014-02-12T02:02:06Z","isPatch":false,"sender":{"key":"robbat2@gentoo.org","avatar":"https://avatars.githubusercontent.com/u/373898?v=4"},"body":"On Tue, Feb 11, 2014 at 05:54:51PM -0800, Stefan Zager wrote:\n> We in the chromium project have a keen interest in adding threading to\n> git in the pursuit of performance for lengthy operations (checkout,\n> status, blame, ...). Our motivation comes from hitting some\n> performance walls when working with repositories the size of chromium\n> and blink:\n+1 from Gentoo on performance improvements for large repos.\n\nThe main repository in the ongoing Git migration project looks to be in\nthe 1.5GB range (and for those that want to propose splitting it up, we\nhave explored that option and found it lacking), with very deep history\n(but no branches of note, and very few tags).\n\n-- \nRobin Hugh Johnson\nGentoo Linux: Developer, Infrastructure Lead\nE-Mail : robbat2@gentoo.org\nGnuPG FP : 11ACBA4F 4778E3F6 E4EDF38E B27B944E 34884E85\n"},{"id":"234637","messageId":"CACsJy8Bsc6sywL9L5QC-SKKmh9J+CKnoG5i78WfUbAG9BdZ8Rw@mail.gmail.com","threadId":"35846","inReplyTo":"CA+TurHgyUK5sfCKrK+3xY8AeOg0t66vEvFxX=JiA9wXww7eZXQ@mail.gmail.com","subject":"Re: Make the git codebase thread-safe","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-02-12T02:11:59Z","receivedAt":"2014-02-12T02:11:59Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Feb 12, 2014 at 8:54 AM, Stefan Zager wrote:\n> We in the chromium project have a keen interest in adding threading to\n> git in the pursuit of performance for lengthy operations (checkout,\n> status, blame, ...). Our motivation comes from hitting some\n> performance walls when working with repositories the size of chromium\n> and blink:\n>\n> https://chromium.googlesource.com/chromium/src\n> https://chromium.googlesource.com/chromium/blink\n\nI have no comments about thread safety improvements (well, not yet).\nIf you have investigated about git performance on chromium\nrepositories, could you please sum it up? Threading may be an option\nto improve performance, but it's probably not the only option.\n-- \nDuy\n"},{"id":"234638","messageId":"CACsJy8AStrZBtSqRiwBtonfW5y0ar=9Z371fE2Krwj=P-w7ERw@mail.gmail.com","threadId":"35846","inReplyTo":"robbat2-20140212T015847-248245854Z@orbis-terrarum.net","subject":"Re: Make the git codebase thread-safe","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-02-12T03:43:19Z","receivedAt":"2014-02-12T03:43:19Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Feb 12, 2014 at 9:02 AM, Robin H. Johnson wrote:\n> On Tue, Feb 11, 2014 at 05:54:51PM -0800, Stefan Zager wrote:\n>> We in the chromium project have a keen interest in adding threading to\n>> git in the pursuit of performance for lengthy operations (checkout,\n>> status, blame, ...). Our motivation comes from hitting some\n>> performance walls when working with repositories the size of chromium\n>> and blink:\n> +1 from Gentoo on performance improvements for large repos.\n>\n> The main repository in the ongoing Git migration project looks to be in\n> the 1.5GB range (and for those that want to propose splitting it up, we\n> have explored that option and found it lacking), with very deep history\n> (but no branches of note, and very few tags).\n\n>From v1.9 shallow clone should work for all push/pull/clone... so\nhistory depth does not matter (on the client side). As for\ngentoo-x86's large worktree, using index v4 and avoid full-tree\noperations (e.g. \"status .\", not \"status\"..) should make all\noperations reasonably fast. I plan to make \"status\" fast even without\npath limiting with the help of inotify, but that's not going to be\nfinished soon. Did I miss anything else?\n-- \nDuy\n"},{"id":"234650","messageId":"52FB5443.8030200@gmail.com","threadId":"35846","inReplyTo":"CACsJy8AStrZBtSqRiwBtonfW5y0ar=9Z371fE2Krwj=P-w7ERw@mail.gmail.com","subject":"Re: Make the git codebase thread-safe","fromName":"Karsten Blees","fromEmail":"karsten.blees@gmail.com","sentAt":"2014-02-12T11:00:19Z","receivedAt":"2014-02-12T11:00:19Z","isPatch":false,"sender":{"key":"karsten.blees@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1111200?v=4"},"body":"Am 12.02.2014 04:43, schrieb Duy Nguyen:\n> On Wed, Feb 12, 2014 at 9:02 AM, Robin H. Johnson wrote:\n>> On Tue, Feb 11, 2014 at 05:54:51PM -0800, Stefan Zager wrote:\n>>> We in the chromium project have a keen interest in adding threading to\n>>> git in the pursuit of performance for lengthy operations (checkout,\n>>> status, blame, ...). Our motivation comes from hitting some\n>>> performance walls when working with repositories the size of chromium\n>>> and blink:\n>> +1 from Gentoo on performance improvements for large repos.\n>>\n>> The main repository in the ongoing Git migration project looks to be in\n>> the 1.5GB range (and for those that want to propose splitting it up, we\n>> have explored that option and found it lacking), with very deep history\n>> (but no branches of note, and very few tags).\n> \n> From v1.9 shallow clone should work for all push/pull/clone... so\n> history depth does not matter (on the client side). As for\n> gentoo-x86's large worktree, using index v4 and avoid full-tree\n> operations (e.g. \"status .\", not \"status\"..) should make all\n> operations reasonably fast. I plan to make \"status\" fast even without\n> path limiting with the help of inotify, but that's not going to be\n> finished soon. Did I miss anything else?\n> \n\nRegarding git-status on msysgit, enable core.preloadindex and core.fscache (as of 1.8.5.2).\n\nThere's no inotify on Windows, and I gave up using ReadDirectoryChangesW to keep fscache up to date, as it _may_ report DOS file names (e.g. C:\\PROGRA~1 instead of C:\\Program Files).\n"},{"id":"234653","messageId":"CABPQNSZ_LLg5i+mpwUj7pzXVQMY1tcXz2gJ+PWG-mP1iyjxoaw@mail.gmail.com","threadId":"35846","inReplyTo":"CA+TurHgyUK5sfCKrK+3xY8AeOg0t66vEvFxX=JiA9wXww7eZXQ@mail.gmail.com","subject":"Re: Make the git codebase thread-safe","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2014-02-12T11:59:56Z","receivedAt":"2014-02-12T11:59:56Z","isPatch":false,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Wed, Feb 12, 2014 at 2:54 AM, Stefan Zager wrote:\n> We in the chromium project have a keen interest in adding threading to\n> git in the pursuit of performance for lengthy operations (checkout,\n> status, blame, ...). Our motivation comes from hitting some\n> performance walls when working with repositories the size of chromium\n> and blink:\n>\n> https://chromium.googlesource.com/chromium/src\n> https://chromium.googlesource.com/chromium/blink\n>\n> We are particularly concerned with the performance of msysgit, and we\n> have already chalked up a significant performance gain by turning on\n> the threading code in pack-objects (which was already enabled for\n> posix platforms, but not on msysgit, owing to the lack of a correct\n> pread implementation).\n\nHow did you manage to do this? I'm not aware of any way to implement\npread on Windows (without going down the insanity-path of wrapping and\npotentially locking inside every IO operation)...\n"},{"id":"234667","messageId":"CAHOQ7J8gvwpwJV2mBPDaARu3cQ54-ZDQ6iGOwKuJRr9Z+XBL7g@mail.gmail.com","threadId":"35846","inReplyTo":"CACsJy8Bsc6sywL9L5QC-SKKmh9J+CKnoG5i78WfUbAG9BdZ8Rw@mail.gmail.com","subject":"Re: Make the git codebase thread-safe","fromName":"Stefan Zager","fromEmail":"szager@chromium.org","sentAt":"2014-02-12T18:12:20Z","receivedAt":"2014-02-12T18:12:20Z","isPatch":false,"sender":{"key":"szager@chromium.org","avatar":null},"body":"On Tue, Feb 11, 2014 at 6:11 PM, Duy Nguyen wrote:\n>\n> I have no comments about thread safety improvements (well, not yet).\n> If you have investigated about git performance on chromium\n> repositories, could you please sum it up? Threading may be an option\n> to improve performance, but it's probably not the only option.\n\nWell, the painful operations that we use frequently are pack-objects,\ncheckout, status, and blame. Anything on Windows that touches a lot\nof files is miserable due to the usual file system slowness on\nWindows, and luafv.sys (the UAC file virtualization driver) seems to\nmake it much worse.\n\nWith threading turned on, pack-objects on Windows now takes about\ntwice as long as on Linux, which is still more than a 2x improvement\nover the non-threaded operation.\n\nCheckout is really bad on Windows. The blink repository is ~200K\nfiles, and a full clean checkout from the index takes about 10 seconds\non Linux, and about 3:30 on Windows. I used the Very Sleepy profiler\nto see where all the time was spent on Windows: 55% of the time was\nspent in OpenFile, and 25% in CloseFile (both in win32). My immediate\ngoal is to add threading to checkout, so those file system calls can\nbe done in parallel.\n\nEnabling the fscache speeds up status quite a bit. I'm optimistic\nthat parallelizing the stat calls will yield a further improvement.\nBeyond that, it may not be possible to do much more without using a\nfile system watcher daemon, like facebook does with mercurial.\n(https://code.facebook.com/posts/218678814984400/scaling-mercurial-at-facebook/)\n\nBlame is something that chromium and blink developers use heavily, and\nit is not unusual for a blame invocation on the blink repository to\nrun for 30 seconds. It seems like it should be possible to\nparallelize blame, but it requires pack file operations to be\nthread-safe.\n\nStefan\n"},{"id":"234668","messageId":"CAHOQ7J-uHmUgepV+0rDkUgHSQzXJHME2yd8ZOz3bPy0x8k_ejA@mail.gmail.com","threadId":"35846","inReplyTo":"CACsJy8AStrZBtSqRiwBtonfW5y0ar=9Z371fE2Krwj=P-w7ERw@mail.gmail.com","subject":"Re: Make the git codebase thread-safe","fromName":"Stefan Zager","fromEmail":"szager@chromium.org","sentAt":"2014-02-12T18:15:26Z","receivedAt":"2014-02-12T18:15:26Z","isPatch":false,"sender":{"key":"szager@chromium.org","avatar":null},"body":"On Tue, Feb 11, 2014 at 7:43 PM, Duy Nguyen wrote:\n>\n> From v1.9 shallow clone should work for all push/pull/clone... so\n> history depth does not matter (on the client side). As for\n> gentoo-x86's large worktree, using index v4 and avoid full-tree\n> operations (e.g. \"status .\", not \"status\"..) should make all\n> operations reasonably fast. I plan to make \"status\" fast even without\n> path limiting with the help of inotify, but that's not going to be\n> finished soon. Did I miss anything else?\n\nChromium developers frequently want to run status over their entire\ncheckout, and a lot of them run 'git commit -a'. We want to do\neverything possible to speed this up.\n"},{"id":"234669","messageId":"CAHOQ7J8QxfvtrS2KdgzUPvkDzJ1Od0CMvdWxrF_bNacVRYOa5Q@mail.gmail.com","threadId":"35846","inReplyTo":"CABPQNSZ_LLg5i+mpwUj7pzXVQMY1tcXz2gJ+PWG-mP1iyjxoaw@mail.gmail.com","subject":"Re: Make the git codebase thread-safe","fromName":"Stefan Zager","fromEmail":"szager@google.com","sentAt":"2014-02-12T18:20:12Z","receivedAt":"2014-02-12T18:20:12Z","isPatch":false,"sender":{"key":"szager@google.com","avatar":null},"body":"On Wed, Feb 12, 2014 at 3:59 AM, Erik Faye-Lund wrote:\n> On Wed, Feb 12, 2014 at 2:54 AM, Stefan Zager wrote:\n>>\n>> We are particularly concerned with the performance of msysgit, and we\n>> have already chalked up a significant performance gain by turning on\n>> the threading code in pack-objects (which was already enabled for\n>> posix platforms, but not on msysgit, owing to the lack of a correct\n>> pread implementation).\n>\n> How did you manage to do this? I'm not aware of any way to implement\n> pread on Windows (without going down the insanity-path of wrapping and\n> potentially locking inside every IO operation)...\n\nI don't want to steal the thunder of my coworker, who wrote the\nimplementation. He plans to submit it upstream soon-ish. It relies\non using the lpOverlapped argument to ReadFile(), with some additional\ntomfoolery to make sure that the implicit position pointer for the\nfile descriptor doesn't get modified.\n\nStefan\n"},{"id":"234671","messageId":"CABPQNSZtQd51gQY7oK8B-BbpNEhxR-onQtiXSfW9sv1t2YW_nw@mail.gmail.com","threadId":"35846","inReplyTo":"CAHOQ7J8QxfvtrS2KdgzUPvkDzJ1Od0CMvdWxrF_bNacVRYOa5Q@mail.gmail.com","subject":"Re: Make the git codebase thread-safe","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2014-02-12T18:27:25Z","receivedAt":"2014-02-12T18:27:25Z","isPatch":false,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Wed, Feb 12, 2014 at 7:20 PM, Stefan Zager wrote:\n> On Wed, Feb 12, 2014 at 3:59 AM, Erik Faye-Lund wrote:\n>> On Wed, Feb 12, 2014 at 2:54 AM, Stefan Zager wrote:\n>>>\n>>> We are particularly concerned with the performance of msysgit, and we\n>>> have already chalked up a significant performance gain by turning on\n>>> the threading code in pack-objects (which was already enabled for\n>>> posix platforms, but not on msysgit, owing to the lack of a correct\n>>> pread implementation).\n>>\n>> How did you manage to do this? I'm not aware of any way to implement\n>> pread on Windows (without going down the insanity-path of wrapping and\n>> potentially locking inside every IO operation)...\n>\n> I don't want to steal the thunder of my coworker, who wrote the\n> implementation. He plans to submit it upstream soon-ish. It relies\n> on using the lpOverlapped argument to ReadFile(), with some additional\n> tomfoolery to make sure that the implicit position pointer for the\n> file descriptor doesn't get modified.\n\nIs the code available somewhere? I'm especially interested in the\n\"additional tomfoolery to make sure that the implicit position pointer\nfor the file descriptor doesn't get modified\"-part, as this was what I\nended up butting my head into when trying to do this myself.\n"},{"id":"234672","messageId":"vpqtxc49o60.fsf@anie.imag.fr","threadId":"35846","inReplyTo":"CAHOQ7J8gvwpwJV2mBPDaARu3cQ54-ZDQ6iGOwKuJRr9Z+XBL7g@mail.gmail.com","subject":"Re: Make the git codebase thread-safe","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-02-12T18:33:11Z","receivedAt":"2014-02-12T18:33:11Z","isPatch":false,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Stefan Zager writes:\n\n> I'm optimistic that parallelizing the stat calls will yield a further\n> improvement.\n\nIt has already been mentionned in the thread, but in case you overlooked\nit: did you look at core.preloadindex? It seems at least very close to\nwhat you want.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"234673","messageId":"CAHOQ7J_Jrj1NJ_tZaCioskQU_xGR2FQPt8=JrWpR6rfs=c847w@mail.gmail.com","threadId":"35846","inReplyTo":"CABPQNSZtQd51gQY7oK8B-BbpNEhxR-onQtiXSfW9sv1t2YW_nw@mail.gmail.com","subject":"Re: Make the git codebase thread-safe","fromName":"Stefan Zager","fromEmail":"szager@google.com","sentAt":"2014-02-12T18:34:24Z","receivedAt":"2014-02-12T18:34:24Z","isPatch":false,"sender":{"key":"szager@google.com","avatar":null},"body":"On Wed, Feb 12, 2014 at 10:27 AM, Erik Faye-Lund wrote:\n> On Wed, Feb 12, 2014 at 7:20 PM, Stefan Zager wrote:\n>>\n>> I don't want to steal the thunder of my coworker, who wrote the\n>> implementation. He plans to submit it upstream soon-ish. It relies\n>> on using the lpOverlapped argument to ReadFile(), with some additional\n>> tomfoolery to make sure that the implicit position pointer for the\n>> file descriptor doesn't get modified.\n>\n> Is the code available somewhere? I'm especially interested in the\n> \"additional tomfoolery to make sure that the implicit position pointer\n> for the file descriptor doesn't get modified\"-part, as this was what I\n> ended up butting my head into when trying to do this myself.\n\nhttps://chromium-review.googlesource.com/#/c/186104/\n"},{"id":"234674","messageId":"CABPQNSYVGc9m0_xfAWe=3b7CXyGZ-2FfTMRbTJ=UECeZUtdgmg@mail.gmail.com","threadId":"35846","inReplyTo":"CAHOQ7J_Jrj1NJ_tZaCioskQU_xGR2FQPt8=JrWpR6rfs=c847w@mail.gmail.com","subject":"Re: Make the git codebase thread-safe","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2014-02-12T18:37:21Z","receivedAt":"2014-02-12T18:37:21Z","isPatch":false,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Wed, Feb 12, 2014 at 7:34 PM, Stefan Zager wrote:\n> On Wed, Feb 12, 2014 at 10:27 AM, Erik Faye-Lund wrote:\n>> On Wed, Feb 12, 2014 at 7:20 PM, Stefan Zager wrote:\n>>>\n>>> I don't want to steal the thunder of my coworker, who wrote the\n>>> implementation. He plans to submit it upstream soon-ish. It relies\n>>> on using the lpOverlapped argument to ReadFile(), with some additional\n>>> tomfoolery to make sure that the implicit position pointer for the\n>>> file descriptor doesn't get modified.\n>>\n>> Is the code available somewhere? I'm especially interested in the\n>> \"additional tomfoolery to make sure that the implicit position pointer\n>> for the file descriptor doesn't get modified\"-part, as this was what I\n>> ended up butting my head into when trying to do this myself.\n>\n> https://chromium-review.googlesource.com/#/c/186104/\n\nReOpenFile, that's fantastic. Thanks a lot!\n"},{"id":"234675","messageId":"CAHOQ7J9ceOLe9CHbM-LjeSCkQpBihu9UA0V_-7zcRM3GD-_pvA@mail.gmail.com","threadId":"35846","inReplyTo":"vpqtxc49o60.fsf@anie.imag.fr","subject":"Re: Make the git codebase thread-safe","fromName":"Stefan Zager","fromEmail":"szager@chromium.org","sentAt":"2014-02-12T18:39:22Z","receivedAt":"2014-02-12T18:39:22Z","isPatch":false,"sender":{"key":"szager@chromium.org","avatar":null},"body":"On Wed, Feb 12, 2014 at 10:33 AM, Matthieu Moy\n wrote:\n> Stefan Zager writes:\n>\n>> I'm optimistic that parallelizing the stat calls will yield a further\n>> improvement.\n>\n> It has already been mentionned in the thread, but in case you overlooked\n> it: did you look at core.preloadindex? It seems at least very close to\n> what you want.\n\nAh yes, sorry, I overlooked that. We have indeed turned on\ncore.preloadindex, and it does indeed speed up status. That speedup\nis reflected in my previous comments about our observations working\nwith chromium and blink.\n\nStefan\n"},{"id":"234676","messageId":"87y51g88sc.fsf@fencepost.gnu.org","threadId":"35846","inReplyTo":"CAHOQ7J8gvwpwJV2mBPDaARu3cQ54-ZDQ6iGOwKuJRr9Z+XBL7g@mail.gmail.com","subject":"Re: Make the git codebase thread-safe","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2014-02-12T18:50:43Z","receivedAt":"2014-02-12T18:50:43Z","isPatch":false,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"Stefan Zager writes:\n\n> On Tue, Feb 11, 2014 at 6:11 PM, Duy Nguyen wrote:\n>>\n>> I have no comments about thread safety improvements (well, not yet).\n>> If you have investigated about git performance on chromium\n>> repositories, could you please sum it up? Threading may be an option\n>> to improve performance, but it's probably not the only option.\n>\n> Well, the painful operations that we use frequently are pack-objects,\n> checkout, status, and blame.\n\nHave you checked the patch in\n and\nfollowups,\nMessage-ID: <1391454849-26558-1-git-send-email-dak@gnu.org>?\n\nWhile this does not yet support -M and -C options, it's conceivable that\nyou don't use them in your server/scripts.\n\n> Anything on Windows that touches a lot of files is miserable due to\n> the usual file system slowness on Windows, and luafv.sys (the UAC file\n> virtualization driver) seems to make it much worse.\n\nThere is an obvious solution here... Dedicated hardware is not that\nexpensive. Virtualization will always have a price.\n\n> Blame is something that chromium and blink developers use heavily, and\n> it is not unusual for a blame invocation on the blink repository to\n> run for 30 seconds. It seems like it should be possible to\n> parallelize blame, but it requires pack file operations to be\n> thread-safe.\n\nReally, give the above patch a try. I am taking longer to finish it\nthan anticipated (with a lot due to procrastination but that is,\nunfortunately, a large part of my workflow), and it's cutting into my\n\"paychecks\" (voluntary donations which to a good degree depend on timely\nand nontrivial progress reports for my freely available work on GNU\nLilyPond).\n\nNote that it looks like the majority of the remaining time on GNU/Linux\ntends to be spent in system time: I/O time, memory management. And I\nhave an SSD drive. When using packed repositories of considerable size,\ndecompression comes into play as well. I don't think that you can hope\nto get noticeably higher I/O throughput by multithreading, so really,\nreally, really consider dedicated hardware running on a native Linux\nfile system.\n\n-- \nDavid Kastrup\n"},{"id":"234679","messageId":"CAHOQ7J_pg6Nqc5TdU9OA81=d+ZG_JpLFQ5-eFLY3uW8CuAQrUQ@mail.gmail.com","threadId":"35846","inReplyTo":"87y51g88sc.fsf@fencepost.gnu.org","subject":"Re: Make the git codebase thread-safe","fromName":"Stefan Zager","fromEmail":"szager@chromium.org","sentAt":"2014-02-12T19:02:37Z","receivedAt":"2014-02-12T19:02:37Z","isPatch":false,"sender":{"key":"szager@chromium.org","avatar":null},"body":"On Wed, Feb 12, 2014 at 10:50 AM, David Kastrup wrote:\n> Stefan Zager writes:\n>\n>> Anything on Windows that touches a lot of files is miserable due to\n>> the usual file system slowness on Windows, and luafv.sys (the UAC file\n>> virtualization driver) seems to make it much worse.\n>\n> There is an obvious solution here... Dedicated hardware is not that\n> expensive. Virtualization will always have a price.\n\nNot sure I follow you. We need to support people developing,\nbuilding, and testing on natively Windows machines. And we need to\nsupport users with reasonable hardware, including spinning disks. If\nwe were only interested in optimizing for Google employees, each of\nwhom has one or more small nuclear reactors under their desk, this\nwould be easy.\n\n>> Blame is something that chromium and blink developers use heavily, and\n>> it is not unusual for a blame invocation on the blink repository to\n>> run for 30 seconds. It seems like it should be possible to\n>> parallelize blame, but it requires pack file operations to be\n>> thread-safe.\n>\n> Really, give the above patch a try. I am taking longer to finish it\n> than anticipated (with a lot due to procrastination but that is,\n> unfortunately, a large part of my workflow), and it's cutting into my\n> \"paychecks\" (voluntary donations which to a good degree depend on timely\n> and nontrivial progress reports for my freely available work on GNU\n> LilyPond).\n\nI will give that a try. How much of a performance improvement have you clocked?\n\n> Note that it looks like the majority of the remaining time on GNU/Linux\n> tends to be spent in system time: I/O time, memory management. And I\n> have an SSD drive. When using packed repositories of considerable size,\n> decompression comes into play as well. I don't think that you can hope\n> to get noticeably higher I/O throughput by multithreading, so really,\n> really, really consider dedicated hardware running on a native Linux\n> file system.\n\nI have a background in hardware, and I have much more faith in modern\ndisk schedulers :)\n\nStefan\n"},{"id":"234681","messageId":"87ppms87n7.fsf@fencepost.gnu.org","threadId":"35846","inReplyTo":"CAHOQ7J_pg6Nqc5TdU9OA81=d+ZG_JpLFQ5-eFLY3uW8CuAQrUQ@mail.gmail.com","subject":"Re: Make the git codebase thread-safe","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2014-02-12T19:15:24Z","receivedAt":"2014-02-12T19:15:24Z","isPatch":false,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"Stefan Zager writes:\n\n> On Wed, Feb 12, 2014 at 10:50 AM, David Kastrup wrote:\n>\n>> Really, give the above patch a try. I am taking longer to finish it\n>> than anticipated (with a lot due to procrastination but that is,\n>> unfortunately, a large part of my workflow), and it's cutting into my\n>> \"paychecks\" (voluntary donations which to a good degree depend on timely\n>> and nontrivial progress reports for my freely available work on GNU\n>> LilyPond).\n>\n> I will give that a try. How much of a performance improvement have\n> you clocked?\n\nDepends on file type and size. With large files with lots of small\nchanges, performance improvements get more impressive.\n\nSome ugly real-world examples are the Emacs repository, src/xdisp.c\n(performance improvement about a factor of 3), a large file in the style\nof /usr/share/dict/words clocking in at a factor of about 5.\n\nAgain, that's with an SSD and ext4 filesystem on GNU/Linux, and there\nare no improvements in system time (I/O) except for patch 4 of the\nseries which helps perhaps 20% or so.\n\nSo the benefits of the patch will come into play mostly for big, bad\nfiles on Windows: other than that, the I/O time is likely to be the\ndominant player anyway.\n\nIf you have benchmarked the stuff, for annoying cases expect I/O time to\ngo down maybe 10-20%, and user time to drop by a factor of 4. Under\nGNU/Linux, that makes for a significant overall improvement. On\nWindows, the payback is likely quite less because of the worse I/O\nperformance. Pity.\n\n-- \nDavid Kastrup\n"},{"id":"234682","messageId":"52FBC9E5.6010609@gmail.com","threadId":"35846","inReplyTo":"CABPQNSYVGc9m0_xfAWe=3b7CXyGZ-2FfTMRbTJ=UECeZUtdgmg@mail.gmail.com","subject":"Re: Make the git codebase thread-safe","fromName":"Karsten Blees","fromEmail":"karsten.blees@gmail.com","sentAt":"2014-02-12T19:22:13Z","receivedAt":"2014-02-12T19:22:13Z","isPatch":false,"sender":{"key":"karsten.blees@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1111200?v=4"},"body":"Am 12.02.2014 19:37, schrieb Erik Faye-Lund:\n> On Wed, Feb 12, 2014 at 7:34 PM, Stefan Zager wrote:\n>> On Wed, Feb 12, 2014 at 10:27 AM, Erik Faye-Lund wrote:\n>>> On Wed, Feb 12, 2014 at 7:20 PM, Stefan Zager wrote:\n>>>>\n>>>> I don't want to steal the thunder of my coworker, who wrote the\n>>>> implementation. He plans to submit it upstream soon-ish. It relies\n>>>> on using the lpOverlapped argument to ReadFile(), with some additional\n>>>> tomfoolery to make sure that the implicit position pointer for the\n>>>> file descriptor doesn't get modified.\n>>>\n>>> Is the code available somewhere? I'm especially interested in the\n>>> \"additional tomfoolery to make sure that the implicit position pointer\n>>> for the file descriptor doesn't get modified\"-part, as this was what I\n>>> ended up butting my head into when trying to do this myself.\n>>\n>> https://chromium-review.googlesource.com/#/c/186104/\n> \n> ReOpenFile, that's fantastic. Thanks a lot!\n\n...but should be loaded dynamically via GetProcAddress, or are we ready to drop XP support?\n"},{"id":"234684","messageId":"CAHOQ7J9A7zPV-kYe1WiQrVuWXXTNDVOQJEbnB+_jzEQ2_4Umxw@mail.gmail.com","threadId":"35846","inReplyTo":"52FBC9E5.6010609@gmail.com","subject":"Re: Make the git codebase thread-safe","fromName":"Stefan Zager","fromEmail":"szager@google.com","sentAt":"2014-02-12T19:30:29Z","receivedAt":"2014-02-12T19:30:29Z","isPatch":false,"sender":{"key":"szager@google.com","avatar":null},"body":"On Wed, Feb 12, 2014 at 11:22 AM, Karsten Blees wrote:\n> Am 12.02.2014 19:37, schrieb Erik Faye-Lund:\n>> On Wed, Feb 12, 2014 at 7:34 PM, Stefan Zager wrote:\n>>> On Wed, Feb 12, 2014 at 10:27 AM, Erik Faye-Lund wrote:\n>>>> On Wed, Feb 12, 2014 at 7:20 PM, Stefan Zager wrote:\n>>>>>\n>>>>> I don't want to steal the thunder of my coworker, who wrote the\n>>>>> implementation. He plans to submit it upstream soon-ish. It relies\n>>>>> on using the lpOverlapped argument to ReadFile(), with some additional\n>>>>> tomfoolery to make sure that the implicit position pointer for the\n>>>>> file descriptor doesn't get modified.\n>>>>\n>>>> Is the code available somewhere? I'm especially interested in the\n>>>> \"additional tomfoolery to make sure that the implicit position pointer\n>>>> for the file descriptor doesn't get modified\"-part, as this was what I\n>>>> ended up butting my head into when trying to do this myself.\n>>>\n>>> https://chromium-review.googlesource.com/#/c/186104/\n>>\n>> ReOpenFile, that's fantastic. Thanks a lot!\n>\n> ...but should be loaded dynamically via GetProcAddress, or are we ready to drop XP support?\n\nRight, that is an issue. From our perspective, it's well past time to\ndrop XP support.\n"},{"id":"234687","messageId":"xmqqr478m6xx.fsf@gitster.dls.corp.google.com","threadId":"35846","inReplyTo":"CAHOQ7J8gvwpwJV2mBPDaARu3cQ54-ZDQ6iGOwKuJRr9Z+XBL7g@mail.gmail.com","subject":"Re: Make the git codebase thread-safe","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-12T20:06:50Z","receivedAt":"2014-02-12T20:06:50Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Zager writes:\n\n> ... I used the Very Sleepy profiler\n> to see where all the time was spent on Windows: 55% of the time was\n> spent in OpenFile, and 25% in CloseFile (both in win32).\n\nThis is somewhat interesting.\n\nWhen we check things out, checkout_paths() has a list of paths to be\nchecked out, and iterates over them and call checkout_entry().\n\nI wonder if you can:\n\n - introduce a version of checkout_entry() that takes file\n descriptors to write to;\n\n - have an asynchronous helper threads that pre-open the paths to be\n written out and feed to a\n queue;\n\n - restructure that loop so that it reads the from the queue, performs the actual writing out,\n and then feeds to another queue; and\n\n - have another asynchronous helper threads that reads from the queue and close them.\n\nCalls to write (and preparation of data to be written) will then\nremain single-threaded, but it sounds like that codepath is not the\nbottleneck in your measurement, so....\n"},{"id":"234688","messageId":"CAHOQ7J8Q1905pVwx9QVib1BM-Xxg8vTL=hDUjT7garX++VXm3g@mail.gmail.com","threadId":"35846","inReplyTo":"xmqqr478m6xx.fsf@gitster.dls.corp.google.com","subject":"Re: Make the git codebase thread-safe","fromName":"Stefan Zager","fromEmail":"szager@chromium.org","sentAt":"2014-02-12T20:27:50Z","receivedAt":"2014-02-12T20:27:50Z","isPatch":false,"sender":{"key":"szager@chromium.org","avatar":null},"body":"On Wed, Feb 12, 2014 at 12:06 PM, Junio C Hamano wrote:\n> Stefan Zager writes:\n>\n>> ... I used the Very Sleepy profiler\n>> to see where all the time was spent on Windows: 55% of the time was\n>> spent in OpenFile, and 25% in CloseFile (both in win32).\n>\n> This is somewhat interesting.\n>\n> When we check things out, checkout_paths() has a list of paths to be\n> checked out, and iterates over them and call checkout_entry().\n>\n> I wonder if you can:\n>\n> - introduce a version of checkout_entry() that takes file\n> descriptors to write to;\n>\n> - have an asynchronous helper threads that pre-open the paths to be\n> written out and feed to a\n> queue;\n>\n> - restructure that loop so that it reads the to be written> from the queue, performs the actual writing out,\n> and then feeds to another queue; and\n>\n> - have another asynchronous helper threads that reads descriptor to be closed> from the queue and close them.\n>\n> Calls to write (and preparation of data to be written) will then\n> remain single-threaded, but it sounds like that codepath is not the\n> bottleneck in your measurement, so....\n\nYes, I considered that as well. At a minimum, that would still\nrequire attr.c to implement thread locking, since attribute files must\nbe parsed to look for stream filters. I have already done that work.\n\nBut I'm not sure it's the best long-term approach to add convoluted\ncustom threading solutions to each git operation as it appears on the\nperformance radar. I'm hoping to make the entire code base more\nthread-friendly, so that threading can be added in a more natural and\nidiomatic (and less painful) way.\n\nFor example, the most natural way to add threading to checkout would\nbe in the loops over the index in check_updates() in unpack-trees.c.\nIf attr.c and sha1_file.c were thread-safe, then it would be possible\nto thread checkout entirely in check_updates(), with a pretty compact\ncode change. I have already done the work in attr.c; sha1_file.c is\nhairier, but do-able.\n\nStefan\n"},{"id":"234701","messageId":"20140212230338.GA7208@glandium.org","threadId":"35846","inReplyTo":"52FB5443.8030200@gmail.com","subject":"Re: Make the git codebase thread-safe","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2014-02-12T23:03:38Z","receivedAt":"2014-02-12T23:03:38Z","isPatch":false,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Wed, Feb 12, 2014 at 12:00:19PM +0100, Karsten Blees wrote:\n> Am 12.02.2014 04:43, schrieb Duy Nguyen:\n> > On Wed, Feb 12, 2014 at 9:02 AM, Robin H. Johnson wrote:\n> >> On Tue, Feb 11, 2014 at 05:54:51PM -0800, Stefan Zager wrote:\n> >>> We in the chromium project have a keen interest in adding threading to\n> >>> git in the pursuit of performance for lengthy operations (checkout,\n> >>> status, blame, ...). Our motivation comes from hitting some\n> >>> performance walls when working with repositories the size of chromium\n> >>> and blink:\n> >> +1 from Gentoo on performance improvements for large repos.\n> >>\n> >> The main repository in the ongoing Git migration project looks to be in\n> >> the 1.5GB range (and for those that want to propose splitting it up, we\n> >> have explored that option and found it lacking), with very deep history\n> >> (but no branches of note, and very few tags).\n> > \n> > From v1.9 shallow clone should work for all push/pull/clone... so\n> > history depth does not matter (on the client side). As for\n> > gentoo-x86's large worktree, using index v4 and avoid full-tree\n> > operations (e.g. \"status .\", not \"status\"..) should make all\n> > operations reasonably fast. I plan to make \"status\" fast even without\n> > path limiting with the help of inotify, but that's not going to be\n> > finished soon. Did I miss anything else?\n> > \n> \n> Regarding git-status on msysgit, enable core.preloadindex and core.fscache (as of 1.8.5.2).\n> \n> There's no inotify on Windows, and I gave up using ReadDirectoryChangesW to\n> keep fscache up to date, as it _may_ report DOS file names (e.g. C:\\PROGRA~1\n> instead of C:\\Program Files).\n\nYou can use GetLongPathNameW to get the latter from the former.\n\nMike\n"},{"id":"234698","messageId":"CAPc5daVcAq2jb2-R32HVEG_GY4=JZLG-AmgZKNdQMzZZX2LOCg@mail.gmail.com","threadId":"35846","inReplyTo":"CAHOQ7J8Q1905pVwx9QVib1BM-Xxg8vTL=hDUjT7garX++VXm3g@mail.gmail.com","subject":"Re: Make the git codebase thread-safe","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-12T23:05:43Z","receivedAt":"2014-02-12T23:05:43Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Wed, Feb 12, 2014 at 12:27 PM, Stefan Zager wrote:\n> On Wed, Feb 12, 2014 at 12:06 PM, Junio C Hamano wrote:\n>> Stefan Zager writes:\n>>\n>> Calls to write (and preparation of data to be written) will then\n>> remain single-threaded, but it sounds like that codepath is not the\n>> bottleneck in your measurement, so....\n>\n> Yes, I considered that as well. At a minimum, that would still\n> require attr.c to implement thread locking, since attribute files must\n> be parsed to look for stream filters. I have already done that work.\n\nI would have imagined that use of the attribute system belongs to \"write and\npreparation of data to be written\" category, i.e. the single threaded\npart of the\nkludge I outlined.\n\n> But I'm not sure it's the best long-term approach to add convoluted\n> custom threading solutions to each git operation as it appears on the\n> performance radar.\n\nYeah, it depends on how clean and non-intrusive an abstraction we can make.\nThe kludge I outlined is certainly not very pretty.\n"},{"id":"234699","messageId":"20140212230909.GB7208@glandium.org","threadId":"35846","inReplyTo":"87ppms87n7.fsf@fencepost.gnu.org","subject":"Re: Make the git codebase thread-safe","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2014-02-12T23:09:09Z","receivedAt":"2014-02-12T23:09:09Z","isPatch":false,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Wed, Feb 12, 2014 at 08:15:24PM +0100, David Kastrup wrote:\n> Stefan Zager writes:\n> \n> > On Wed, Feb 12, 2014 at 10:50 AM, David Kastrup wrote:\n> >\n> >> Really, give the above patch a try. I am taking longer to finish it\n> >> than anticipated (with a lot due to procrastination but that is,\n> >> unfortunately, a large part of my workflow), and it's cutting into my\n> >> \"paychecks\" (voluntary donations which to a good degree depend on timely\n> >> and nontrivial progress reports for my freely available work on GNU\n> >> LilyPond).\n> >\n> > I will give that a try. How much of a performance improvement have\n> > you clocked?\n> \n> Depends on file type and size. With large files with lots of small\n> changes, performance improvements get more impressive.\n> \n> Some ugly real-world examples are the Emacs repository, src/xdisp.c\n> (performance improvement about a factor of 3), a large file in the style\n> of /usr/share/dict/words clocking in at a factor of about 5.\n> \n> Again, that's with an SSD and ext4 filesystem on GNU/Linux, and there\n> are no improvements in system time (I/O) except for patch 4 of the\n> series which helps perhaps 20% or so.\n> \n> So the benefits of the patch will come into play mostly for big, bad\n> files on Windows: other than that, the I/O time is likely to be the\n> dominant player anyway.\n\nHow much fragmentation does that add to the files, though?\n\nMike\n"},{"id":"234703","messageId":"52FC0C96.6080909@gmail.com","threadId":"35846","inReplyTo":"20140212230338.GA7208@glandium.org","subject":"Re: Make the git codebase thread-safe","fromName":"Karsten Blees","fromEmail":"karsten.blees@gmail.com","sentAt":"2014-02-13T00:06:46Z","receivedAt":"2014-02-13T00:06:46Z","isPatch":false,"sender":{"key":"karsten.blees@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1111200?v=4"},"body":"Am 13.02.2014 00:03, schrieb Mike Hommey:\n> On Wed, Feb 12, 2014 at 12:00:19PM +0100, Karsten Blees wrote:\n>> Am 12.02.2014 04:43, schrieb Duy Nguyen:\n>>> On Wed, Feb 12, 2014 at 9:02 AM, Robin H. Johnson wrote:\n>>>> On Tue, Feb 11, 2014 at 05:54:51PM -0800, Stefan Zager wrote:\n>>>>> We in the chromium project have a keen interest in adding threading to\n>>>>> git in the pursuit of performance for lengthy operations (checkout,\n>>>>> status, blame, ...). Our motivation comes from hitting some\n>>>>> performance walls when working with repositories the size of chromium\n>>>>> and blink:\n>>>> +1 from Gentoo on performance improvements for large repos.\n>>>>\n>>>> The main repository in the ongoing Git migration project looks to be in\n>>>> the 1.5GB range (and for those that want to propose splitting it up, we\n>>>> have explored that option and found it lacking), with very deep history\n>>>> (but no branches of note, and very few tags).\n>>>\n>>> From v1.9 shallow clone should work for all push/pull/clone... so\n>>> history depth does not matter (on the client side). As for\n>>> gentoo-x86's large worktree, using index v4 and avoid full-tree\n>>> operations (e.g. \"status .\", not \"status\"..) should make all\n>>> operations reasonably fast. I plan to make \"status\" fast even without\n>>> path limiting with the help of inotify, but that's not going to be\n>>> finished soon. Did I miss anything else?\n>>>\n>>\n>> Regarding git-status on msysgit, enable core.preloadindex and core.fscache (as of 1.8.5.2).\n>>\n>> There's no inotify on Windows, and I gave up using ReadDirectoryChangesW to\n>> keep fscache up to date, as it _may_ report DOS file names (e.g. C:\\PROGRA~1\n>> instead of C:\\Program Files).\n> \n> You can use GetLongPathNameW to get the latter from the former.\n> \n> Mike\n> \n\nExcept if its a delete or rename notification...my final ReadDirectoryChangesW version cached the files by their long _and_ short names, but was so complex that it slowed most commands down rather than speeding them up :-)\n"},{"id":"234707","messageId":"20140213014229.GE4582@vauxhall.crustytoothpaste.net","threadId":"35846","inReplyTo":"CA+TurHgyUK5sfCKrK+3xY8AeOg0t66vEvFxX=JiA9wXww7eZXQ@mail.gmail.com","subject":"Re: Make the git codebase thread-safe","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2014-02-13T01:42:30Z","receivedAt":"2014-02-13T01:42:30Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Tue, Feb 11, 2014 at 05:54:51PM -0800, Stefan Zager wrote:\n> To this end, I'd like to start submitting patches that make the code\n> base generally more thread-safe and thread-friendly. Right after this\n> email, I'm going to send the first such patch, which makes the global\n> list of pack files (packed_git) internal to sha1_file.c.\n\nI'm definitely interested in this if it also works on POSIX systems. At\nwork, we have a 7.6 GiB repo (packed)[0], so while performance is not\nbad, I certainly wouldn't object if it were better.\n\n[0] Using du -sh. For comparison, the Linux kernel repo is 1.4 GiB.\n\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n"},{"id":"234712","messageId":"87lhxf8s6l.fsf@fencepost.gnu.org","threadId":"35846","inReplyTo":"20140212230909.GB7208@glandium.org","subject":"Re: Make the git codebase thread-safe","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2014-02-13T06:04:02Z","receivedAt":"2014-02-13T06:04:02Z","isPatch":false,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"Mike Hommey writes:\n\n> On Wed, Feb 12, 2014 at 08:15:24PM +0100, David Kastrup wrote:\n>> Stefan Zager writes:\n>> \n>> > On Wed, Feb 12, 2014 at 10:50 AM, David Kastrup wrote:\n>> >\n>> >> Really, give the above patch a try. I am taking longer to finish it\n>> >> than anticipated (with a lot due to procrastination but that is,\n>> >> unfortunately, a large part of my workflow), and it's cutting into my\n>> >> \"paychecks\" (voluntary donations which to a good degree depend on timely\n>> >> and nontrivial progress reports for my freely available work on GNU\n>> >> LilyPond).\n>> >\n>> > I will give that a try. How much of a performance improvement have\n>> > you clocked?\n>> \n>> Depends on file type and size. With large files with lots of small\n>> changes, performance improvements get more impressive.\n>> \n>> Some ugly real-world examples are the Emacs repository, src/xdisp.c\n>> (performance improvement about a factor of 3), a large file in the style\n>> of /usr/share/dict/words clocking in at a factor of about 5.\n>> \n>> Again, that's with an SSD and ext4 filesystem on GNU/Linux, and there\n>> are no improvements in system time (I/O) except for patch 4 of the\n>> series which helps perhaps 20% or so.\n>> \n>> So the benefits of the patch will come into play mostly for big, bad\n>> files on Windows: other than that, the I/O time is likely to be the\n>> dominant player anyway.\n>\n> How much fragmentation does that add to the files, though?\n\nUh, git-blame is a read-only operation. It does not add fragmentation\nto any file. The patch will add a diff of probably a few dozen hunks to\nbuiltin/blame.c. Do you call that \"fragmentation\"? It is small enough\nthat I expect even\n\n git blame builtin/blame.c\n\nto be faster than before. But that interpretation of your question\nprobably tries to make too much sense out of what is just nonsense in\nthe given context.\n\n-- \nDavid Kastrup\n"},{"id":"234711","messageId":"52FC820F.3030707@viscovery.net","threadId":"35846","inReplyTo":"CAHOQ7J9A7zPV-kYe1WiQrVuWXXTNDVOQJEbnB+_jzEQ2_4Umxw@mail.gmail.com","subject":"Re: Make the git codebase thread-safe","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2014-02-13T08:27:59Z","receivedAt":"2014-02-13T08:27:59Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 2/12/2014 20:30, schrieb Stefan Zager:\n> On Wed, Feb 12, 2014 at 11:22 AM, Karsten Blees wrote:\n>> Am 12.02.2014 19:37, schrieb Erik Faye-Lund:\n>>> ReOpenFile, that's fantastic. Thanks a lot!\n>>\n>> ...but should be loaded dynamically via GetProcAddress, or are we ready to drop XP support?\n> \n> Right, that is an issue. From our perspective, it's well past time to\n> drop XP support.\n\nNot from mine.\n\n-- Hannes\n"},{"id":"234713","messageId":"87a9dv8lew.fsf@fencepost.gnu.org","threadId":"35846","inReplyTo":"87ppms87n7.fsf@fencepost.gnu.org","subject":"Re: Make the git codebase thread-safe","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2014-02-13T08:30:15Z","receivedAt":"2014-02-13T08:30:15Z","isPatch":false,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"David Kastrup writes:\n\n> Again, that's with an SSD and ext4 filesystem on GNU/Linux, and there\n> are no improvements in system time (I/O) except for patch 4 of the\n> series which helps perhaps 20% or so.\n>\n> So the benefits of the patch will come into play mostly for big, bad\n> files on Windows: other than that, the I/O time is likely to be the\n> dominant player anyway.\n>\n> If you have benchmarked the stuff, for annoying cases expect I/O time\n> to go down maybe 10-20%, and user time to drop by a factor of 4.\n> Under GNU/Linux, that makes for a significant overall improvement. On\n> Windows, the payback is likely quite less because of the worse I/O\n> performance. Pity.\n\nBut of course, you can significantly reduce the relevant file\nopen/close/search times by running\n\n git gc --aggressive\n\nWhile this does not actually help with performance in GNU/Linux (though\nwith file space), dealing with few but compressed files under Windows is\nlikely a reasonably big win since the uncompression happens in user\nspace and cannot be bungled by Microsoft (apart from bad memory\nmanagement strategies).\n\n-- \nDavid Kastrup\n"},{"id":"234715","messageId":"8761oj8l0l.fsf@fencepost.gnu.org","threadId":"35846","inReplyTo":"52FC820F.3030707@viscovery.net","subject":"Re: Make the git codebase thread-safe","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2014-02-13T08:38:50Z","receivedAt":"2014-02-13T08:38:50Z","isPatch":false,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"Johannes Sixt writes:\n\n> Am 2/12/2014 20:30, schrieb Stefan Zager:\n>> On Wed, Feb 12, 2014 at 11:22 AM, Karsten Blees wrote:\n>>> Am 12.02.2014 19:37, schrieb Erik Faye-Lund:\n>>>> ReOpenFile, that's fantastic. Thanks a lot!\n>>>\n>>> ...but should be loaded dynamically via GetProcAddress, or are we\n>>> ready to drop XP support?\n>> \n>> Right, that is an issue. From our perspective, it's well past time to\n>> drop XP support.\n>\n> Not from mine.\n\nXP has not even reached end of life yet. As a point of comparison,\nthere are tensions on the Emacs developer list several times a decade\nbecause some people suggest it might be time to drop the MSDOS port\nand/or the associated restriction of having filenames be unique in the\n8+3 naming scheme.\n\n-- \nDavid Kastrup\n"},{"id":"234722","messageId":"20140213093439.GA15366@glandium.org","threadId":"35846","inReplyTo":"87lhxf8s6l.fsf@fencepost.gnu.org","subject":"Re: Make the git codebase thread-safe","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2014-02-13T09:34:39Z","receivedAt":"2014-02-13T09:34:39Z","isPatch":false,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Thu, Feb 13, 2014 at 07:04:02AM +0100, David Kastrup wrote:\n> Mike Hommey writes:\n> \n> > On Wed, Feb 12, 2014 at 08:15:24PM +0100, David Kastrup wrote:\n> >> Stefan Zager writes:\n> >> \n> >> > On Wed, Feb 12, 2014 at 10:50 AM, David Kastrup wrote:\n> >> >\n> >> >> Really, give the above patch a try. I am taking longer to finish it\n> >> >> than anticipated (with a lot due to procrastination but that is,\n> >> >> unfortunately, a large part of my workflow), and it's cutting into my\n> >> >> \"paychecks\" (voluntary donations which to a good degree depend on timely\n> >> >> and nontrivial progress reports for my freely available work on GNU\n> >> >> LilyPond).\n> >> >\n> >> > I will give that a try. How much of a performance improvement have\n> >> > you clocked?\n> >> \n> >> Depends on file type and size. With large files with lots of small\n> >> changes, performance improvements get more impressive.\n> >> \n> >> Some ugly real-world examples are the Emacs repository, src/xdisp.c\n> >> (performance improvement about a factor of 3), a large file in the style\n> >> of /usr/share/dict/words clocking in at a factor of about 5.\n> >> \n> >> Again, that's with an SSD and ext4 filesystem on GNU/Linux, and there\n> >> are no improvements in system time (I/O) except for patch 4 of the\n> >> series which helps perhaps 20% or so.\n> >> \n> >> So the benefits of the patch will come into play mostly for big, bad\n> >> files on Windows: other than that, the I/O time is likely to be the\n> >> dominant player anyway.\n> >\n> > How much fragmentation does that add to the files, though?\n> \n> Uh, git-blame is a read-only operation. It does not add fragmentation\n> to any file. The patch will add a diff of probably a few dozen hunks to\n> builtin/blame.c. Do you call that \"fragmentation\"? It is small enough\n> that I expect even\n> \n> git blame builtin/blame.c\n> \n> to be faster than before. But that interpretation of your question\n> probably tries to make too much sense out of what is just nonsense in\n> the given context.\n\nSorry, I thought you were talking about write operations, not reads.\n\nMike\n"},{"id":"234723","messageId":"20140213094846.GB15366@glandium.org","threadId":"35846","inReplyTo":"20140213093439.GA15366@glandium.org","subject":"Re: Make the git codebase thread-safe","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2014-02-13T09:48:46Z","receivedAt":"2014-02-13T09:48:46Z","isPatch":false,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Thu, Feb 13, 2014 at 06:34:39PM +0900, Mike Hommey wrote:\n> On Thu, Feb 13, 2014 at 07:04:02AM +0100, David Kastrup wrote:\n> > Mike Hommey writes:\n> > \n> > > On Wed, Feb 12, 2014 at 08:15:24PM +0100, David Kastrup wrote:\n> > >> Stefan Zager writes:\n> > >> \n> > >> > On Wed, Feb 12, 2014 at 10:50 AM, David Kastrup wrote:\n> > >> >\n> > >> >> Really, give the above patch a try. I am taking longer to finish it\n> > >> >> than anticipated (with a lot due to procrastination but that is,\n> > >> >> unfortunately, a large part of my workflow), and it's cutting into my\n> > >> >> \"paychecks\" (voluntary donations which to a good degree depend on timely\n> > >> >> and nontrivial progress reports for my freely available work on GNU\n> > >> >> LilyPond).\n> > >> >\n> > >> > I will give that a try. How much of a performance improvement have\n> > >> > you clocked?\n> > >> \n> > >> Depends on file type and size. With large files with lots of small\n> > >> changes, performance improvements get more impressive.\n> > >> \n> > >> Some ugly real-world examples are the Emacs repository, src/xdisp.c\n> > >> (performance improvement about a factor of 3), a large file in the style\n> > >> of /usr/share/dict/words clocking in at a factor of about 5.\n> > >> \n> > >> Again, that's with an SSD and ext4 filesystem on GNU/Linux, and there\n> > >> are no improvements in system time (I/O) except for patch 4 of the\n> > >> series which helps perhaps 20% or so.\n> > >> \n> > >> So the benefits of the patch will come into play mostly for big, bad\n> > >> files on Windows: other than that, the I/O time is likely to be the\n> > >> dominant player anyway.\n> > >\n> > > How much fragmentation does that add to the files, though?\n> > \n> > Uh, git-blame is a read-only operation. It does not add fragmentation\n> > to any file. The patch will add a diff of probably a few dozen hunks to\n> > builtin/blame.c. Do you call that \"fragmentation\"? It is small enough\n> > that I expect even\n> > \n> > git blame builtin/blame.c\n> > \n> > to be faster than before. But that interpretation of your question\n> > probably tries to make too much sense out of what is just nonsense in\n> > the given context.\n> \n> Sorry, I thought you were talking about write operations, not reads.\n\nSpecifically, I thought you were talking about git checkout.\n\nMike\n"},{"id":"234738","messageId":"loom.20140213T193220-631@post.gmane.org","threadId":"35846","inReplyTo":"52FBC9E5.6010609@gmail.com","subject":"Re: Make the git codebase thread-safe","fromName":"Zachary Turner","fromEmail":"zturner@chromium.org","sentAt":"2014-02-13T18:38:00Z","receivedAt":"2014-02-13T18:38:00Z","isPatch":false,"sender":{"key":"zturner@chromium.org","avatar":null},"body":"Karsten Blees gmail.com> writes:\n\n> \n> Am 12.02.2014 19:37, schrieb Erik Faye-Lund:\n> > On Wed, Feb 12, 2014 at 7:34 PM, Stefan Zager google.com> \nwrote:\n> >> On Wed, Feb 12, 2014 at 10:27 AM, Erik Faye-Lund \ngmail.com> wrote:\n> >>> On Wed, Feb 12, 2014 at 7:20 PM, Stefan Zager google.com> \nwrote:\n> >>>>\n> >>>> I don't want to steal the thunder of my coworker, who wrote the\n> >>>> implementation. He plans to submit it upstream soon-ish. It relies\n> >>>> on using the lpOverlapped argument to ReadFile(), with some \nadditional\n> >>>> tomfoolery to make sure that the implicit position pointer for the\n> >>>> file descriptor doesn't get modified.\n> >>>\n> >>> Is the code available somewhere? I'm especially interested in the\n> >>> \"additional tomfoolery to make sure that the implicit position pointer\n> >>> for the file descriptor doesn't get modified\"-part, as this was what I\n> >>> ended up butting my head into when trying to do this myself.\n> >>\n> >> https://chromium-review.googlesource.com/#/c/186104/\n> > \n> > ReOpenFile, that's fantastic. Thanks a lot!\n> \n> ...but should be loaded dynamically via GetProcAddress, or are we ready to \ndrop XP support?\n> \n> \n\nOriginal patch author here. In trying to prepare this patch to use \nGetProcAddress to load dynamically, I've run into a bit of a snag. \nNO_THREAD_SAFE_PREAD is a compile-time flag, which will be incompatible with \nany attempt to make this a runtime decision a la LoadLibrary / \nGetProcAddress. On XP, we would need to fallback to the single-threaded \npath, and on Vista+ we would use the thread-able path, and obviously this \ndecision could not be made until runtime.\n\nIf MinGW were the only configuration using NO_THREAD_SAFE_PREAD, I would \njust remove it entirely, but it appears Cygwin configuration uses it also.\n\nSuggestions? \n\nOne possibility is to disallow (by convention, perhaps), the use of pread() \nand read() against the same fd. The only reason ReOpenFile is necessary at \nall is because some code somewhere is mixing read-styles against the same \nfd.\n"},{"id":"234737","messageId":"CAHOQ7J-Fy3mcqx6hmtawBOPCRF9i7hvpZWvbHNVLoxhpG9Dzsw@mail.gmail.com","threadId":"35846","inReplyTo":"52FC820F.3030707@viscovery.net","subject":"Re: Make the git codebase thread-safe","fromName":"Stefan Zager","fromEmail":"szager@google.com","sentAt":"2014-02-13T18:40:06Z","receivedAt":"2014-02-13T18:40:06Z","isPatch":false,"sender":{"key":"szager@google.com","avatar":null},"body":"On Thu, Feb 13, 2014 at 12:27 AM, Johannes Sixt wrote:\n> Am 2/12/2014 20:30, schrieb Stefan Zager:\n>> On Wed, Feb 12, 2014 at 11:22 AM, Karsten Blees wrote:\n>>> Am 12.02.2014 19:37, schrieb Erik Faye-Lund:\n>>>> ReOpenFile, that's fantastic. Thanks a lot!\n>>>\n>>> ...but should be loaded dynamically via GetProcAddress, or are we ready to drop XP support?\n>>\n>> Right, that is an issue. From our perspective, it's well past time to\n>> drop XP support.\n>\n> Not from mine.\n\nAll this really means is that the build config will test WIN_VER, and\nthere will need to be an additional binary distribution of msysgit for\nnewer Windows.\n"},{"id":"234749","messageId":"52FD4C84.7060209@gmail.com","threadId":"35846","inReplyTo":"loom.20140213T193220-631@post.gmane.org","subject":"Re: Make the git codebase thread-safe","fromName":"Karsten Blees","fromEmail":"karsten.blees@gmail.com","sentAt":"2014-02-13T22:51:48Z","receivedAt":"2014-02-13T22:51:48Z","isPatch":false,"sender":{"key":"karsten.blees@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1111200?v=4"},"body":"Am 13.02.2014 19:38, schrieb Zachary Turner:\n\n> The only reason ReOpenFile is necessary at \n> all is because some code somewhere is mixing read-styles against the same \n> fd.\n> \n\nI don't understand...ReadFile with OVERLAPPED parameter doesn't modify the HANDLE's file position, so you should be able to mix read()/pread() however you like (as long as read() is only called from one thread).\n\nI tried without ReOpenFile and it seems to work like a charm, or am I missing something?\n\n----8<----\nssize_t mingw_pread(int fd, void *buf, size_t count, off64_t offset)\n{\n\tDWORD bytes_read;\n\tOVERLAPPED overlapped;\n\tmemset(&overlapped, 0, sizeof(overlapped));\n\toverlapped.Offset = (DWORD) offset;\n\toverlapped.OffsetHigh = (DWORD) (offset >> 32);\n\n\tif (!ReadFile((HANDLE) _get_osfhandle(fd), buf, count, &bytes_read,\n\t &overlapped)) {\n\t\terrno = err_win_to_posix(GetLastError());\n\t\treturn -1;\n\t}\n\treturn (ssize_t) bytes_read;\n}\n"},{"id":"234750","messageId":"CAHOQ7J8syoQLGwwkwPEX3wZir8sWDQ+k8sgHAKn=n_-Q_S8ipA@mail.gmail.com","threadId":"35846","inReplyTo":"52FD4C84.7060209@gmail.com","subject":"Re: Make the git codebase thread-safe","fromName":"Stefan Zager","fromEmail":"szager@google.com","sentAt":"2014-02-13T22:53:31Z","receivedAt":"2014-02-13T22:53:31Z","isPatch":false,"sender":{"key":"szager@google.com","avatar":null},"body":"On Thu, Feb 13, 2014 at 2:51 PM, Karsten Blees wrote:\n> Am 13.02.2014 19:38, schrieb Zachary Turner:\n>\n>> The only reason ReOpenFile is necessary at\n>> all is because some code somewhere is mixing read-styles against the same\n>> fd.\n>>\n>\n> I don't understand...ReadFile with OVERLAPPED parameter doesn't modify the HANDLE's file position, so you should be able to mix read()/pread() however you like (as long as read() is only called from one thread).\n\nThat is, apparently, a bald-faced lie in the ReadFile API doc. First\nimplementation didn't use ReOpenFile, and it crashed all over the\nplace. ReOpenFile fixed it.\n\nStefan\n"},{"id":"234751","messageId":"CAAErz9hzeiJ9f9tJ+Z-kOHvrPqgcZrpvrpBpa_tMjnKm4YWSXA@mail.gmail.com","threadId":"35846","inReplyTo":"CAHOQ7J8syoQLGwwkwPEX3wZir8sWDQ+k8sgHAKn=n_-Q_S8ipA@mail.gmail.com","subject":"Re: Make the git codebase thread-safe","fromName":"Zachary Turner","fromEmail":"zturner@chromium.org","sentAt":"2014-02-13T23:09:41Z","receivedAt":"2014-02-13T23:09:41Z","isPatch":false,"sender":{"key":"zturner@chromium.org","avatar":null},"body":"To elaborate a little bit more, you can verify with a sample program\nthat ReadFile with OVERLAPPED does in fact modify the HANDLE's file\nposition. The documentation doesn't actually state one way or\nanother. My original attempt at a patch didn't have the ReOpenFile,\nand we experienced regular read corruption. We scratched our heads\nover it for a bit, and then hypothesized that someone must be mixing\nread styles, which led to this ReOpenFile workaround, which\nincidentally also solved the corruption problems. We wrote a similar\nsample program to verify that when using ReOpenHandle, and changing\nthe file pointer of the duplicated handle, that the file pointer of\nthe original handle is not modified.\n\nWe did not actually try to identify the source of the mixed read\nstyles, but it seems like the only possible explanation.\n\nOn Thu, Feb 13, 2014 at 2:53 PM, Stefan Zager wrote:\n> On Thu, Feb 13, 2014 at 2:51 PM, Karsten Blees wrote:\n>> Am 13.02.2014 19:38, schrieb Zachary Turner:\n>>\n>>> The only reason ReOpenFile is necessary at\n>>> all is because some code somewhere is mixing read-styles against the same\n>>> fd.\n>>>\n>>\n>> I don't understand...ReadFile with OVERLAPPED parameter doesn't modify the HANDLE's file position, so you should be able to mix read()/pread() however you like (as long as read() is only called from one thread).\n>\n> That is, apparently, a bald-faced lie in the ReadFile API doc. First\n> implementation didn't use ReOpenFile, and it crashed all over the\n> place. ReOpenFile fixed it.\n>\n> Stefan\n"},{"id":"234785","messageId":"52FE68C9.3060403@gmail.com","threadId":"35846","inReplyTo":"CAAErz9hzeiJ9f9tJ+Z-kOHvrPqgcZrpvrpBpa_tMjnKm4YWSXA@mail.gmail.com","subject":"Re: Make the git codebase thread-safe","fromName":"Karsten Blees","fromEmail":"karsten.blees@gmail.com","sentAt":"2014-02-14T19:04:41Z","receivedAt":"2014-02-14T19:04:41Z","isPatch":false,"sender":{"key":"karsten.blees@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1111200?v=4"},"body":"Am 14.02.2014 00:09, schrieb Zachary Turner:\n> To elaborate a little bit more, you can verify with a sample program\n> that ReadFile with OVERLAPPED does in fact modify the HANDLE's file\n> position. The documentation doesn't actually state one way or\n> another. My original attempt at a patch didn't have the ReOpenFile,\n> and we experienced regular read corruption. We scratched our heads\n> over it for a bit, and then hypothesized that someone must be mixing\n> read styles, which led to this ReOpenFile workaround, which\n> incidentally also solved the corruption problems. We wrote a similar\n> sample program to verify that when using ReOpenHandle, and changing\n> the file pointer of the duplicated handle, that the file pointer of\n> the original handle is not modified.\n> \n> We did not actually try to identify the source of the mixed read\n> styles, but it seems like the only possible explanation.\n> \n> On Thu, Feb 13, 2014 at 2:53 PM, Stefan Zager wrote:\n>> On Thu, Feb 13, 2014 at 2:51 PM, Karsten Blees wrote:\n>>> Am 13.02.2014 19:38, schrieb Zachary Turner:\n>>>\n>>>> The only reason ReOpenFile is necessary at\n>>>> all is because some code somewhere is mixing read-styles against the same\n>>>> fd.\n>>>>\n>>>\n>>> I don't understand...ReadFile with OVERLAPPED parameter doesn't modify the HANDLE's file position, so you should be able to mix read()/pread() however you like (as long as read() is only called from one thread).\n>>\n>> That is, apparently, a bald-faced lie in the ReadFile API doc. First\n>> implementation didn't use ReOpenFile, and it crashed all over the\n>> place. ReOpenFile fixed it.\n>>\n>> Stefan\n\nDamn...you're right, multi-threaded git-index-pack works fine, but some tests fail badly. Mixed reads would have to be from git_mmap, which is the only other caller of pread().\n\nA simple alternative to ReOpenHandle is to reset the file pointer to its original position, as in compat/pread.c::git_pread. Thus single-theaded code can mix read()/pread() at will, but multi-threaded code has to use pread() exclusively (which is usually the case anyway). A main thread using read() and background threads using pread() (which is technically allowed by POSIX) will fail with this solution.\n\nThis version passes the test suite on msysgit:\n\n----8<----\nssize_t mingw_pread(int fd, void *buf, size_t count, off64_t offset)\n{\n\tDWORD bytes_read;\n\tOVERLAPPED overlapped;\n\toff64_t current;\n\tmemset(&overlapped, 0, sizeof(overlapped));\n\toverlapped.Offset = (DWORD) offset;\n\toverlapped.OffsetHigh = (DWORD) (offset >> 32);\n\n\tcurrent = lseek64(fd, 0, SEEK_CUR);\n\n\tif (!ReadFile((HANDLE)_get_osfhandle(fd), buf, count, &bytes_read, &overlapped)) {\n\t\terrno = err_win_to_posix(GetLastError());\n\t\treturn -1;\n\t}\n\n\tlseek64(fd, current, SEEK_SET);\n\n\treturn (ssize_t) bytes_read;\n}\n"},{"id":"234788","messageId":"CAAErz9j=_FpWLSyUk43pp8A6e7Ej0crT8ghW5-yxBEbGkd6O+A@mail.gmail.com","threadId":"35846","inReplyTo":"CAAErz9g7ND1htfk=yxRJJLbSEgBi4EV_AHC9uDRptugGWFWcXw@mail.gmail.com","subject":"Re: Make the git codebase thread-safe","fromName":"Zachary Turner","fromEmail":"zturner@chromium.org","sentAt":"2014-02-14T19:16:50Z","receivedAt":"2014-02-14T19:16:50Z","isPatch":false,"sender":{"key":"zturner@chromium.org","avatar":null},"body":"(Gah, sorry if you're receiving multiple emails to your personal\naddresses, I need to get used to manually setting Plain-text mode\nevery time I send a message).\n\nFor the mixed read, we wouldn't be looking for another caller of\npread() (since it doesn't care what the file pointer is), but instead\na caller of read() or lseek() (since those do depend on the current\nfile pointer). In index-pack.c, I see two possible culprits:\n\n1) A call to xread() from inside fill()\n2) A call to lseek in parse_pack_objects()\n\nDo you think these could be related? If so, maybe that opens up some\nother solutions?\n\nBTW, the version you posted isn't thread safe. Suppose thread A and\nthread B execute this function at the same time. A executes through\nthe ReadFile(), but does not yet reset the second lseek64. B then\nexecutes the first lseek64(), storing off the modified file pointer.\nThen A finishes, then B finishes. At the end, the file pointer is\nstill modified.\n\nOn Fri, Feb 14, 2014 at 11:15 AM, Zachary Turner wrote:\n> For the mixed read, we wouldn't be looking for another caller of pread()\n> (since it doesn't care what the file pointer is), but instead a caller of\n> read() or lseek(). In index-pack.c, I see two possible culprits:\n>\n> 1) A call to xread() from inside fill()\n> 2) A call to lseek in parse_pack_objects()\n>\n> Do you think these could be related? If so, maybe that opens up some other\n> solutions?\n>\n> BTW, the version you posted isn't thread safe. Suppose thread A and thread\n> B execute this function at the same time. A executes through the\n> ReadFile(), but does not yet reset the second lseek64. B then executes the\n> first lseek64(), storing off the modified file pointer. Then A finishes,\n> then B finishes. At the end, the file pointer is still modified.\n>\n>\n>\n> On Fri, Feb 14, 2014 at 11:04 AM, Karsten Blees \n> wrote:\n>>\n>> Am 14.02.2014 00:09, schrieb Zachary Turner:\n>> > To elaborate a little bit more, you can verify with a sample program\n>> > that ReadFile with OVERLAPPED does in fact modify the HANDLE's file\n>> > position. The documentation doesn't actually state one way or\n>> > another. My original attempt at a patch didn't have the ReOpenFile,\n>> > and we experienced regular read corruption. We scratched our heads\n>> > over it for a bit, and then hypothesized that someone must be mixing\n>> > read styles, which led to this ReOpenFile workaround, which\n>> > incidentally also solved the corruption problems. We wrote a similar\n>> > sample program to verify that when using ReOpenHandle, and changing\n>> > the file pointer of the duplicated handle, that the file pointer of\n>> > the original handle is not modified.\n>> >\n>> > We did not actually try to identify the source of the mixed read\n>> > styles, but it seems like the only possible explanation.\n>> >\n>> > On Thu, Feb 13, 2014 at 2:53 PM, Stefan Zager wrote:\n>> >> On Thu, Feb 13, 2014 at 2:51 PM, Karsten Blees\n>> >> wrote:\n>> >>> Am 13.02.2014 19:38, schrieb Zachary Turner:\n>> >>>\n>> >>>> The only reason ReOpenFile is necessary at\n>> >>>> all is because some code somewhere is mixing read-styles against the\n>> >>>> same\n>> >>>> fd.\n>> >>>>\n>> >>>\n>> >>> I don't understand...ReadFile with OVERLAPPED parameter doesn't modify\n>> >>> the HANDLE's file position, so you should be able to mix read()/pread()\n>> >>> however you like (as long as read() is only called from one thread).\n>> >>\n>> >> That is, apparently, a bald-faced lie in the ReadFile API doc. First\n>> >> implementation didn't use ReOpenFile, and it crashed all over the\n>> >> place. ReOpenFile fixed it.\n>> >>\n>> >> Stefan\n>>\n>> Damn...you're right, multi-threaded git-index-pack works fine, but some\n>> tests fail badly. Mixed reads would have to be from git_mmap, which is the\n>> only other caller of pread().\n>>\n>> A simple alternative to ReOpenHandle is to reset the file pointer to its\n>> original position, as in compat/pread.c::git_pread. Thus single-theaded code\n>> can mix read()/pread() at will, but multi-threaded code has to use pread()\n>> exclusively (which is usually the case anyway). A main thread using read()\n>> and background threads using pread() (which is technically allowed by POSIX)\n>> will fail with this solution.\n>>\n>> This version passes the test suite on msysgit:\n>>\n>> ----8<----\n>> ssize_t mingw_pread(int fd, void *buf, size_t count, off64_t offset)\n>> {\n>> DWORD bytes_read;\n>> OVERLAPPED overlapped;\n>> off64_t current;\n>> memset(&overlapped, 0, sizeof(overlapped));\n>> overlapped.Offset = (DWORD) offset;\n>> overlapped.OffsetHigh = (DWORD) (offset >> 32);\n>>\n>> current = lseek64(fd, 0, SEEK_CUR);\n>>\n>> if (!ReadFile((HANDLE)_get_osfhandle(fd), buf, count, &bytes_read,\n>> &overlapped)) {\n>> errno = err_win_to_posix(GetLastError());\n>> return -1;\n>> }\n>>\n>> lseek64(fd, current, SEEK_SET);\n>>\n>> return (ssize_t) bytes_read;\n>> }\n>>\n>\n"},{"id":"234803","messageId":"CAHOQ7J_mM8mO5gEn_-xtmReS86pSDRrVaCAMzbgJPnmpu3F=oA@mail.gmail.com","threadId":"35846","inReplyTo":"CAAErz9g7ND1htfk=yxRJJLbSEgBi4EV_AHC9uDRptugGWFWcXw@mail.gmail.com","subject":"Re: Make the git codebase thread-safe","fromName":"Stefan Zager","fromEmail":"szager@google.com","sentAt":"2014-02-14T19:52:58Z","receivedAt":"2014-02-14T19:52:58Z","isPatch":false,"sender":{"key":"szager@google.com","avatar":null},"body":"On Fri, Feb 14, 2014 at 11:15 AM, Zachary Turner wrote:\n> For the mixed read, we wouldn't be looking for another caller of pread()\n> (since it doesn't care what the file pointer is), but instead a caller of\n> read() or lseek(). In index-pack.c, I see two possible culprits:\n>\n> 1) A call to xread() from inside fill()\n> 2) A call to lseek in parse_pack_objects()\n>\n> Do you think these could be related? If so, maybe that opens up some other\n> solutions?\n\n>From my observations, it's not that simple. As you pointed out to me\nbefore, fill() is only called before the threading part of the code,\nand lseek is only called after the threading part; and the lseek() is\nlseek(fd, 0, SEEK_CUR), so it's purely advisory.\n\nAlso, here is the error output we got before you added ReOpenFile:\n\nremote: Total 2514467 (delta 1997300), reused 2513040 (delta 1997113)\nChecking connectivity... error: packfile\nd:/src/chromium2/_gclient_src_a6y1bf/.git/objects/pack/pack-3b8d06040ac37f14d0b43859926f1153fea61a7a.pack\ndoes not match index\nwarning: packfile\nd:/src/chromium2/_gclient_src_a6y1bf/.git/objects/pack/pack-3b8d06040ac37f14d0b43859926f1153fea61a7a.pack\ncannot be accessed\nerror: packfile\nd:/src/chromium2/_gclient_src_a6y1bf/.git/objects/pack/pack-3b8d06040ac37f14d0b43859926f1153fea61a7a.pack\ndoes not match index\nwarning: packfile\nd:/src/chromium2/_gclient_src_a6y1bf/.git/objects/pack/pack-3b8d06040ac37f14d0b43859926f1153fea61a7a.pack\ncannot be accessed\nerror: packfile\nd:/src/chromium2/_gclient_src_a6y1bf/.git/objects/pack/pack-3b8d06040ac37f14d0b43859926f1153fea61a7a.pack\ndoes not match index\nwarning: packfile\nd:/src/chromium2/_gclient_src_a6y1bf/.git/objects/pack/pack-3b8d06040ac37f14d0b43859926f1153fea61a7a.pack\ncannot be accessed\nfatal: bad object e0f9f23f765a45e6d80863a8f881ee735c9347fe\n\n\nThe 'Checking connectivity...' message comes from builtin/clone.c,\nwhich runs in a separate process from builtin/index-pack.c. What this\nsuggests to me is that file descriptors for the loose object files are\nnot being flushed or closed properly before index-pack finishes.\n\n\n> BTW, the version you posted isn't thread safe. Suppose thread A and thread\n> B execute this function at the same time. A executes through the\n> ReadFile(), but does not yet reset the second lseek64. B then executes the\n> first lseek64(), storing off the modified file pointer. Then A finishes,\n> then B finishes. At the end, the file pointer is still modified.\n\nYes, that. I would also point out that in our experiments, ReOpenFile\nis not nearly as expensive as its name might suggest. Since the\nsolution using ReOpenFile is pretty solidly thread-safe (at least as\nfar as we can tell), I'm in favor of using it unless or until we\nproperly root-case the failure.\n\n\nStefan\n"},{"id":"234816","messageId":"CAHOQ7J9QFWAY+12RSwgp3a6VwSOWnwRsP=R8Tf2JA2yG+Ay84Q@mail.gmail.com","threadId":"35846","inReplyTo":"52FE68C9.3060403@gmail.com","subject":"Re: Make the git codebase thread-safe","fromName":"Stefan Zager","fromEmail":"szager@google.com","sentAt":"2014-02-14T21:49:34Z","receivedAt":"2014-02-14T21:49:34Z","isPatch":false,"sender":{"key":"szager@google.com","avatar":null},"body":"On Fri, Feb 14, 2014 at 11:04 AM, Karsten Blees wrote:\n>\n> Damn...you're right, multi-threaded git-index-pack works fine, but some tests fail badly. Mixed reads would have to be from git_mmap, which is the only other caller of pread().\n\nmsysgit used git_mmap() as defined in compat/win32mmap.c, which does\nnot use pread.\n\nStefan\n"},{"id":"234822","messageId":"52FEA271.2030405@gmail.com","threadId":"35846","inReplyTo":"CAAErz9j=_FpWLSyUk43pp8A6e7Ej0crT8ghW5-yxBEbGkd6O+A@mail.gmail.com","subject":"Re: Make the git codebase thread-safe","fromName":"Karsten Blees","fromEmail":"karsten.blees@gmail.com","sentAt":"2014-02-14T23:10:41Z","receivedAt":"2014-02-14T23:10:41Z","isPatch":false,"sender":{"key":"karsten.blees@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1111200?v=4"},"body":"Am 14.02.2014 20:16, schrieb Zachary Turner:\n> For the mixed read, we wouldn't be looking for another caller of\n> pread() (since it doesn't care what the file pointer is), but instead\n> a caller of read() or lseek() (since those do depend on the current\n> file pointer). In index-pack.c, I see two possible culprits:\n> \n> 1) A call to xread() from inside fill()\n> 2) A call to lseek in parse_pack_objects()\n> \n> Do you think these could be related? If so, maybe that opens up some\n> other solutions?\n> \n\nYeah, I think that's it. The problem is that the single-threaded part (parse_pack_objects/parse_pack_header) _also_ calls pread (via sha1_object -> get_data_from_pack -> unpack_data). So a pread() that modifies the file position would naturally be bad in this single-threaded scenario. Incidentally, that's exactly what the lstat64 in the version below fixes (similar to git_pread).\n\n> BTW, the version you posted isn't thread safe.\n\nIt is true that, in a multi-threaded scenario, my version modifies the file position in some indeterministic way. However, as you noted above, the file position is irrelevant to pread(), so that's perfectly thread-safe, as long as all threads use pread() exclusively.\n\nUsing [x]read() in one of the threads would _not_ be thread-safe, but we're not doing that here. Both fill()/xread() and parse_pack_objects()/lseek() are unreachable from threaded_second_pass(), and the main thread just waits for the background threads to complete...\n\n>>> A simple alternative to ReOpenHandle is to reset the file pointer to its\n>>> original position, as in compat/pread.c::git_pread. Thus single-theaded code\n>>> can mix read()/pread() at will, but multi-threaded code has to use pread()\n>>> exclusively (which is usually the case anyway). A main thread using read()\n>>> and background threads using pread() (which is technically allowed by POSIX)\n>>> will fail with this solution.\n>>>\n>>> This version passes the test suite on msysgit:\n>>>\n>>> ----8<----\n>>> ssize_t mingw_pread(int fd, void *buf, size_t count, off64_t offset)\n>>> {\n>>> DWORD bytes_read;\n>>> OVERLAPPED overlapped;\n>>> off64_t current;\n>>> memset(&overlapped, 0, sizeof(overlapped));\n>>> overlapped.Offset = (DWORD) offset;\n>>> overlapped.OffsetHigh = (DWORD) (offset >> 32);\n>>>\n>>> current = lseek64(fd, 0, SEEK_CUR);\n>>>\n>>> if (!ReadFile((HANDLE)_get_osfhandle(fd), buf, count, &bytes_read,\n>>> &overlapped)) {\n>>> errno = err_win_to_posix(GetLastError());\n>>> return -1;\n>>> }\n>>>\n>>> lseek64(fd, current, SEEK_SET);\n>>>\n>>> return (ssize_t) bytes_read;\n>>> }\n>>>\n>>\n"},{"id":"234831","messageId":"CACsJy8Dzj5iyaUseNyU76ojG1C0VYR=v7xsc=6TSGxTh=Xh3Ag@mail.gmail.com","threadId":"35846","inReplyTo":"CAAErz9j=_FpWLSyUk43pp8A6e7Ej0crT8ghW5-yxBEbGkd6O+A@mail.gmail.com","subject":"Re: Make the git codebase thread-safe","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-02-15T00:45:45Z","receivedAt":"2014-02-15T00:45:45Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Feb 15, 2014 at 2:16 AM, Zachary Turner wrote:\n> (Gah, sorry if you're receiving multiple emails to your personal\n> addresses, I need to get used to manually setting Plain-text mode\n> every time I send a message).\n>\n> For the mixed read, we wouldn't be looking for another caller of\n> pread() (since it doesn't care what the file pointer is), but instead\n> a caller of read() or lseek() (since those do depend on the current\n> file pointer). In index-pack.c, I see two possible culprits:\n>\n> 1) A call to xread() from inside fill()\n> 2) A call to lseek in parse_pack_objects()\n>\n> Do you think these could be related? If so, maybe that opens up some\n> other solutions?\n\nFor index-pack alone, what's wrong with open one file handle per thread?\n-- \nDuy\n"},{"id":"234832","messageId":"CAHOQ7J-gGbnADQ+3TGy6b6LJSLH8jvAbdTrc20Ybh=p0D2FmsQ@mail.gmail.com","threadId":"35846","inReplyTo":"CACsJy8Dzj5iyaUseNyU76ojG1C0VYR=v7xsc=6TSGxTh=Xh3Ag@mail.gmail.com","subject":"Re: Make the git codebase thread-safe","fromName":"Stefan Zager","fromEmail":"szager@google.com","sentAt":"2014-02-15T00:50:28Z","receivedAt":"2014-02-15T00:50:28Z","isPatch":false,"sender":{"key":"szager@google.com","avatar":null},"body":"On Fri, Feb 14, 2014 at 4:45 PM, Duy Nguyen wrote:\n> On Sat, Feb 15, 2014 at 2:16 AM, Zachary Turner wrote:\n>> (Gah, sorry if you're receiving multiple emails to your personal\n>> addresses, I need to get used to manually setting Plain-text mode\n>> every time I send a message).\n>>\n>> For the mixed read, we wouldn't be looking for another caller of\n>> pread() (since it doesn't care what the file pointer is), but instead\n>> a caller of read() or lseek() (since those do depend on the current\n>> file pointer). In index-pack.c, I see two possible culprits:\n>>\n>> 1) A call to xread() from inside fill()\n>> 2) A call to lseek in parse_pack_objects()\n>>\n>> Do you think these could be related? If so, maybe that opens up some\n>> other solutions?\n>\n> For index-pack alone, what's wrong with open one file handle per thread?\n\nNothing wrong with that, except that it would mean either using\nthread-local storage (which the code doesn't currently use); or\nplumbing pack_fd through the call stack, which doesn't sound very fun.\n\nStefan\n\n> --\n> Duy\n"},{"id":"234833","messageId":"CACsJy8AQNmmW40R-H7kz1dmwiaSKVgu+GP=Jt1qTKgfbZoMkMA@mail.gmail.com","threadId":"35846","inReplyTo":"CAHOQ7J-gGbnADQ+3TGy6b6LJSLH8jvAbdTrc20Ybh=p0D2FmsQ@mail.gmail.com","subject":"Re: Make the git codebase thread-safe","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-02-15T00:56:00Z","receivedAt":"2014-02-15T00:56:00Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Feb 15, 2014 at 7:50 AM, Stefan Zager wrote:\n> On Fri, Feb 14, 2014 at 4:45 PM, Duy Nguyen wrote:\n>> On Sat, Feb 15, 2014 at 2:16 AM, Zachary Turner wrote:\n>>> (Gah, sorry if you're receiving multiple emails to your personal\n>>> addresses, I need to get used to manually setting Plain-text mode\n>>> every time I send a message).\n>>>\n>>> For the mixed read, we wouldn't be looking for another caller of\n>>> pread() (since it doesn't care what the file pointer is), but instead\n>>> a caller of read() or lseek() (since those do depend on the current\n>>> file pointer). In index-pack.c, I see two possible culprits:\n>>>\n>>> 1) A call to xread() from inside fill()\n>>> 2) A call to lseek in parse_pack_objects()\n>>>\n>>> Do you think these could be related? If so, maybe that opens up some\n>>> other solutions?\n>>\n>> For index-pack alone, what's wrong with open one file handle per thread?\n>\n> Nothing wrong with that, except that it would mean either using\n> thread-local storage (which the code doesn't currently use); or\n> plumbing pack_fd through the call stack, which doesn't sound very fun.\n\nCurrent code does use thread-local storage (struct thread_local and\nget_thread_data). Adding a new file handle when NO_THREAD_SAFE_PREAD\nis defined is simpler imo.\n-- \nDuy\n"},{"id":"234834","messageId":"CAAErz9gO3NrAF5Zhu277NLqBv-4otQVWGBP6fX00x2OJ3v0_fg@mail.gmail.com","threadId":"35846","inReplyTo":"CACsJy8AQNmmW40R-H7kz1dmwiaSKVgu+GP=Jt1qTKgfbZoMkMA@mail.gmail.com","subject":"Re: Make the git codebase thread-safe","fromName":"Zachary Turner","fromEmail":"zturner@chromium.org","sentAt":"2014-02-15T01:15:20Z","receivedAt":"2014-02-15T01:15:20Z","isPatch":false,"sender":{"key":"zturner@chromium.org","avatar":null},"body":"Even if we make that change to use TLS for this case, the\nimplementation of pread() will still change in such a way that the\nsemantics of pread() are different on Windows. Is that ok?\n\nJust to summarize, here's the viable approaches I've seen discussed so far:\n\n1) Use _WINVER at compile time to select either a thread-safe or\nnon-thread-safe implementation of pread. This is the easiest possible\ncode change, but would necessitate 2 binary distributions of git for\nwindows.\n2) Use TLS as you suggest and have one fd per pack thread. Probably\nthe most complicated code change (at least for me, being a first-time\ncontributor)\n3) Use Karsten's suggested implementation from earlier in the thread.\nSeems to work, but it's a little confusing from a readability\nstandpoint since the implementation is not-thread safe except in this\nspecific usage context.\n\nOn Fri, Feb 14, 2014 at 4:56 PM, Duy Nguyen wrote:\n> On Sat, Feb 15, 2014 at 7:50 AM, Stefan Zager wrote:\n>> On Fri, Feb 14, 2014 at 4:45 PM, Duy Nguyen wrote:\n>>> On Sat, Feb 15, 2014 at 2:16 AM, Zachary Turner wrote:\n>>>> (Gah, sorry if you're receiving multiple emails to your personal\n>>>> addresses, I need to get used to manually setting Plain-text mode\n>>>> every time I send a message).\n>>>>\n>>>> For the mixed read, we wouldn't be looking for another caller of\n>>>> pread() (since it doesn't care what the file pointer is), but instead\n>>>> a caller of read() or lseek() (since those do depend on the current\n>>>> file pointer). In index-pack.c, I see two possible culprits:\n>>>>\n>>>> 1) A call to xread() from inside fill()\n>>>> 2) A call to lseek in parse_pack_objects()\n>>>>\n>>>> Do you think these could be related? If so, maybe that opens up some\n>>>> other solutions?\n>>>\n>>> For index-pack alone, what's wrong with open one file handle per thread?\n>>\n>> Nothing wrong with that, except that it would mean either using\n>> thread-local storage (which the code doesn't currently use); or\n>> plumbing pack_fd through the call stack, which doesn't sound very fun.\n>\n> Current code does use thread-local storage (struct thread_local and\n> get_thread_data). Adding a new file handle when NO_THREAD_SAFE_PREAD\n> is defined is simpler imo.\n> --\n> Duy\n"},{"id":"234835","messageId":"CACsJy8BdRd8yLjtYqGqQd2b1f550GLq6duZCD3JNiTO+K3GK6w@mail.gmail.com","threadId":"35846","inReplyTo":"CAAErz9gO3NrAF5Zhu277NLqBv-4otQVWGBP6fX00x2OJ3v0_fg@mail.gmail.com","subject":"Re: Make the git codebase thread-safe","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-02-15T01:39:47Z","receivedAt":"2014-02-15T01:39:47Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Feb 15, 2014 at 8:15 AM, Zachary Turner wrote:\n> Even if we make that change to use TLS for this case, the\n> implementation of pread() will still change in such a way that the\n> semantics of pread() are different on Windows. Is that ok?\n>\n> Just to summarize, here's the viable approaches I've seen discussed so far:\n>\n> 1) Use _WINVER at compile time to select either a thread-safe or\n> non-thread-safe implementation of pread. This is the easiest possible\n> code change, but would necessitate 2 binary distributions of git for\n> windows.\n> 2) Use TLS as you suggest and have one fd per pack thread. Probably\n> the most complicated code change (at least for me, being a first-time\n> contributor)\n\nIt's not so complicated. I suggested a patch [1] before (surprise!).\n\n> 3) Use Karsten's suggested implementation from earlier in the thread.\n> Seems to work, but it's a little confusing from a readability\n> standpoint since the implementation is not-thread safe except in this\n> specific usage contex\n\n[1] http://article.gmane.org/gmane.comp.version-control.git/196042\n-- \nDuy\n"},{"id":"234998","messageId":"xmqqlhx8fgqs.fsf@gitster.dls.corp.google.com","threadId":"35846","inReplyTo":"CACsJy8BdRd8yLjtYqGqQd2b1f550GLq6duZCD3JNiTO+K3GK6w@mail.gmail.com","subject":"Re: Make the git codebase thread-safe","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-18T17:55:07Z","receivedAt":"2014-02-18T17:55:07Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> On Sat, Feb 15, 2014 at 8:15 AM, Zachary Turner wrote:\n> ...\n>> 2) Use TLS as you suggest and have one fd per pack thread. Probably\n>> the most complicated code change (at least for me, being a first-time\n>> contributor)\n>\n> It's not so complicated. I suggested a patch [1] before (surprise!).\n> ...\n> [1] http://article.gmane.org/gmane.comp.version-control.git/196042\n\nThat message is at the tail end of the discussion. I wonder why\nnothing came out of it back then.\n\nWhile I do not see anything glaringly wrong with the change from a\nquick glance over it, it would be nice to hear how well it performs\non their platform from Windows folks.\n\nThanks.\n"},{"id":"234999","messageId":"CAAErz9iApJ=og65zoctJLfR8PqO2g3PZVJZ76m=ZGzt3TeMHbw@mail.gmail.com","threadId":"35846","inReplyTo":"xmqqlhx8fgqs.fsf@gitster.dls.corp.google.com","subject":"Re: Make the git codebase thread-safe","fromName":"Zachary Turner","fromEmail":"zturner@chromium.org","sentAt":"2014-02-18T18:14:27Z","receivedAt":"2014-02-18T18:14:27Z","isPatch":false,"sender":{"key":"zturner@chromium.org","avatar":null},"body":"It shouldn't be hard for us to run some tests with this patch applied.\n Will report back in a day or two.\n\nOn Tue, Feb 18, 2014 at 9:55 AM, Junio C Hamano wrote:\n> Duy Nguyen writes:\n>\n>> On Sat, Feb 15, 2014 at 8:15 AM, Zachary Turner wrote:\n>> ...\n>>> 2) Use TLS as you suggest and have one fd per pack thread. Probably\n>>> the most complicated code change (at least for me, being a first-time\n>>> contributor)\n>>\n>> It's not so complicated. I suggested a patch [1] before (surprise!).\n>> ...\n>> [1] http://article.gmane.org/gmane.comp.version-control.git/196042\n>\n> That message is at the tail end of the discussion. I wonder why\n> nothing came out of it back then.\n>\n> While I do not see anything glaringly wrong with the change from a\n> quick glance over it, it would be nice to hear how well it performs\n> on their platform from Windows folks.\n>\n> Thanks.\n"},{"id":"372957","messageId":"20190402005245.4983-1-matheus.bernardino@usp.br","threadId":"35846","inReplyTo":"CA+TurHgyUK5sfCKrK+3xY8AeOg0t66vEvFxX=JiA9wXww7eZXQ@mail.gmail.com","subject":"Re: Make the git codebase thread-safe","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-04-02T00:52:45Z","receivedAt":"2019-04-02T00:53:00Z","isPatch":false,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Hi,\n\nI am planning to work on making pack access thread-safe as my GSoC\nproject, and after that, parallelize git blame or checkout. Or even use\nthe thread-safe pack access to improve the already parallel grep or\npack-objects.\n\nWith this in mind, I would like to know if the problem discussed in this\nthread[1] is still an issue on the repos you folks work with (gentoo,\nchromium, etc.). And also, could you please let me know which git\ncommands did you find to me more problematic in them, nowadays?\n\nI downloaded chromium to give it a try and got (on a machine with i7 and\nSSD, running Manjaro Linux):\n\n- 17s on blame for a file with long history[2]\n- 2m on blame for a huge file[3]\n- 15s on log for both [2] and [3]\n- 1s for git status\n\nIt seems quite a lot, especially with SSD, IMO.\n\n[1] https://public-inbox.org/git/CA+TurHgyUK5sfCKrK+3xY8AeOg0t66vEvFxX=JiA9wXww7eZXQ@mail.gmail.com/\n[2] ./chrome/browser/about_flags.cc (same with ./DEPS)\n[3] third_party/sqlite/amalgamation/sqlite3.c (7.5M)\n\nBest,\nMatheus Tavares\n"},{"id":"372959","messageId":"CACsJy8BSDz1JO+w1N9w2W1zxY+EWTxiU6yB_V0eeOD--g-TzeA@mail.gmail.com","threadId":"35846","inReplyTo":"20190402005245.4983-1-matheus.bernardino@usp.br","subject":"Re: Make the git codebase thread-safe","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-04-02T01:07:18Z","receivedAt":"2019-04-02T01:07:47Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Apr 2, 2019 at 7:52 AM Matheus Tavares\n wrote:\n> I downloaded chromium to give it a try and got (on a machine with i7 and\n> SSD, running Manjaro Linux):\n>\n> - 17s on blame for a file with long history[2]\n> - 2m on blame for a huge file[3]\n> - 15s on log for both [2] and [3]\n> - 1s for git status\n>\n> It seems quite a lot, especially with SSD, IMO.\n\nThere have been a couple of optimizations that are probably still not\nenabled by default because they only benefit large repos. So you may\nwant to check and turn them on before measuring anything:\ncommit-graph, pack bitmap, untracked cache or fsmonitor. All these\nshould be mentioned in 'git help config' (as starting point). Also\nsearch \"threads\" in that man page because some commands may have multi\nthreads support but disabled by default for the same reason.\n\nFrom your command list though, I think you might get the same results\n(maybe with a bit faster 'git status') even with all optimizations on.\n-- \nDuy\n"},{"id":"372972","messageId":"87lg0s66nm.fsf@fencepost.gnu.org","threadId":"35846","inReplyTo":"CACsJy8BSDz1JO+w1N9w2W1zxY+EWTxiU6yB_V0eeOD--g-TzeA@mail.gmail.com","subject":"Re: Make the git codebase thread-safe","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2019-04-02T10:30:37Z","receivedAt":"2019-04-02T10:30:48Z","isPatch":false,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"Duy Nguyen writes:\n\n> On Tue, Apr 2, 2019 at 7:52 AM Matheus Tavares\n> wrote:\n>> I downloaded chromium to give it a try and got (on a machine with i7 and\n>> SSD, running Manjaro Linux):\n>>\n>> - 17s on blame for a file with long history[2]\n>> - 2m on blame for a huge file[3]\n>> - 15s on log for both [2] and [3]\n>> - 1s for git status\n>>\n>> It seems quite a lot, especially with SSD, IMO.\n>\n> There have been a couple of optimizations that are probably still not\n> enabled by default because they only benefit large repos.\n\nI've proposed a trivial change in 2014 that could have cut down typical\nblame times significantly but nobody was interested in testing and\ncommitting it, and it is conceivable that in limited-memory situations\nit might warrant some accounting/mitigation for weird histories (not\nthat there isn't other code like that).\n\nRebased/appended.\n\n-- \nDavid Kastrup\n\n\nFrom a076daf13d144cb74ae5fd7250445bbfb4669a05 Mon Sep 17 00:00:00 2001\nFrom: David Kastrup \nDate: Sun, 2 Feb 2014 18:33:35 +0100\nSubject: [PATCH] blame.c: don't drop origin blobs as eagerly\n\nWhen a parent blob already has chunks queued up for blaming, dropping\nthe blob at the end of one blame step will cause it to get reloaded\nright away, doubling the amount of I/O and unpacking when processing a\nlinear history.\n\nKeeping such parent blobs in memory seems like a reasonable optimization\nthat should incur additional memory pressure mostly when processing the\nmerges from old branches.\n---\n blame.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/blame.c b/blame.c\nindex 5c07dec190..c11c516921 100644\n--- a/blame.c\n+++ b/blame.c\n@@ -1562,7 +1562,8 @@ static void pass_blame(struct blame_scoreboard *sb, struct blame_origin *origin,\n \t}\n \tfor (i = 0; i < num_sg; i++) {\n \t\tif (sg_origin[i]) {\n-\t\t\tdrop_origin_blob(sg_origin[i]);\n+\t\t\tif (!sg_origin[i]->suspects)\n+\t\t\t\tdrop_origin_blob(sg_origin[i]);\n \t\t\tblame_origin_decref(sg_origin[i]);\n \t\t}\n \t}\n-- \n2.20.1\n\n"},{"id":"372975","messageId":"CACsJy8BuJbxj5fHwTc+aogWcWGR_6A0CXS78-h0zi4rYLa0kXQ@mail.gmail.com","threadId":"35846","inReplyTo":"87lg0s66nm.fsf@fencepost.gnu.org","subject":"Re: Make the git codebase thread-safe","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-04-02T11:35:04Z","receivedAt":"2019-04-02T11:35:32Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Apr 2, 2019 at 5:30 PM David Kastrup wrote:\n>\n> Duy Nguyen writes:\n>\n> > On Tue, Apr 2, 2019 at 7:52 AM Matheus Tavares\n> > wrote:\n> >> I downloaded chromium to give it a try and got (on a machine with i7 and\n> >> SSD, running Manjaro Linux):\n> >>\n> >> - 17s on blame for a file with long history[2]\n> >> - 2m on blame for a huge file[3]\n> >> - 15s on log for both [2] and [3]\n> >> - 1s for git status\n> >>\n> >> It seems quite a lot, especially with SSD, IMO.\n> >\n> > There have been a couple of optimizations that are probably still not\n> > enabled by default because they only benefit large repos.\n>\n> I've proposed a trivial change in 2014 that could have cut down typical\n> blame times significantly but nobody was interested in testing and\n> committing it, and it is conceivable that in limited-memory situations\n> it might warrant some accounting/mitigation for weird histories (not\n> that there isn't other code like that).\n\nI didn't really read the patch (I don't know much about blame.c to\nreally contribute anything there). But a quick \"git blame --show-stats\nunpack-trees.c\" shows this\n\nWithout the patch:\n\nnum read blob: 767\nnum get patch: 425\nnum commits: 343\n\nWith the patch:\n\nnum read blob: 419\nnum get patch: 425\nnum commits: 343\n\nThat's a nice reduction of blob reading. On a typical small file, the\nactual time saving might be not much. But it could really help when\nyou blame a large file.\n\nPerhaps you could resubmit it again for inclusion? (at least a\nsign-off-by is missing then)\n\n> Rebased/appended.\n>\n> --\n> David Kastrup\n-- \nDuy\n"},{"id":"372976","messageId":"87d0m462up.fsf@fencepost.gnu.org","threadId":"35846","inReplyTo":"CACsJy8BuJbxj5fHwTc+aogWcWGR_6A0CXS78-h0zi4rYLa0kXQ@mail.gmail.com","subject":"Re: Make the git codebase thread-safe","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2019-04-02T11:52:46Z","receivedAt":"2019-04-02T11:52:56Z","isPatch":false,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"Duy Nguyen writes:\n\n> On Tue, Apr 2, 2019 at 5:30 PM David Kastrup wrote:\n>>\n>> Duy Nguyen writes:\n>>\n>> > On Tue, Apr 2, 2019 at 7:52 AM Matheus Tavares\n>> > wrote:\n>> >> I downloaded chromium to give it a try and got (on a machine with i7 and\n>> >> SSD, running Manjaro Linux):\n>> >>\n>> >> - 17s on blame for a file with long history[2]\n>> >> - 2m on blame for a huge file[3]\n>> >> - 15s on log for both [2] and [3]\n>> >> - 1s for git status\n>> >>\n>> >> It seems quite a lot, especially with SSD, IMO.\n>> >\n>> > There have been a couple of optimizations that are probably still not\n>> > enabled by default because they only benefit large repos.\n>>\n>> I've proposed a trivial change in 2014 that could have cut down typical\n>> blame times significantly but nobody was interested in testing and\n>> committing it, and it is conceivable that in limited-memory situations\n>> it might warrant some accounting/mitigation for weird histories (not\n>> that there isn't other code like that).\n>\n> I didn't really read the patch (I don't know much about blame.c to\n> really contribute anything there). But a quick \"git blame --show-stats\n> unpack-trees.c\" shows this\n>\n> Without the patch:\n>\n> num read blob: 767\n> num get patch: 425\n> num commits: 343\n>\n> With the patch:\n>\n> num read blob: 419\n> num get patch: 425\n> num commits: 343\n>\n> That's a nice reduction of blob reading. On a typical small file, the\n> actual time saving might be not much. But it could really help when\n> you blame a large file.\n>\n> Perhaps you could resubmit it again for inclusion? (at least a\n> sign-off-by is missing then)\n\nI don't expect it to go anywhere but will do. Feel free to herd it.\n\n-- \nDavid Kastrup\n"},{"id":"372993","messageId":"CAHd-oW5K5VrbhfYw+-bWmYXbysH1z8b2kSuDvxhPKBXgXj=KXw@mail.gmail.com","threadId":"35846","inReplyTo":"CACsJy8BSDz1JO+w1N9w2W1zxY+EWTxiU6yB_V0eeOD--g-TzeA@mail.gmail.com","subject":"Re: Make the git codebase thread-safe","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-04-02T19:06:57Z","receivedAt":"2019-04-02T19:07:11Z","isPatch":false,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Mon, Apr 1, 2019 at 10:07 PM Duy Nguyen wrote:\n>\n> On Tue, Apr 2, 2019 at 7:52 AM Matheus Tavares\n> wrote:\n> > I downloaded chromium to give it a try and got (on a machine with i7 and\n> > SSD, running Manjaro Linux):\n> >\n> > - 17s on blame for a file with long history[2]\n> > - 2m on blame for a huge file[3]\n> > - 15s on log for both [2] and [3]\n> > - 1s for git status\n> >\n> > It seems quite a lot, especially with SSD, IMO.\n>\n> There have been a couple of optimizations that are probably still not\n> enabled by default because they only benefit large repos. So you may\n> want to check and turn them on before measuring anything:\n> commit-graph, pack bitmap, untracked cache or fsmonitor. All these\n> should be mentioned in 'git help config' (as starting point). Also\n> search \"threads\" in that man page because some commands may have multi\n> threads support but disabled by default for the same reason.\n\nNice, thanks for the suggestions!\n\n> From your command list though, I think you might get the same results\n> (maybe with a bit faster 'git status') even with all optimizations on.\n\nYes, you were right. With the optimizations on, I got the following\ntimes on those same files:\n\n- 17~18s on blame for about_flags.cc\n- 1m50s~2m on blame for sqlite3.c\n- 15s on log for both\n- 0.3~0.5s on git status\n\n> --\n> Duy\n"}]}