{"thread":{"id":"22620","subject":"Re: Bug#569505: git-core: 'git add' corrupts repository if the working directory is modified as it runs","startedAt":"2010-02-12T00:27:41Z","lastAt":"2010-02-22T22:31:16Z","messageCount":84,"participants":["Jonathan Nieder","Zygo Blaxell","Ilari Liusvaara","Thomas Rast","Dmitry Potapov","Paolo Bonzini","Junio C Hamano","Johannes Schindelin","Jakub Narebski","Avery Pennarun","Nicolas Pitre","Jeff King","Wincent Colaiuta","Bill Lear","Peter Harris","Erik Faye-Lund"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"134281","messageId":"20100212002741.GB9883@progeny.tock","threadId":"22620","inReplyTo":"20100211234753.22574.48799.reportbug@gibbs.hungrycats.org","subject":"Re: Bug#569505: git-core: 'git add' corrupts repository if the working directory is modified as it runs","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-02-12T00:27:41Z","receivedAt":"2010-02-12T00:27:41Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi gitsters,\n\nZygo Blaxell reported through http://bugs.debian.org/569505 that ‘git\nupdate-index’ has some issues when the files it is adding change under\nits feet:\n\nMy thoughts:\n\n - Low-hanging fruit: it should be possible for update-index to check\n the stat information to see if the file has changed between when it\n first opens it and when it finishes.\n\n - Zygo reported suppress that ‘git gc’ didn’t notice the problem.\n Should ‘git gc’ imply a ‘git fsck --no-full’?\n\n - Recovering from this kind of mistake in early history is indeed\n hard. Any tricks for doing this? Maybe fast-export | fast-import\n can do something with this, or maybe replace + filter-branch once\n it learns to be a little smarter.\n\n - How do checkout-index and cat-file blob react to a blob whose\n contents do not reflect its object name? Are they behaving\n appropriately? I would want cat-file blob to be able to retrieve\n such a broken blob’s contents, checkout-index not so much.\n\nI imagine there are other things to learn, too. The report and\nreproduction recipe follow.\n\nThoughts?\nJonathan\n\nPackage: git-core\nVersion: 1:1.6.6.1-1\nSeverity: important\n\n'git add' will happily corrupt a git repo if it is run while files in\nthe working directory are being modified. A blob is added to the index\nwith contents that do not match its SHA1 hash. If the index is then\ncommitted, the corrupt blob cannot be checked out (or is checked out\nwith incorrect contents, depending on which tool you use to try to get\nthe file out of git) in the future.\n\nSurprisingly, it's possible to clone, fetch, push, pull, and sometimes\neven gc the corrupted repo several times before anyone notices the\ncorruption. If the affected commit is included in a merge with history\nfrom other git users, the only way to fix it is to rebase (or come up\nwith a blob whose contents match the affected SHA1 hash somehow).\n\nIt is usually possible to retrieve data committed before the corruption\nby simply checking out an earlier tree in the affected branch's history.\n\nThe following shell code demonstrates this problem. It runs a thread\nwhich continuously modifies a file, and another thread that does\n'git commit -am' and 'git fsck' in a continuous loop until corruption\nis detected. This might take up to 20 seconds on a slow machine.\n\n\t#!/bin/sh\n\tset -e\n\n\t# Create an empty git repo in /tmp/git-test\n\trm -fr /tmp/git-test\n\tmkdir /tmp/git-test\n\tcd /tmp/git-test\n\tgit init\n\n\t# Create a file named foo and add it to the repo\n\ttouch foo\n\tgit add foo\n\n\t# Thread 1: continuously modify foo:\n\twhile echo -n .; do\n\t\tdd if=/dev/urandom of=foo count=1024 bs=1k conv=notrunc >/dev/null 2>&1\n\tdone &\n\n\t# Thread 2: loop until the repo is corrupted\n\twhile git fsck; do\n\t\t# Note the implied 'git add' in 'commit -a'\n\t\t# It will do the same with explicit 'git add'\n\t\tgit commit -a -m'Test'\n\tdone\n\n\t# Kill thread 1, we don't need it any more\n\tkill $!\n\n\t# Success! Well, sort of.\n\techo Repository is corrupted. Have a nice day.\n\nI discovered this bug accidentally when I was using inotifywait (from\nthe inotify-tools package) to automatically commit snapshots of a working\ndirectory triggered by write events.\n\nI tested this with a number of kernel versions from 2.6.27 to 2.6.31.\nAll of them reproduced this problem. I checked this because strace\nshows 'git add' doing a mmap(..., MAP_PRIVATE, ...) of its input file,\nso I was wondering if there might have been a recent change in mmap()\nbehavior in either git or the kernel.\n\ngit 1.5.6.5 has this problem too, but some of the error messages are\ndifferent, and the problem sometimes manifests itself as silent corruption\nof other objects (e.g. if someone checks out a corrupt tree and then does\n'git add -u' or 'git commit -a', they will include the corrupt data in\ntheir commit).\n"},{"id":"134286","messageId":"20100212012314.GC24809@gibbs.hungrycats.org","threadId":"22620","inReplyTo":"20100212002741.GB9883@progeny.tock","subject":"Re: Bug#569505: git-core: 'git add' corrupts repository if the working directory is modified as it runs","fromName":"Zygo Blaxell","fromEmail":"zblaxell@gibbs.hungrycats.org","sentAt":"2010-02-12T01:23:14Z","receivedAt":"2010-02-12T01:23:14Z","isPatch":false,"sender":{"key":"zblaxell@gibbs.hungrycats.org","avatar":null},"body":"On Thu, Feb 11, 2010 at 06:27:41PM -0600, Jonathan Nieder wrote:\n> Zygo Blaxell reported through http://bugs.debian.org/569505 that ???git\n> update-index??? has some issues when the files it is adding change under\n> its feet:\n> \n> My thoughts:\n> \n> - Low-hanging fruit: it should be possible for update-index to check\n> the stat information to see if the file has changed between when it\n> first opens it and when it finishes.\n\nI don't think this is a good idea. stat() is very coarse-grained, and\nprovides accuracy of only a second on a lot of file systems where git\nworking directories might be found. If you run the test script on an\next3 filesystem on a modern machine the stat() data won't change at all\neven though the file contents change completely many times.\n\nWhat would be a good idea is to make sure that the code that copies a\nfile into the index and calculates its hash does both in a single pass\nover the same input data. That might require replacing a simple mmap()\nof the input file with a read-hash-copy loop.\n"},{"id":"134425","messageId":"20100213121238.GA2559@progeny.tock","threadId":"22620","inReplyTo":"20100212012314.GC24809@gibbs.hungrycats.org","subject":"Re: 'git add' corrupts repository if the working directory is modified as it runs","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-02-13T12:12:38Z","receivedAt":"2010-02-13T12:12:38Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Zygo Blaxell wrote:\n> On Thu, Feb 11, 2010 at 06:27:41PM -0600, Jonathan Nieder wrote:\n\n>> - Low-hanging fruit: it should be possible for update-index to check\n>> the stat information to see if the file has changed between when it\n>> first opens it and when it finishes.\n>\n> I don't think this is a good idea. stat() is very coarse-grained\n\nYou’re probably right. For many file types, st_size is likely to\nchange (in this way your script is testing something unusual), but\nthat is no excuse to behave poorly when it doesn’t.\n\n> What would be a good idea is to make sure that the code that copies a\n> file into the index and calculates its hash does both in a single pass\n> over the same input data. That might require replacing a simple mmap()\n> of the input file with a read-hash-copy loop.\n\nThis leaves me nervous about speed. Consider the following simple\ncase: someone the file to be added is already in the object\nrepository somewhere (maybe the user has tried this code before, or\na file was renamed with 'mv', or a patch applied with 'patch', or an\nunmount and remount dirtied the stat information).\n\nWith the current code, write_sha1_file() will hash the file, notice\nthat object is already in .git/objects, and return. With a\nread-hash-copy loop, git would have to store a (compressed or\nuncompressed) copy of the file somewhere in the meantime.\n\nBut I’d be happy to see code appear that proves me wrong. ;-) One\nsimple benchmark to try is running the git test suite.\n\nCheers,\nJonathan\n"},{"id":"134428","messageId":"20100213133951.GA14352@Knoppix","threadId":"22620","inReplyTo":"20100213121238.GA2559@progeny.tock","subject":"Re: 'git add' corrupts repository if the working directory is modified as it runs","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2010-02-13T13:39:52Z","receivedAt":"2010-02-13T13:39:52Z","isPatch":false,"sender":{"key":"ilari.liusvaara@elisanet.fi","avatar":null},"body":"On Sat, Feb 13, 2010 at 06:12:38AM -0600, Jonathan Nieder wrote:\n> \n> This leaves me nervous about speed. Consider the following simple\n> case: someone the file to be added is already in the object\n> repository somewhere (maybe the user has tried this code before, or\n> a file was renamed with 'mv', or a patch applied with 'patch', or an\n> unmount and remount dirtied the stat information).\n> \n> With the current code, write_sha1_file() will hash the file, notice\n> that object is already in .git/objects, and return. With a\n> read-hash-copy loop, git would have to store a (compressed or\n> uncompressed) copy of the file somewhere in the meantime.\n\nIt could be done by first reading the file and computing hash,\nif the hash matches existing object, return that hash. Otherwise\nread the file for object write, hashing it again and use that value\nfor object ID.\n\nThis would require two hash computations in non-existing case,\nbut SHA-1 is pretty fast. If the first computation produces match,\nthen it doesn't matter if file is modified as adding and modifying\nin parallel results undefined contents anyway (just that it should\nnot corrupt the repository).\n\n-Ilari\n"},{"id":"134437","messageId":"201002131539.54142.trast@student.ethz.ch","threadId":"22620","inReplyTo":"20100213133951.GA14352@Knoppix","subject":"Re: 'git add' corrupts repository if the working directory is modified as it runs","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2010-02-13T14:39:53Z","receivedAt":"2010-02-13T14:39:53Z","isPatch":false,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"On Saturday 13 February 2010 14:39:52 Ilari Liusvaara wrote:\n> On Sat, Feb 13, 2010 at 06:12:38AM -0600, Jonathan Nieder wrote:\n> > \n> > With the current code, write_sha1_file() will hash the file, notice\n> > that object is already in .git/objects, and return. With a\n> > read-hash-copy loop, git would have to store a (compressed or\n> > uncompressed) copy of the file somewhere in the meantime.\n> \n> It could be done by first reading the file and computing hash,\n> if the hash matches existing object, return that hash. Otherwise\n> read the file for object write, hashing it again and use that value\n> for object ID.\n\nThat is still racy. The real problem is that the file is mmap()ed,\nand git then first computes the SHA1 of that buffer, next it\ncompresses it.[*]\n\nDue to the last sentence in the following snippet from mmap(2):\n\n MAP_PRIVATE\n Create a private copy-on-write mapping. Updates to the map-\n ping are not visible to other processes mapping the same\n file, and are not carried through to the underlying file.\n It is unspecified whether changes made to the file after the\n mmap() call are visible in the mapped region.\n\nThis is racy despite the use of MAP_PRIVATE: the mapped contents can\nchange at any time.\n\nAFAICS there are only two possible solutions:\n\n* Copy the file (possibly block-by-block) as we go, to make sure that\n the data we SHA1 is the same we compress.\n\n* Unpack and re-hash the compressed data to verify that the SHA1 is\n correct. In case of failure either retry (but you could have to do\n this infinitely often if the user just hates you!) or abort.\n\n(Of course, in neither case does the user have any sort of guarantee\nabout what data ended up in the repository, but he never had that, we\nonly try to ensure repo consistency.)\n\n\n[*] The \"do we have this\" check actually happens before the\ncompression, and that arm is thus race-free.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"134444","messageId":"20100213162924.GA14623@Knoppix","threadId":"22620","inReplyTo":"201002131539.54142.trast@student.ethz.ch","subject":"Re: 'git add' corrupts repository if the working directory is modified as it runs","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2010-02-13T16:29:24Z","receivedAt":"2010-02-13T16:29:24Z","isPatch":false,"sender":{"key":"ilari.liusvaara@elisanet.fi","avatar":null},"body":"On Sat, Feb 13, 2010 at 03:39:53PM +0100, Thomas Rast wrote:\n> On Saturday 13 February 2010 14:39:52 Ilari Liusvaara wrote:\n> > On Sat, Feb 13, 2010 at 06:12:38AM -0600, Jonathan Nieder wrote:\n> > > \n> > > With the current code, write_sha1_file() will hash the file, notice\n> > > that object is already in .git/objects, and return. With a\n> > > read-hash-copy loop, git would have to store a (compressed or\n> > > uncompressed) copy of the file somewhere in the meantime.\n> > \n> > It could be done by first reading the file and computing hash,\n> > if the hash matches existing object, return that hash. Otherwise\n> > read the file for object write, hashing it again and use that value\n> > for object ID.\n> \n> That is still racy. The real problem is that the file is mmap()ed,\n> and git then first computes the SHA1 of that buffer, next it\n> compresses it.[*]\n\nHmm... One needs to copy the data block at time into temporary buffer\nand use that for feeding zlib and SHA-1. That ensures that whatever\nSHA-1 hashes and zlib compresses are consistent.\n\n-Ilari\n"},{"id":"134498","messageId":"37fcd2781002131409r4166e496h9d12d961a2330914@mail.gmail.com","threadId":"22620","inReplyTo":"20100213162924.GA14623@Knoppix","subject":"Re: 'git add' corrupts repository if the working directory is modified as it runs","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2010-02-13T22:09:23Z","receivedAt":"2010-02-13T22:09:23Z","isPatch":false,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"On Sat, Feb 13, 2010 at 7:29 PM, Ilari Liusvaara\n wrote:\n>\n> Hmm... One needs to copy the data block at time into temporary buffer\n> and use that for feeding zlib and SHA-1. That ensures that whatever\n> SHA-1 hashes and zlib compresses are consistent.\n\nIf you want this then just compile Git with NO_MMAP = YesPlease\nIt should solve the described problem.\n\nDmitry\n"},{"id":"134500","messageId":"20100213223733.GP24809@gibbs.hungrycats.org","threadId":"22620","inReplyTo":"37fcd2781002131409r4166e496h9d12d961a2330914@mail.gmail.com","subject":"Re: 'git add' corrupts repository if the working directory is modified as it runs","fromName":"Zygo Blaxell","fromEmail":"zblaxell@esightcorp.com","sentAt":"2010-02-13T22:37:33Z","receivedAt":"2010-02-13T22:37:33Z","isPatch":false,"sender":{"key":"zblaxell@esightcorp.com","avatar":null},"body":"On Sun, Feb 14, 2010 at 01:09:23AM +0300, Dmitry Potapov wrote:\n> On Sat, Feb 13, 2010 at 7:29 PM, Ilari Liusvaara\n> wrote:\n> > Hmm... One needs to copy the data block at time into temporary buffer\n> > and use that for feeding zlib and SHA-1. That ensures that whatever\n> > SHA-1 hashes and zlib compresses are consistent.\n> \n> If you want this then just compile Git with NO_MMAP = YesPlease\n> It should solve the described problem.\n\nDoesn't that also turn off mmap in other places where it's harmless or\neven beneficial? Otherwise, why use mmap in Git at all? It also doesn't\nsolve the problem in cases when mmap support is compiled in.\n\nThere is a performance-robustness trade-off here. If we do an extra\ncopy of the file data, we get always consistent repo contents but lose\nspeed (not very much speed, since sha1 and zlib are much slower than\na memory copy). If we don't, we still get consistent repo contents\nif--and only if--the files never happen to be modified during a git add.\nI can live with that, as long as the limitation is documented and there's\na config switch somewhere that I can turn on for cases when I can't make\nsuch assurances.\n\nI imagine similar reasoning led to the existence of the\nreceive.fsckObjects option. Personally, checking all objects fetched\nfrom remote repos is not a feature I'd ever want to be able to turn off;\nhowever, it's easy to think of cases where integrity matters less than\nspeed (e.g. build systems doing clone-build-destroy cycles from a trusted,\nreliable server over a similarly trusted network).\n"},{"id":"134508","messageId":"20100214011812.GA2175@dpotapov.dyndns.org","threadId":"22620","inReplyTo":"20100213223733.GP24809@gibbs.hungrycats.org","subject":"[PATCH] don't use mmap() to hash files","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2010-02-14T01:18:12Z","receivedAt":"2010-02-14T01:18:12Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"If a mmapped file is changed by another program during git-add, it\ncauses the repository corruption. Disabling mmap() in index_fd() does\nnot have any negative impact on the overall speed of Git. In fact, it\nmakes git hash-object to work slightly faster. Here is the best result\nbefore and after patch based on 5 runs on the Linix kernel repository:\n\nBefore:\n\n$ git ls-files | time git hash-object --stdin-path > /dev/null\n2.15user 0.36system 0:02.52elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+103248minor)pagefaults 0swaps\n\nAfter:\n\n$ git ls-files | time ../git/git hash-object --stdin-path > /dev/null\n2.09user 0.33system 0:02.42elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+1073minor)pagefaults 0swaps\n\nSigned-off-by: Dmitry Potapov \n---\n\nI think more people should test this change to see its impact on\nperformance. For me, it was positive. Here is my results:\n\nUsing mmap (git version 1.7.0)\n\n2.18user 0.33system 0:02.52elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+103248minor)pagefaults 0swaps\n$ git ls-files | time git hash-object --stdin-path > /dev/null\n2.23user 0.28system 0:02.53elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+103249minor)pagefaults 0swaps\n$ git ls-files | time git hash-object --stdin-path > /dev/null\n2.20user 0.31system 0:02.52elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+103248minor)pagefaults 0swaps\n$ git ls-files | time git hash-object --stdin-path > /dev/null\n2.21user 0.30system 0:02.51elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+103248minor)pagefaults 0swaps\n$ git ls-files | time git hash-object --stdin-path > /dev/null\n2.15user 0.36system 0:02.52elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+103248minor)pagefaults 0swaps\n\nUsing read() instead of mmap() (1.7.0 with the above patch)\n\n$ git ls-files | time ../git/git hash-object --stdin-path > /dev/null\n2.19user 0.24system 0:02.42elapsed 100%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+1073minor)pagefaults 0swaps\n$ git ls-files | time ../git/git hash-object --stdin-path > /dev/null\n2.15user 0.26system 0:02.42elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+1073minor)pagefaults 0swaps\n$ git ls-files | time ../git/git hash-object --stdin-path > /dev/null\n2.18user 0.24system 0:02.42elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+1073minor)pagefaults 0swaps\n$ git ls-files | time ../git/git hash-object --stdin-path > /dev/null\n2.18user 0.25system 0:02.42elapsed 100%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+1073minor)pagefaults 0swaps\n$ git ls-files | time ../git/git hash-object --stdin-path > /dev/null\n2.09user 0.33system 0:02.42elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+1073minor)pagefaults 0swaps\n\n\n sha1_file.c | 22 +++++++---------------\n 1 files changed, 7 insertions(+), 15 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 657825e..83f82a2 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2438,22 +2438,14 @@ int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_object,\n \t enum object_type type, const char *path)\n {\n \tint ret;\n-\tsize_t size = xsize_t(st->st_size);\n+\tstruct strbuf sbuf = STRBUF_INIT;\n \n-\tif (!S_ISREG(st->st_mode)) {\n-\t\tstruct strbuf sbuf = STRBUF_INIT;\n-\t\tif (strbuf_read(&sbuf, fd, 4096) >= 0)\n-\t\t\tret = index_mem(sha1, sbuf.buf, sbuf.len, write_object,\n-\t\t\t\t\ttype, path);\n-\t\telse\n-\t\t\tret = -1;\n-\t\tstrbuf_release(&sbuf);\n-\t} else if (size) {\n-\t\tvoid *buf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);\n-\t\tret = index_mem(sha1, buf, size, write_object, type, path);\n-\t\tmunmap(buf, size);\n-\t} else\n-\t\tret = index_mem(sha1, NULL, size, write_object, type, path);\n+\tif (strbuf_read(&sbuf, fd, 4096) >= 0)\n+\t\tret = index_mem(sha1, sbuf.buf, sbuf.len, write_object,\n+\t\t\t\ttype, path);\n+\telse\n+\t\tret = -1;\n+\tstrbuf_release(&sbuf);\n \tclose(fd);\n \treturn ret;\n }\n-- \n1.7.0\n"},{"id":"134509","messageId":"4B775384.50009@gnu.org","threadId":"22620","inReplyTo":"20100212002741.GB9883@progeny.tock","subject":"mmap with MAP_PRIVATE is useless (was Re: Bug#569505: git-core: 'git add' corrupts repository if the working directory is modified as it runs)","fromName":"Paolo Bonzini","fromEmail":"bonzini@gnu.org","sentAt":"2010-02-14T01:36:04Z","receivedAt":"2010-02-14T01:36:04Z","isPatch":false,"sender":{"key":"bonzini@gnu.org","avatar":"https://avatars.githubusercontent.com/u/42082?v=4"},"body":"\n> I tested this with a number of kernel versions from 2.6.27 to 2.6.31.\n> All of them reproduced this problem. I checked this because strace\n> shows 'git add' doing a mmap(..., MAP_PRIVATE, ...) of its input file,\n> so I was wondering if there might have been a recent change in mmap()\n> behavior in either git or the kernel.\n\n From mmap(2): \"it is unspecified whether changes made to the file after \nthe mmap() call are visible in the mapped region\".\n\nYou may think that doing a dummy \"*p = *p\" every 4096 bytes `fixes' it \n(because it causes copy-on-write for every page) but even that does not \nwork because you can get a SIGBUS if the file is truncated while you \nhave it mapped privately (e.g. by fopen (\"file\", \"w\") or open with O_TRUNC).\n\nTestcase:\n\n#include \n#include \n#include \n\nint\nmain ()\n{\n system (\"echo foo > file.test\");\n int f = open (\"file.test\", O_RDONLY);\n char *p = mmap (NULL, 4096, PROT_READ, MAP_PRIVATE, f, 0);\n close (f);\n printf (\"%s\", p); // prints \"foo\\n\"\n\n f = open (\"file.test\", O_RDWR | O_CREAT | O_TRUNC, 0666);\n write (f, \"bar\\n\", 4); // comment out and next printf SIGSEGVs\n printf (\"%s\", p); // prints \"bar\\n\"\n}\n\nThis means that MAP_PRIVATE is utterly useless.\n\nPaolo\n"},{"id":"134510","messageId":"7vtytk61im.fsf@alter.siamese.dyndns.org","threadId":"22620","inReplyTo":"20100214011812.GA2175@dpotapov.dyndns.org","subject":"Re: [PATCH] don't use mmap() to hash files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-14T01:37:21Z","receivedAt":"2010-02-14T01:37:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dmitry Potapov writes:\n\n> If a mmapped file is changed by another program during git-add, it\n> causes the repository corruption. Disabling mmap() in index_fd() does\n> not have any negative impact on the overall speed of Git. In fact, it\n> makes git hash-object to work slightly faster....\n> ...\n> I think more people should test this change to see its impact on\n> performance. For me, it was positive. Here is my results:\n\nI wasn't particularly impressed by the original problem description, but\nthis is a very interesting result.\n"},{"id":"134512","messageId":"7vy6iw4m6r.fsf@alter.siamese.dyndns.org","threadId":"22620","inReplyTo":"4B775384.50009@gnu.org","subject":"Re: mmap with MAP_PRIVATE is useless","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-14T01:53:48Z","receivedAt":"2010-02-14T01:53:48Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Paolo Bonzini writes:\n\n> This means that MAP_PRIVATE is utterly useless.\n\nI do not think we ever used MAP_PRIVATE in order to protect outselves from\nuncontrolled changes made by the outside world in the first place. Back\nwhen most of these mmap calls were written by Linus and myself, we weren't\ninterested in using MAP_PRIVATE, or any other trick for that matter, to\ndeal with the case where the user tells git to go index a file, and then\nmucks with the file before git finishes and gives back control.\n\nWe do use mmap in read-write mode when reading from the index file, and we\nuse MAP_PRIVATE to protect the outside world from our writing into the\nmapped memory. As far as I know that is the only mmap for which\nMAP_PRIVATE matters in the core git codebase.\n\nOur calls to mmap() almost all have MAP_PRIVATE, even for read-only mmap,\nbut that is more or less from inertia, aka \"an existing call to mmap is\nwith these options, I'll add another call imitating that\".\n"},{"id":"134511","messageId":"alpine.DEB.1.00.1002140249410.20986@pacific.mpi-cbg.de","threadId":"22620","inReplyTo":"20100214011812.GA2175@dpotapov.dyndns.org","subject":"Re: [PATCH] don't use mmap() to hash files","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2010-02-14T01:53:58Z","receivedAt":"2010-02-14T01:53:58Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 14 Feb 2010, Dmitry Potapov wrote:\n\n> +\tif (strbuf_read(&sbuf, fd, 4096) >= 0)\n\nHow certain are you at this point that all of fd's contents fit into your \nmemory?\n\nAnd even if you could be certain, a hint is missing that strbuf_read(), \nits name notwithstanding, does not read NUL-terminated strings. Oh, and \nthe size is just a hint for the initial size, and it reads until EOF. That \nhas to be said in the commit message.\n\nCiao,\nDscho\n"},{"id":"134513","messageId":"7v1vgo4lwa.fsf@alter.siamese.dyndns.org","threadId":"22620","inReplyTo":"alpine.DEB.1.00.1002140249410.20986@pacific.mpi-cbg.de","subject":"Re: [PATCH] don't use mmap() to hash files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-14T02:00:05Z","receivedAt":"2010-02-14T02:00:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> And even if you could be certain, a hint is missing that strbuf_read(), \n> its name notwithstanding, does not read NUL-terminated strings. Oh, and \n> the size is just a hint for the initial size, and it reads until EOF. That \n> has to be said in the commit message.\n\nIt is healthy to ask for explanation, especially when the patch cannot\njustify itself by reading only the diff but reviewer needs to check the\nsurrounding code.\n\nI however think you are asking a little too much in this particular case.\nThat strbuf_read() code is exactly the same as in used for \"reading from\npipe and we don't know how big it is\" case in the original code. You can\nsee it in the lines deleted by the patch.\n"},{"id":"134514","messageId":"4B775BD2.3040007@gnu.org","threadId":"22620","inReplyTo":"7vy6iw4m6r.fsf@alter.siamese.dyndns.org","subject":"Re: mmap with MAP_PRIVATE is useless","fromName":"Paolo Bonzini","fromEmail":"bonzini@gnu.org","sentAt":"2010-02-14T02:11:30Z","receivedAt":"2010-02-14T02:11:30Z","isPatch":false,"sender":{"key":"bonzini@gnu.org","avatar":"https://avatars.githubusercontent.com/u/42082?v=4"},"body":"On 02/14/2010 02:53 AM, Junio C Hamano wrote:\n> Back when most of these mmap calls were written by Linus and myself,\n> we weren't interested in using MAP_PRIVATE, or any other trick for\n> that matter, to deal with the case where the user tells git to go\n> index a file, and then mucks with the file before git finishes and\n> gives back control.\n\nEh, this one in particular (in index_fd) is quite ancient...\n\ncommit e83c5163316f89bfbde7d9ab23ca2e25604af290\nAuthor: Linus Torvalds \nDate: Thu Apr 7 15:13:13 2005 -0700\n\n Initial revision of \"git\", the information manager from hell\n\n:-)\n\nThere were three mmap calls -- in read_sha1_file, read_cache and \nindex_fd -- and all three were of the same mmap (NULL, st.st_size, \nPROT_READ, MAP_PRIVATE, fd, 0) shape.\n\nPaolo\n"},{"id":"134515","messageId":"20100214021847.GA9704@dpotapov.dyndns.org","threadId":"22620","inReplyTo":"7vtytk61im.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] don't use mmap() to hash files","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2010-02-14T02:18:47Z","receivedAt":"2010-02-14T02:18:47Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"On Sat, Feb 13, 2010 at 05:37:21PM -0800, Junio C Hamano wrote:\n> Dmitry Potapov writes:\n> \n> > If a mmapped file is changed by another program during git-add, it\n> > causes the repository corruption. Disabling mmap() in index_fd() does\n> > not have any negative impact on the overall speed of Git. In fact, it\n> > makes git hash-object to work slightly faster....\n> > ...\n> > I think more people should test this change to see its impact on\n> > performance. For me, it was positive. Here is my results:\n> \n> I wasn't particularly impressed by the original problem description, but\n> this is a very interesting result.\n\nMy initial reaction was to write that the whole problem is due to abuse\nGit for the purposes that it was not intended. But then I decided to do\nsome testing to see what impact it has. And, because I do not see any\nnegative impact (in fact, slightly improve speed), and I decided to ask\nother people (who are interested in this patch) to do more testing.\n\n\nDmitry\n"},{"id":"134516","messageId":"20100214024259.GB9704@dpotapov.dyndns.org","threadId":"22620","inReplyTo":"alpine.DEB.1.00.1002140249410.20986@pacific.mpi-cbg.de","subject":"Re: [PATCH] don't use mmap() to hash files","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2010-02-14T02:42:59Z","receivedAt":"2010-02-14T02:42:59Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"On Sun, Feb 14, 2010 at 02:53:58AM +0100, Johannes Schindelin wrote:\n> On Sun, 14 Feb 2010, Dmitry Potapov wrote:\n> \n> > +\tif (strbuf_read(&sbuf, fd, 4096) >= 0)\n> \n> How certain are you at this point that all of fd's contents fit into your \n> memory?\n\nYou can't be sure... In fact, we know mmap() also may fail for huge\nfiles, so can strbuf_read(). Perhaps, mmap() behaves better when you\nwant to hash a huge file that does not fit in free physical memory, but\nI do not think it is an important use case for any VCS, which mostly\nstores small text files and a few not so big binary files. Git is not\ndesign to store your video collection. (probably, Git can be improved to\nhandle big files better but I leave that exercise to those who want to\nstore their media files in Git).\n\n> \n> And even if you could be certain, a hint is missing that strbuf_read(), \n> its name notwithstanding, does not read NUL-terminated strings. Oh, and \n> the size is just a hint for the initial size, and it reads until EOF. That \n> has to be said in the commit message.\n\nI did not add _any_ new code, including the above line. It was there\nbefore my patch. I only removed a few lines for the case when we used\nmmap() and left the code that used strbuf_read() (mmap() was used for\nregular files and strbuf_read() for other type of descriptors).\n\nThe fact that I re-aligned those lines could not introduce any bug, so\nif you think this code incorrect then it was before my patch, but I do\nnot see why. And, I see no reason to comment on the code that does not\nchange at all (including that the third parameter of strbuf_read() is\njust a hint). I may agree with you that strbuf_read with 'hint' is a bit\nconfusing, but it has nothing to do with my patch...\n\n\nDmitry\n"},{"id":"134517","messageId":"20100214030504.GA17952@dpotapov.dyndns.org","threadId":"22620","inReplyTo":"20100214011812.GA2175@dpotapov.dyndns.org","subject":"[PATCH v2] don't use mmap() to hash files","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2010-02-14T03:05:04Z","receivedAt":"2010-02-14T03:05:04Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"If a mmapped file is changed by another program during git-add, it\ncauses the repository corruption. Disabling mmap() in index_fd() does\nnot have any negative impact on the overall speed of Git. In fact, it\nmakes git hash-object to work slightly faster. Here is the best result\nbefore and after patch based on 5 runs on the Linix kernel repository:\n\nBefore:\n\n$ git ls-files | time git hash-object --stdin-path > /dev/null\n2.15user 0.36system 0:02.52elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+103248minor)pagefaults 0swaps\n\nAfter:\n\n$ git ls-files | time ../git/git hash-object --stdin-path > /dev/null\n2.09user 0.33system 0:02.42elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+1073minor)pagefaults 0swaps\n\nSigned-off-by: Dmitry Potapov \n---\n\nIn this version, I have improved the hint value for regular files to\navoid useless re-allocation and copy.\n\n sha1_file.c | 27 +++++++++++----------------\n 1 files changed, 11 insertions(+), 16 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 657825e..26c6231 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2438,22 +2438,17 @@ int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_object,\n \t enum object_type type, const char *path)\n {\n \tint ret;\n-\tsize_t size = xsize_t(st->st_size);\n-\n-\tif (!S_ISREG(st->st_mode)) {\n-\t\tstruct strbuf sbuf = STRBUF_INIT;\n-\t\tif (strbuf_read(&sbuf, fd, 4096) >= 0)\n-\t\t\tret = index_mem(sha1, sbuf.buf, sbuf.len, write_object,\n-\t\t\t\t\ttype, path);\n-\t\telse\n-\t\t\tret = -1;\n-\t\tstrbuf_release(&sbuf);\n-\t} else if (size) {\n-\t\tvoid *buf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);\n-\t\tret = index_mem(sha1, buf, size, write_object, type, path);\n-\t\tmunmap(buf, size);\n-\t} else\n-\t\tret = index_mem(sha1, NULL, size, write_object, type, path);\n+\tstruct strbuf sbuf = STRBUF_INIT;\n+\t/* for regular files, we supply the real file size, otherwise\n+\t `size' is just a hint */\n+\tsize_t size = S_ISREG(st->st_mode) ? xsize_t(st->st_size) : 4096;\n+\n+\tif (strbuf_read(&sbuf, fd, size) >= 0)\n+\t\tret = index_mem(sha1, sbuf.buf, sbuf.len, write_object,\n+\t\t\t\ttype, path);\n+\telse\n+\t\tret = -1;\n+\tstrbuf_release(&sbuf);\n \tclose(fd);\n \treturn ret;\n }\n-- \n1.7.0\n"},{"id":"134518","messageId":"7v8wawy0ee.fsf@alter.siamese.dyndns.org","threadId":"22620","inReplyTo":"20100214021847.GA9704@dpotapov.dyndns.org","subject":"Re: [PATCH] don't use mmap() to hash files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-14T03:14:01Z","receivedAt":"2010-02-14T03:14:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dmitry Potapov writes:\n\n> My initial reaction was to write that the whole problem is due to abuse\n> Git for the purposes that it was not intended. But then I decided to do\n> some testing to see what impact it has. And, because I do not see any\n> negative impact (in fact, slightly improve speed), and I decided to ask\n> other people (who are interested in this patch) to do more testing.\n\nYes, I know, and I greatly appreciate it.\n\nLater, we might want to split the codepath again to:\n\n (0) see if it is huge or small if we are not reading from pipe;\n\n (1) if we do not know the size or if it is moderately tiny, keep doing\n what your code does;\n\n (2) if we know we are reading something huge with known size, then have a\n loop to read-a-bit-compress-and-write-it-out-while-hashing, and\n finally rename the loose resulting object to the final name. Or we\n may even want to do that into a new pack on its own.\n\nbut obviously all of that can come after this initial round ;-)\n"},{"id":"134527","messageId":"m3fx5484ci.fsf@localhost.localdomain","threadId":"22620","inReplyTo":"20100214024259.GB9704@dpotapov.dyndns.org","subject":"Re: [PATCH] don't use mmap() to hash files","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-02-14T11:07:17Z","receivedAt":"2010-02-14T11:07:17Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Dmitry Potapov writes:\n> On Sun, Feb 14, 2010 at 02:53:58AM +0100, Johannes Schindelin wrote:\n> > On Sun, 14 Feb 2010, Dmitry Potapov wrote:\n> > \n> > > +\tif (strbuf_read(&sbuf, fd, 4096) >= 0)\n> > \n> > How certain are you at this point that all of fd's contents fit into your \n> > memory?\n> \n> You can't be sure... In fact, we know mmap() also may fail for huge\n> files, so can strbuf_read(). Perhaps, mmap() behaves better when you\n> want to hash a huge file that does not fit in free physical memory, but\n> I do not think it is an important use case for any VCS, which mostly\n> stores small text files and a few not so big binary files. Git is not\n> design to store your video collection. (probably, Git can be improved to\n> handle big files better but I leave that exercise to those who want to\n> store their media files in Git).\n\nSomething like git-bigfiles: http://caca.zoy.org/wiki/git-bigfiles ?\n\n(found via http://blog.bitquabit.com/2010/02/10/fightings-been-fun-and-all-its-time-shut-and-get-along/\n found via http://tomayko.com/, entry for 10 Feb 2010).\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"134528","messageId":"201002141214.15025.trast@student.ethz.ch","threadId":"22620","inReplyTo":"7v8wawy0ee.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] don't use mmap() to hash files","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2010-02-14T11:14:14Z","receivedAt":"2010-02-14T11:14:14Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"On Sunday 14 February 2010 04:14:01 Junio C Hamano wrote:\n> Later, we might want to split the codepath again to:\n> \n> (0) see if it is huge or small if we are not reading from pipe;\n> \n> (1) if we do not know the size or if it is moderately tiny, keep doing\n> what your code does;\n> \n> (2) if we know we are reading something huge with known size, then have a\n> loop to read-a-bit-compress-and-write-it-out-while-hashing, and\n> finally rename the loose resulting object to the final name. Or we\n> may even want to do that into a new pack on its own.\n\nThere's a slight problem with that code (I tried to finish last\nnight's attempt but got stuck on this):\n\nThe create_tmpfile() and move_temp_to_file() duo goes to some lengths\nto ensure that the file is created in the same directory that we want\nit to end up in. However, in the block-based scheme, you cannot know\nwhich directory this will be before you have already written the\nentire output.\n\nSo again I guess there are a few possible solutions:\n\n* Try the cross-directory rename anyway, but if it doesn't work,\n copy&unlink. This of course means that you may write the same\n object over network twice.\n\n* Declare that keeping the memory usage near what it is today (the\n full output buffer plus a constant) is okay.\n\n* Give up and stick with Dmitry's patch :-)\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"134530","messageId":"7vsk94niok.fsf@alter.siamese.dyndns.org","threadId":"22620","inReplyTo":"201002141214.15025.trast@student.ethz.ch","subject":"Re: [PATCH] don't use mmap() to hash files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-14T11:46:51Z","receivedAt":"2010-02-14T11:46:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast writes:\n\n> * Give up and stick with Dmitry's patch :-)\n\nYou obviously didn't read the last line of my message before responding.\n\nIn any case, I have a suspicion that streaming to a single loose object\nfile would not buy us much (that is the only case the \"cross directory\nrename\" could matter), exactly because we wouldn't want to leave an object\nin loose form if it is so big that we do not want to slurp it in full into\nmemory anyway. If we stream such a huge object directly to a new pack, on\nthe other hand, there won't be any cross directory rename issues.\n"},{"id":"134532","messageId":"4B77E4AC.6030600@gnu.org","threadId":"22620","inReplyTo":"20100214024259.GB9704@dpotapov.dyndns.org","subject":"Re: [PATCH] don't use mmap() to hash files","fromName":"Paolo Bonzini","fromEmail":"bonzini@gnu.org","sentAt":"2010-02-14T11:55:24Z","receivedAt":"2010-02-14T11:55:24Z","isPatch":true,"sender":{"key":"bonzini@gnu.org","avatar":"https://avatars.githubusercontent.com/u/42082?v=4"},"body":"On 02/14/2010 03:42 AM, Dmitry Potapov wrote:\n> In fact, we know mmap() also may fail for huge files, so can\n> strbuf_read().\n\nOn a 64-bit machine mmap should fail pretty much never, as it is limited\nby address space and not available memory.\n\nThat said I can reproduce the result here too, and it's actually quite \nunderstandable from your \"time\" results: mmap has a minor page fault for \nevery 4k you read (or at least that's the order of magnitude), read has \nbasically none. Furthermore, it's true that with read you touch the \nwhole memory twice from beginning to end, while with mmap you touch a \npage twice and move on; however, a file's contents will almost always \nfit in the L2 cache, so it's not too expensive.\n\nI tried madvise(buf, size, MADV_SEQUENTIAL) and MADV_WILLNEED but it has \nno effect.\n\nI suspect mmap will be faster only if the data is not in cache (so the \ncost of a page fault is negligible compared to going to disk) and the \naverage file size is a few megabytes.\n\nPaolo\n"},{"id":"134560","messageId":"alpine.DEB.1.00.1002141908150.20986@pacific.mpi-cbg.de","threadId":"22620","inReplyTo":"20100214024259.GB9704@dpotapov.dyndns.org","subject":"Re: [PATCH] don't use mmap() to hash files","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2010-02-14T18:10:01Z","receivedAt":"2010-02-14T18:10:01Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 14 Feb 2010, Dmitry Potapov wrote:\n\n> On Sun, Feb 14, 2010 at 02:53:58AM +0100, Johannes Schindelin wrote:\n> > On Sun, 14 Feb 2010, Dmitry Potapov wrote:\n> > \n> > > +\tif (strbuf_read(&sbuf, fd, 4096) >= 0)\n> > \n> > How certain are you at this point that all of fd's contents fit into \n> > your memory?\n> \n> You can't be sure... In fact, we know mmap() also may fail for huge\n> files, so can strbuf_read().\n\nThat's comparing oranges to apples. In one case, the address space runs \nout, in the other the available memory. The latter is much more likely.\n\n> > And even if you could be certain, a hint is missing that \n> > strbuf_read(), its name notwithstanding, does not read NUL-terminated \n> > strings. Oh, and the size is just a hint for the initial size, and it \n> > reads until EOF. That has to be said in the commit message.\n> \n> I did not add _any_ new code, including the above line. It was there\n> before my patch.\n\nBut that explanation does not answer my question, does it? And my question \nwas not unreasonable to ask, was it?\n\nCiao,\nDscho\n"},{"id":"134562","messageId":"37fcd2781002141106v761ce6e0kc5c5bdd5001f72a9@mail.gmail.com","threadId":"22620","inReplyTo":"alpine.DEB.1.00.1002141908150.20986@pacific.mpi-cbg.de","subject":"Re: [PATCH] don't use mmap() to hash files","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2010-02-14T19:06:39Z","receivedAt":"2010-02-14T19:06:39Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"On Sun, Feb 14, 2010 at 9:10 PM, Johannes Schindelin\n wrote:\n>\n> On Sun, 14 Feb 2010, Dmitry Potapov wrote:\n>\n>> On Sun, Feb 14, 2010 at 02:53:58AM +0100, Johannes Schindelin wrote:\n>> > On Sun, 14 Feb 2010, Dmitry Potapov wrote:\n>> >\n>> > > + if (strbuf_read(&sbuf, fd, 4096) >= 0)\n>> >\n>> > How certain are you at this point that all of fd's contents fit into\n>> > your memory?\n>>\n>> You can't be sure... In fact, we know mmap() also may fail for huge\n>> files, so can strbuf_read().\n>\n> That's comparing oranges to apples. In one case, the address space runs\n> out, in the other the available memory. The latter is much more likely.\n\n\"much more likely\" is not a very qualitative characteristic... I would\nprefer to see numbers. My gut feeling is that it is not a problem in\nreal use cases where Git is used as VCS and not storage for huge media\nfiles. In fact, we do not use mmap() on Windows or MacOS at all, and\nI have not heard that users of those platforms suffered much more from\ninability to store huge files than those who use Linux. I do not want\nto say that there is no difference, but it may not as large as you try\nto portray. In any case, my patch was to let people to test it and to\nsee what impact it has and come up with some numbers.\n\nBTW, probably, it is not difficult to stream a large file in chunks (and\nit may be even much faster, because we work on CPU cache), but I suspect\nit will not resolve all issues with huge files, because eventually we\nneed to store them in a pack file. So we need to develop some strategy\nhow to deal with them.\n\nOne way to deal with them is to stream directly into a separate pack.\nStill, it does not resolve all problems, because each pack file should\nbe mapped into a memory, and this may be a problem for 32-bit system\n(or even 64-bit systems where a sysadmin set limit on amount virtual\nmemory available a single program).\n\nThe other way to handle huge files is to split them into chunks.\nhttp://article.gmane.org/gmane.comp.version-control.git/120112\n\nMaybe there are other approaches. I heard some people tried to do\nsomething about it, but i have never interested in big files to look\nat this issue closely.\n\n>\n>> > And even if you could be certain, a hint is missing that\n>> > strbuf_read(), its name notwithstanding, does not read NUL-terminated\n>> > strings. Oh, and the size is just a hint for the initial size, and it\n>> > reads until EOF. That has to be said in the commit message.\n>>\n>> I did not add _any_ new code, including the above line. It was there\n>> before my patch.\n>\n> But that explanation does not answer my question, does it?\n\nI believe it did, or I did not understand your question.\n\nDmitry\n"},{"id":"134564","messageId":"alpine.DEB.1.00.1002142021100.20986@pacific.mpi-cbg.de","threadId":"22620","inReplyTo":"37fcd2781002141106v761ce6e0kc5c5bdd5001f72a9@mail.gmail.com","subject":"Re: [PATCH] don't use mmap() to hash files","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2010-02-14T19:22:28Z","receivedAt":"2010-02-14T19:22:28Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 14 Feb 2010, Dmitry Potapov wrote:\n\n> On Sun, Feb 14, 2010 at 9:10 PM, Johannes Schindelin \n> wrote:\n> >\n> > On Sun, 14 Feb 2010, Dmitry Potapov wrote:\n> >\n> >> On Sun, Feb 14, 2010 at 02:53:58AM +0100, Johannes Schindelin wrote:\n> >> > On Sun, 14 Feb 2010, Dmitry Potapov wrote:\n> >> >\n> >> > > + if (strbuf_read(&sbuf, fd, 4096) >= 0)\n> >> >\n> >> > How certain are you at this point that all of fd's contents fit \n> >> > into your memory?\n> >>\n> >> You can't be sure... In fact, we know mmap() also may fail for huge \n> >> files, so can strbuf_read().\n> >\n> > That's comparing oranges to apples. In one case, the address space \n> > runs out, in the other the available memory. The latter is much more \n> > likely.\n> \n> \"much more likely\" is not a very qualitative characteristic...\n\nGit was touted as a \"content tracker\". So I use it as such.\n\nConcrete example: in one of my repositories, the average file size is well \nover 2 gigabytes.\n\nGo figure,\nDscho\n"},{"id":"134563","messageId":"alpine.DEB.1.00.1002142025160.20986@pacific.mpi-cbg.de","threadId":"22620","inReplyTo":"alpine.DEB.1.00.1002142021100.20986@pacific.mpi-cbg.de","subject":"Re: [PATCH] don't use mmap() to hash files","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2010-02-14T19:28:28Z","receivedAt":"2010-02-14T19:28:28Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 14 Feb 2010, Johannes Schindelin wrote:\n\n> On Sun, 14 Feb 2010, Dmitry Potapov wrote:\n> \n> > On Sun, Feb 14, 2010 at 9:10 PM, Johannes Schindelin \n> > wrote:\n> > >\n> > > On Sun, 14 Feb 2010, Dmitry Potapov wrote:\n> > >\n> > >> On Sun, Feb 14, 2010 at 02:53:58AM +0100, Johannes Schindelin \n> > >> wrote:\n> > >> > On Sun, 14 Feb 2010, Dmitry Potapov wrote:\n> > >> >\n> > >> > > + if (strbuf_read(&sbuf, fd, 4096) >= 0)\n> > >> >\n> > >> > How certain are you at this point that all of fd's contents fit \n> > >> > into your memory?\n> > >>\n> > >> You can't be sure... In fact, we know mmap() also may fail for huge \n> > >> files, so can strbuf_read().\n> > >\n> > > That's comparing oranges to apples. In one case, the address space \n> > > runs out, in the other the available memory. The latter is much more \n> > > likely.\n> > \n> > \"much more likely\" is not a very qualitative characteristic...\n> \n> Git was touted as a \"content tracker\". So I use it as such.\n> \n> Concrete example: in one of my repositories, the average file size is \n> well over 2 gigabytes.\n\nJust to make extremely sure that you undertand the issue: adding these \nfiles on a computer with 512 megabyte RAM works at the moment. Can you \nguarantee that there is no regression in that respect _with_ your patch?\n\nGit is in wide use. If you provide a patch, it is not good enough anymore \nif it Works For You(tm), when it Does Not Work Somewhere Else Anymore(tm).\n\nCiao,\nDscho\n"},{"id":"134566","messageId":"37fcd2781002141155r1af33e54w41422dea57661185@mail.gmail.com","threadId":"22620","inReplyTo":"alpine.DEB.1.00.1002142021100.20986@pacific.mpi-cbg.de","subject":"Re: [PATCH] don't use mmap() to hash files","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2010-02-14T19:55:12Z","receivedAt":"2010-02-14T19:55:12Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"On Sun, Feb 14, 2010 at 10:22 PM, Johannes Schindelin\n wrote:\n>\n> Git was touted as a \"content tracker\". So I use it as such.\n>\n\nWell... those who got their Git repository corrupted also used Git as a\n\"content tracker\". But I believe Git is a content tracker primary for\nsource code, thus using it as a backup tool or to store your multi-media\ncollection is abuse of Git... Having said so, I have no objection that\nGit could be used in those areas too... So, I would like to hear any\nspecific suggestion how to achieve those goals better...\n\n\nDmitry\n"},{"id":"134567","messageId":"37fcd2781002141156n7e2b9673s1eb6c12869facdb2@mail.gmail.com","threadId":"22620","inReplyTo":"alpine.DEB.1.00.1002142025160.20986@pacific.mpi-cbg.de","subject":"Re: [PATCH] don't use mmap() to hash files","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2010-02-14T19:56:46Z","receivedAt":"2010-02-14T19:56:46Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"On Sun, Feb 14, 2010 at 10:28 PM, Johannes Schindelin\n wrote:\n>>\n>> Concrete example: in one of my repositories, the average file size is\n>> well over 2 gigabytes.\n>\n> Just to make extremely sure that you undertand the issue: adding these\n> files on a computer with 512 megabyte RAM works at the moment. Can you\n> guarantee that there is no regression in that respect _with_ your patch?\n\nIt may not work without enough swap space, and it will not pretty anyway\ndue to swapping. So, I see the following options:\n\n1. to introduce a configuration parameter that will define whether to use\nmmap() to hash files or not. It is a trivial change, but the real question\nis what default value for this option (should we do some heuristic based\non filesize vs available memory?)\n\n2. to stream files in chunks. It is better because it is faster, especially on\nlarge files, as you calculate SHA-1 and zip data while they are in CPU\ncache. However, it may be more difficult to implement, because we have\nfilters that should be apply to files that are put to the repository.\n\n3. to improve Git to support huge files on computers with low memory.\n\nI think #3 is a noble goal, but I do not have time for that. I can try to take\non #2, but it may take more time than I have now. As to #1, I am ready to\nsend the patch if we agree that is the right way to go...\n\nI am open to any your suggestion. Maybe there are some options here..\n\n\nDmitry\n"},{"id":"134581","messageId":"32541b131002141513m29f9a796ma8fb5855a45f91e9@mail.gmail.com","threadId":"22620","inReplyTo":"37fcd2781002141106v761ce6e0kc5c5bdd5001f72a9@mail.gmail.com","subject":"Re: [PATCH] don't use mmap() to hash files","fromName":"Avery Pennarun","fromEmail":"apenwarr@gmail.com","sentAt":"2010-02-14T23:13:13Z","receivedAt":"2010-02-14T23:13:13Z","isPatch":true,"sender":{"key":"apenwarr@gmail.com","avatar":"https://avatars.githubusercontent.com/u/20592?v=4"},"body":"On Sun, Feb 14, 2010 at 2:06 PM, Dmitry Potapov wrote:\n> On Sun, Feb 14, 2010 at 9:10 PM, Johannes Schindelin\n> wrote:\n>> That's comparing oranges to apples. In one case, the address space runs\n>> out, in the other the available memory. The latter is much more likely.\n>\n> \"much more likely\" is not a very qualitative characteristic... I would\n> prefer to see numbers.\n\nWell, the numbers are rather easy to calculate of course. On a 32-bit\nmachine, your (ideal) maximum address space size is 4GB. On a 64-bit\nmachine, it's a heck of a lot bigger. And in either case, a single\nprocess consuming it all doesn't matter since it won't hurt other\nprocesses. But the available RAM is frequently less than 4GB and that\nhas to be shared between *all* your processes.\n\n> BTW, probably, it is not difficult to stream a large file in chunks (and\n> it may be even much faster, because we work on CPU cache), but I suspect\n> it will not resolve all issues with huge files, because eventually we\n> need to store them in a pack file. So we need to develop some strategy\n> how to deal with them.\n\nIt definitely doesn't resolve all the issues. There are different\nways of looking at this; one is to not bother make git-add work\nsmoothly with large files, because calculating the deltas will later\ncause a disastrous meltdown anyway. In fact, arguably you should\nprevent git-add from adding large files at all, because at least then\nyou don't get the repository into a hard-to-recover-from state with\nhuge files. (This happened at work a few months ago; most people have\nno idea what to do in such a situation.)\n\nThe other way to look at it is that if we want git to *eventually*\nwork with huge files, we have to fix each bug one at a time, and we\ncan't go making things worse.\n\nFor my own situation, I think I'm more likely to (and I know people\nwho are more likely to) try storing huge files in git than I am likely\nto modify a file *while* I'm trying to store it in git.\n\n> One way to deal with them is to stream directly into a separate pack.\n> Still, it does not resolve all problems, because each pack file should\n> be mapped into a memory, and this may be a problem for 32-bit system\n> (or even 64-bit systems where a sysadmin set limit on amount virtual\n> memory available a single program).\n>\n> The other way to handle huge files is to split them into chunks.\n> http://article.gmane.org/gmane.comp.version-control.git/120112\n\nI have a bit of experience splitting files into chunks:\nhttp://groups.google.com/group/bup-list/browse_thread/thread/812031efd4c5f7e4\n\nIt works. Also note that the speed gain from mmap'ing packs appears\nto be much less than the gain from mmap'ing indexes. You could\nprobably sacrifice most or all of the former and never really notice.\nCaching expanded deltas can be pretty valuable, though. (bup\npresently avoids that whole question by not using deltas.)\n\nI can also confirm that streaming objects directly into packs is a\nmassive performance increase when dealing with big files. However,\nyou then start to run into git's heuristics that often assume (for\nexample) that if an object is in a pack, it should never (or rarely)\nbe pruned. This is normally a fine assumption, because if it was\nlikely to get pruned, it probably never would have been put into a\npack in the first place.\n\nHave fun,\n\nAvery\n"},{"id":"134582","messageId":"20100214235219.GE24809@gibbs.hungrycats.org","threadId":"22620","inReplyTo":"37fcd2781002141156n7e2b9673s1eb6c12869facdb2@mail.gmail.com","subject":"Re: [PATCH] don't use mmap() to hash files","fromName":"Zygo Blaxell","fromEmail":"zblaxell@esightcorp.com","sentAt":"2010-02-14T23:52:19Z","receivedAt":"2010-02-14T23:52:19Z","isPatch":true,"sender":{"key":"zblaxell@esightcorp.com","avatar":null},"body":"On Sun, Feb 14, 2010 at 10:56:46PM +0300, Dmitry Potapov wrote:\n> It may not work without enough swap space, and it will not pretty anyway\n> due to swapping. So, I see the following options:\n> \n> 1. to introduce a configuration parameter that will define whether to use\n> mmap() to hash files or not. It is a trivial change, but the real question\n> is what default value for this option (should we do some heuristic based\n> on filesize vs available memory?)\n\nI'm a fan of having the option. It lets users who know what they're\ndoing (and who might be doing something unusual, like files that are\nlarge relative to memory size, or files that are being modified during\ngit add) decide how to make the performance-robustness trade-off.\n\n\"Large\" is relative. How common is it to add files to git that are\nlarge relative to the RAM size of the machine? I have a machine with 6GB\nof RAM and a repo on that machine with half a dozen 0.5-1.5GB files in it.\nGit add is painfully slow on that repo even with mmap(). I wouldn't dare\ntrying to manipulate that repo on a machine with less than 1GB of RAM.\n"},{"id":"134597","messageId":"alpine.LFD.2.00.1002142252020.1946@xanadu.home","threadId":"22620","inReplyTo":"32541b131002141513m29f9a796ma8fb5855a45f91e9@mail.gmail.com","subject":"Re: [PATCH] don't use mmap() to hash files","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2010-02-15T04:16:37Z","receivedAt":"2010-02-15T04:16:37Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Sun, 14 Feb 2010, Avery Pennarun wrote:\n\n> On Sun, Feb 14, 2010 at 2:06 PM, Dmitry Potapov wrote:\n> > BTW, probably, it is not difficult to stream a large file in chunks (and\n> > it may be even much faster, because we work on CPU cache), but I suspect\n> > it will not resolve all issues with huge files, because eventually we\n> > need to store them in a pack file. So we need to develop some strategy\n> > how to deal with them.\n> \n> It definitely doesn't resolve all the issues. There are different\n> ways of looking at this; one is to not bother make git-add work\n> smoothly with large files, because calculating the deltas will later\n> cause a disastrous meltdown anyway.\n\nWe have that core.bigFileThreshold configuration variable now. It is \ncurrently only used by fast-import. But the idea is to apply it across \nthe board when that makes sense. And one of those places is to skip \nover those files when it comes to delta compression.\n\n> In fact, arguably you should prevent git-add from adding large files \n> at all, because at least then you don't get the repository into a \n> hard-to-recover-from state with huge files. (This happened at work a \n> few months ago; most people have no idea what to do in such a \n> situation.)\n\nGit needs to be fixed in that case, not be crippled.\n\n> The other way to look at it is that if we want git to *eventually*\n> work with huge files, we have to fix each bug one at a time, and we\n> can't go making things worse.\n\nObviously.\n\n> For my own situation, I think I'm more likely to (and I know people\n> who are more likely to) try storing huge files in git than I am likely\n> to modify a file *while* I'm trying to store it in git.\n\nAnd fancy operations on huge files are pretty unlikely. Blame, diff, \netc, are suited for text file which are by nature relatively small. \nAnd if your source code is all pasted in one single huge file that Git \ncan't handle right now, then the compiler is unlikely to cope either.\n\n> > One way to deal with them is to stream directly into a separate pack.\n> > Still, it does not resolve all problems, because each pack file should\n> > be mapped into a memory, and this may be a problem for 32-bit system\n> > (or even 64-bit systems where a sysadmin set limit on amount virtual\n> > memory available a single program).\n\nThis is not a problem at all. Git already deals with packs bigger than \nthe available memory by not mapping them whole -- see documentation for \ncore.packedGitWindowSize.\n\n> > The other way to handle huge files is to split them into chunks.\n> > http://article.gmane.org/gmane.comp.version-control.git/120112\n\nNo. The chunk idea doesn't fit the Git model well enough without many \ncorner cases all over the place which is a major drawback. I think that \nwas discussed in that thread already.\n\n> I have a bit of experience splitting files into chunks:\n> http://groups.google.com/group/bup-list/browse_thread/thread/812031efd4c5f7e4\n> \n> It works. Also note that the speed gain from mmap'ing packs appears\n> to be much less than the gain from mmap'ing indexes. You could\n> probably sacrifice most or all of the former and never really notice.\n> Caching expanded deltas can be pretty valuable, though. (bup\n> presently avoids that whole question by not using deltas.)\n\nWe do have a cache of expanded deltas already.\n\n> I can also confirm that streaming objects directly into packs is a\n> massive performance increase when dealing with big files. However,\n> you then start to run into git's heuristics that often assume (for\n> example) that if an object is in a pack, it should never (or rarely)\n> be pruned. This is normally a fine assumption, because if it was\n> likely to get pruned, it probably never would have been put into a\n> pack in the first place.\n\nWould you please for my own sanity tell me where we do such thing. I \nthought I had a firm grip on the pack model but you're casting a shadow \nof doubts on some code I might have written myself.\n\n\nNicolas\n"},{"id":"134605","messageId":"32541b131002142101i226663cfk90d1ba14f1031788@mail.gmail.com","threadId":"22620","inReplyTo":"alpine.LFD.2.00.1002142252020.1946@xanadu.home","subject":"Re: [PATCH] don't use mmap() to hash files","fromName":"Avery Pennarun","fromEmail":"apenwarr@gmail.com","sentAt":"2010-02-15T05:01:49Z","receivedAt":"2010-02-15T05:01:49Z","isPatch":true,"sender":{"key":"apenwarr@gmail.com","avatar":"https://avatars.githubusercontent.com/u/20592?v=4"},"body":"On Sun, Feb 14, 2010 at 11:16 PM, Nicolas Pitre wrote:\n> On Sun, 14 Feb 2010, Avery Pennarun wrote:\n>> In fact, arguably you should prevent git-add from adding large files\n>> at all, because at least then you don't get the repository into a\n>> hard-to-recover-from state with huge files.  (This happened at work a\n>> few months ago; most people have no idea what to do in such a\n>> situation.)\n>\n> Git needs to be fixed in that case, not be crippled.\n\nThat would be ideal, but is more work than disabling imports for large\nfiles by default (for example), which would be easy. In any case, my\nsolution at work was to say \"if it hurts, don't do that\" and it seems\nto have worked out okay for now.\n\n>> For my own situation, I think I'm more likely to (and I know people\n>> who are more likely to) try storing huge files in git than I am likely\n>> to modify a file *while* I'm trying to store it in git.\n>\n> And fancy operations on huge files are pretty unlikely.  Blame, diff,\n> etc, are suited for text file which are by nature relatively small.\n> And if your source code is all pasted in one single huge file that Git\n> can't handle right now, then the compiler is unlikely to cope either.\n\nWell, I'm thinking of things like textual database dumps, such as\nthose produced by mysqldump. It would be nice to be able to diff\nthose efficiently, even if they're several gigs in size. bup's\nhierarchical chunking allows this.\n\n>> > The other way to handle huge files is to split them into chunks.\n>> > http://article.gmane.org/gmane.comp.version-control.git/120112\n>\n> No.  The chunk idea doesn't fit the Git model well enough without many\n> corner cases all over the place which is a major drawback.  I think that\n> was discussed in that thread already.\n>\n>> I have a bit of experience splitting files into chunks:\n>> http://groups.google.com/group/bup-list/browse_thread/thread/812031efd4c5f7e4\n\nNote that bup's rolling-checksum-based hierarchical chunking is not\nthe same as the chunking that was discussed in that thread, and it\nresolves most of the problems. Unless I'm missing something.\n\nAlso note that bup just uses normal tree objects (for better or worse)\ninstead of introducing a new object type.\n\n>> It works.  Also note that the speed gain from mmap'ing packs appears\n>> to be much less than the gain from mmap'ing indexes.  You could\n>> probably sacrifice most or all of the former and never really notice.\n>> Caching expanded deltas can be pretty valuable, though.  (bup\n>> presently avoids that whole question by not using deltas.)\n>\n> We do have a cache of expanded deltas already.\n\nYes, sorry to have implied otherwise. I was just comparing the\nperformance advantage of the delta expansion cache (which should be a\nlot) with that of mmaping packfiles (which probably isn't much since\nthe packfile data is typically needed in expanded form anyway).\n\n>> I can also confirm that streaming objects directly into packs is a\n>> massive performance increase when dealing with big files.  However,\n>> you then start to run into git's heuristics that often assume (for\n>> example) that if an object is in a pack, it should never (or rarely)\n>> be pruned.  This is normally a fine assumption, because if it was\n>> likely to get pruned, it probably never would have been put into a\n>> pack in the first place.\n>\n> Would you please for my own sanity tell me where we do such thing.  I\n> thought I had a firm grip on the pack model but you're casting a shadow\n> of doubts on some code I might have written myself.\n\nSorry, I didn't hunt down the code, but I ran into it while\nexperimenting before. The rules are something like:\n\n- git-prune only prunes unpacked objects\n\n- git-repack claims to be willing to explode unreachable objects back\ninto loose objects with -A, but I'm not quite sure if its definition\nof \"unreachable\" is the same as mine. And I'm not sure rewriting a\npack with -A makes the old pack reliably unreachable according to -d.\nIt's possible I was just being dense.\n\n- there seems to be no documented situation in which you can ever\ndelete unused objects from a pack without using repack -a or -A, which\ncan be amazingly slow if your packs are huge. (Ideally you'd only\nrepack the particular packs that you want to shrink.) For example, my\nbup repo is currently 200 GB.\n\nAnyway, I didn't have much luck when playing with it earlier, but\ndidn't investigate since I assumed it's just a workflow that nobody\nmuch cares about. Which I think is a reasonable position for git\ndevelopers to take anyway.\n\nHave fun,\n\nAvery\n"},{"id":"134607","messageId":"alpine.LFD.2.00.1002142328310.1946@xanadu.home","threadId":"22620","inReplyTo":"37fcd2781002141156n7e2b9673s1eb6c12869facdb2@mail.gmail.com","subject":"Re: [PATCH] don't use mmap() to hash files","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2010-02-15T05:05:37Z","receivedAt":"2010-02-15T05:05:37Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Sun, 14 Feb 2010, Dmitry Potapov wrote:\n\n> 1. to introduce a configuration parameter that will define whether to use\n> mmap() to hash files or not. It is a trivial change, but the real question\n> is what default value for this option (should we do some heuristic based\n> on filesize vs available memory?)\n\nI don't like such kind of heuristic. They're almost always wrong, and \nany issue is damn hard to reproduce. I tend to believe that mmap() works \nbetter by letting the OS paging in and out memory as needed while \nreading data into allocated memory is only going to force the system \ninto swap.\n\n> 2. to stream files in chunks. It is better because it is faster, especially on\n> large files, as you calculate SHA-1 and zip data while they are in CPU\n> cache. However, it may be more difficult to implement, because we have\n> filters that should be apply to files that are put to the repository.\n\nSo? \"More difficult\" when it is the right thing to do is no excuse not \nto do it and satisfy ourselves with an half solution. Barely replacing \nmmap() with read() has drawbacks while the advantages aren't that many. \nGaining a few speed percentage while making it less robust when memory \nis tight isn't such a great compromize to me. BUT if you were to \nreplace mmap() with read() and make the process chunked then you do \nimprove both speed _and_ memory usage.\n\nAs to huge file: we have that core.bigFileThreshold variable now, and \nanything that crosses it should be considered \"stream in / stream out\" \nwithout further considerations. That means no diff, no rename \nsimilarity estimates, no delta, no filter, no blame, no fancies. If you \nhave source code files that big then you do have a bigger problem \nalready anyway. Typical huge files are rarely manipulated, and when \nthey do it is pretty unlikely to be compared with other versions using \ndiff, and then that also means that you have the storage capacity and \nnetwork bandwidth to deal with them. Hence repository tightness is not \nyour top concern in that case, but repack/checkout speed most likely is.\n\nSo big files should be streamed to a pack of their own at \"git add\" \ntime. Then repack will simply \"reuse pack data\" without delta \ncompression attempts, meaning that they will be streamed into a \nsingle huge pack with no issue (this particular case is already \nsupported in the code).\n\n> 3. to improve Git to support huge files on computers with low memory.\n\nThat comes for free with #2.\n\n\nNicolas\n"},{"id":"134617","messageId":"alpine.LFD.2.00.1002150016110.1946@xanadu.home","threadId":"22620","inReplyTo":"32541b131002142101i226663cfk90d1ba14f1031788@mail.gmail.com","subject":"Re: [PATCH] don't use mmap() to hash files","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2010-02-15T05:48:41Z","receivedAt":"2010-02-15T05:48:41Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Mon, 15 Feb 2010, Avery Pennarun wrote:\n\n> - git-prune only prunes unpacked objects\n> \n> - git-repack claims to be willing to explode unreachable objects back\n> into loose objects with -A, but I'm not quite sure if its definition\n> of \"unreachable\" is the same as mine. \n\nUnreachable means not referenced by the specified rev-list \nspecification. So if you give it --all --reflog then it means any \nobjects that is not reachable through either your branches, tags or \nreflog entries.\n\n> And I'm not sure rewriting a\n> pack with -A makes the old pack reliably unreachable according to -d.\n\nReachability doesn't apply to packs. That applies to objects. And \nunreachable objects may be copied to loose objects with -A, or simply \nforgotten about with -a. Then -d will literally delete the old pack \nfile.\n\n> - there seems to be no documented situation in which you can ever\n> delete unused objects from a pack without using repack -a or -A, which\n> can be amazingly slow if your packs are huge. (Ideally you'd only\n> repack the particular packs that you want to shrink.) For example, my\n> bup repo is currently 200 GB.\n\nIdeally you don't keep volatile objects into huge packs. That's why we \nhave .keep to flag those packs that are huge and pure so not to touch \nthem anymore.\n\nIncremental repacking is there to gather only those _reachable_ loose \nobjects into a new pack. The objects that you're likely to make \nunreachable are probably going to come from a temporary branch that you \ndeleted which is likely to affect objects only from that latest and \nsmall pack.\n\nAnd repacking can be done unattended and in parallel to normal Git \noperations with no issues. So even if it is slow to repack huge packs, \nit is something that you might do during the night and only once in a \nwhile.\n\nBut if you really want to shrink only one pack without touching the \nother packs, and you do know which objects have to be removed from that \npack, then it is trivial to write a small script using git-show-index, \nsorting the output by offset, filter out the unwanted objects, keeping \nonly the SHA1 column, and feeding the result into git-pack-objects. Oh \nand delete the original pack when done of course. It is also trivial to \ngenerate the list of all packed objects, compare it to the list of all \nreachable objects, and prune objects from the packs that contains those \nobjects which are not to be found in the reachable object list.\n\n\nNicolas\n"},{"id":"134624","messageId":"4B78FC36.1090108@gnu.org","threadId":"22620","inReplyTo":"37fcd2781002141156n7e2b9673s1eb6c12869facdb2@mail.gmail.com","subject":"Re: [PATCH] don't use mmap() to hash files","fromName":"Paolo Bonzini","fromEmail":"bonzini@gnu.org","sentAt":"2010-02-15T07:48:06Z","receivedAt":"2010-02-15T07:48:06Z","isPatch":true,"sender":{"key":"bonzini@gnu.org","avatar":"https://avatars.githubusercontent.com/u/42082?v=4"},"body":"On 02/14/2010 08:56 PM, Dmitry Potapov wrote:\n> It may not work without enough swap space\n\nNo, the kernel will use the file itself as \"swap\" if it has to page out \nthe memory (since the mmap is PROT_READ).\n\nPaolo\n"},{"id":"134636","messageId":"37fcd2781002150423n36378105t9bee9c0e5106ca4c@mail.gmail.com","threadId":"22620","inReplyTo":"alpine.LFD.2.00.1002142328310.1946@xanadu.home","subject":"Re: [PATCH] don't use mmap() to hash files","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2010-02-15T12:23:59Z","receivedAt":"2010-02-15T12:23:59Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"On Mon, Feb 15, 2010 at 8:05 AM, Nicolas Pitre wrote:\n> On Sun, 14 Feb 2010, Dmitry Potapov wrote:\n>\n>> 1. to introduce a configuration parameter that will define whether to use\n>> mmap() to hash files or not. It is a trivial change, but the real question\n>> is what default value for this option (should we do some heuristic based\n>> on filesize vs available memory?)\n>\n> I don't like such kind of heuristic.  They're almost always wrong, and\n> any issue is damn hard to reproduce. I tend to believe that mmap() works\n> better by letting the OS paging in and out memory as needed while\n> reading data into allocated memory is only going to force the system\n> into swap.\n\nProbably, you are right. Heuristic is a bad idea. Still, we may want to\nadd an option to disable mmap() during hash calculation if we preserve\nmmap() here. Though, I don't like keeping mmap() there if we go for #2..\nSee below...\n\n>\n>> 2. to stream files in chunks. It is better because it is faster, especially on\n>> large files, as you calculate SHA-1 and zip data while they are in CPU\n>> cache. However, it may be more difficult to implement, because we have\n>> filters that should be apply to files that are put to the repository.\n>\n> So?  \"More difficult\" when it is the right thing to do is no excuse not\n> to do it and satisfy ourselves with an half solution.  Barely replacing\n> mmap() with read() has drawbacks while the advantages aren't that many.\n> Gaining a few speed percentage while making it less robust when memory\n> is tight isn't such a great compromize to me.  BUT if you were to\n> replace mmap() with read() and make the process chunked then you do\n> improve both speed _and_ memory usage.\n\nI have not had time to look closely at this, but there is one problem\nthat I noticed -- the header of any git object contains the blob length.\nWe know this length in advance (without reading all data) only for\nregular files and only if they do not have any filter to be applied.\nIn all other cases, it seems we cannot do much better than we do now,\nassuming that we do not want to change the storage format...\n\nIf so, the question remains what to do about regular files with some\nfilter. Currently, we use mmap() for the original data but store the\nprocessed data in memory anyway. The question is whether want to keep\nthis use of mmap() here? Considering that it is a potential source of a\nrepository corruption and these filters should not be used for big files\nbecause they take a lot of memory anyway, I think we should get rid of\nmmap() in hashing file completely, once we can process regular files\nwithout filters in chunks.\n\n\nDmitry\n"},{"id":"134637","messageId":"37fcd2781002150425od069e75ueaff424e2357d78b@mail.gmail.com","threadId":"22620","inReplyTo":"4B78FC36.1090108@gnu.org","subject":"Re: [PATCH] don't use mmap() to hash files","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2010-02-15T12:25:27Z","receivedAt":"2010-02-15T12:25:27Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"On Mon, Feb 15, 2010 at 10:48 AM, Paolo Bonzini wrote:\n> On 02/14/2010 08:56 PM, Dmitry Potapov wrote:\n>>\n>> It may not work without enough swap space\n>\n> No, the kernel will use the file itself as \"swap\" if it has to page out the\n> memory (since the mmap is PROT_READ).\n\nI was speaking about read(), when the whole file is read in memory. In\nthis case, you are going to use a lot of \"swap\" and it is not pretty...\nThat's why I said maybe we should have an option here...\n\n\nDmitry\n"},{"id":"134653","messageId":"32541b131002151119o2f528ddv147d71d12d9d11fe@mail.gmail.com","threadId":"22620","inReplyTo":"alpine.LFD.2.00.1002150016110.1946@xanadu.home","subject":"Re: [PATCH] don't use mmap() to hash files","fromName":"Avery Pennarun","fromEmail":"apenwarr@gmail.com","sentAt":"2010-02-15T19:19:48Z","receivedAt":"2010-02-15T19:19:48Z","isPatch":true,"sender":{"key":"apenwarr@gmail.com","avatar":"https://avatars.githubusercontent.com/u/20592?v=4"},"body":"On Mon, Feb 15, 2010 at 12:48 AM, Nicolas Pitre wrote:\n> Ideally you don't keep volatile objects into huge packs.  That's why we\n> have .keep to flag those packs that are huge and pure so not to touch\n> them anymore.\n\nOf course the problem here is that as soon as you import a single\n(possibly volatile) 2GB file, your pack becomes \"huge.\" So these\nheuristics stop working very well and start to need some revision.\n\nThanks for the clarification on packing and repacking. I might be\nable to use this method to come up with a better repacking/pruning\nalgorithm in bup, and from there to perhaps forward it on to git.\n\nHave fun,\n\nAvery\n"},{"id":"134654","messageId":"alpine.LFD.2.00.1002151426350.1946@xanadu.home","threadId":"22620","inReplyTo":"32541b131002151119o2f528ddv147d71d12d9d11fe@mail.gmail.com","subject":"Re: [PATCH] don't use mmap() to hash files","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2010-02-15T19:29:56Z","receivedAt":"2010-02-15T19:29:56Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Mon, 15 Feb 2010, Avery Pennarun wrote:\n\n> On Mon, Feb 15, 2010 at 12:48 AM, Nicolas Pitre wrote:\n> > Ideally you don't keep volatile objects into huge packs.  That's why we\n> > have .keep to flag those packs that are huge and pure so not to touch\n> > them anymore.\n> \n> Of course the problem here is that as soon as you import a single\n> (possibly volatile) 2GB file, your pack becomes \"huge.\" So these\n> heuristics stop working very well and start to need some revision.\n\nYou don't have to repack that often though. In which case the \nsingle-object 2GB pack might be discarded before the next repack. And \nloose objects are packed into a pack of their own more often than \nmultiple packs being repacked into a single pack. So I think the \ncurrent heuristics should still work pretty well.\n\n\nNicolas\n"},{"id":"134907","messageId":"7vljer1gyg.fsf_-_@alter.siamese.dyndns.org","threadId":"22620","inReplyTo":"20100214011812.GA2175@dpotapov.dyndns.org","subject":"[PATCH] Teach \"git add\" and friends to be paranoid","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-18T01:16:23Z","receivedAt":"2010-02-18T01:16:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When creating a loose object, we normally mmap(2) the entire file, and\nhash and then compress to write it out in two separate steps for\nefficiency.\n\nThis is perfectly good for the intended use of git---nobody is supposed to\nbe insane enough to expect that it won't break anything to muck with the\ncontents of a file after telling git to index it and before getting the\ncontrol back from git.\n\nBut the nature of breakage caused by such an abuse is rather bad. We will\nend up with loose object files, whose names do not match what are stored\nand recovered when uncompressed.\n\nThis teaches the index_mem() codepath to be paranoid and hash and compress\nthe data after reading it in core. The contents hashed may not match the\ncontents of the file in an insane use case, but at least this way the\nresult will be internally consistent.\n\nSigned-off-by: Junio C Hamano \n---\n sha1_file.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++-----------\n 1 files changed, 66 insertions(+), 15 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 657825e..d8a7722 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2278,7 +2278,8 @@ static int create_tmpfile(char *buffer, size_t bufsiz, const char *filename)\n }\n \n static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,\n-\t\t\t void *buf, unsigned long len, time_t mtime)\n+\t\t\t void *buf, unsigned long len, time_t mtime,\n+\t\t\t int paranoid)\n {\n \tint fd, ret;\n \tsize_t size;\n@@ -2286,6 +2287,7 @@ static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,\n \tz_stream stream;\n \tchar *filename;\n \tstatic char tmpfile[PATH_MAX];\n+\tgit_SHA_CTX ctx;\n \n \tfilename = sha1_file_name(sha1);\n \tfd = create_tmpfile(tmpfile, sizeof(tmpfile), filename);\n@@ -2312,12 +2314,41 @@ static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,\n \tstream.next_in = (unsigned char *)hdr;\n \tstream.avail_in = hdrlen;\n \twhile (deflate(&stream, 0) == Z_OK)\n-\t\t/* nothing */;\n+\t\t; /* nothing */\n \n \t/* Then the data itself.. */\n-\tstream.next_in = buf;\n-\tstream.avail_in = len;\n-\tret = deflate(&stream, Z_FINISH);\n+\tif (paranoid) {\n+\t\tunsigned char stablebuf[262144];\n+\t\tchar *bufptr = buf;\n+\t\tunsigned long remainder = len;\n+\n+\t\tgit_SHA1_Init(&ctx);\n+\t\tgit_SHA1_Update(&ctx, hdr, hdrlen);\n+\n+\t\tret = Z_OK;\n+\t\twhile (remainder) {\n+\t\t\tunsigned long chunklen = remainder;\n+\n+\t\t\tif (sizeof(stablebuf) <= chunklen)\n+\t\t\t\tchunklen = sizeof(stablebuf);\n+\t\t\tmemcpy(stablebuf, bufptr, chunklen);\n+\t\t\tgit_SHA1_Update(&ctx, stablebuf, chunklen);\n+\t\t\tstream.next_in = stablebuf;\n+\t\t\tstream.avail_in = chunklen;\n+\t\t\tdo {\n+\t\t\t\tret = deflate(&stream, Z_NO_FLUSH);\n+\t\t\t} while (ret == Z_OK);\n+\t\t\tbufptr += chunklen;\n+\t\t\tremainder -= chunklen;\n+\t\t}\n+\t\tif (ret != Z_STREAM_END)\n+\t\t\tret = deflate(&stream, Z_FINISH);\n+\t} else {\n+\t\tstream.next_in = buf;\n+\t\tstream.avail_in = len;\n+\t\tret = deflate(&stream, Z_FINISH);\n+\t}\n+\n \tif (ret != Z_STREAM_END)\n \t\tdie(\"unable to deflate new object %s (%d)\", sha1_to_hex(sha1), ret);\n \n@@ -2327,6 +2358,12 @@ static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,\n \n \tsize = stream.total_out;\n \n+\tif (paranoid) {\n+\t\tunsigned char paranoid_sha1[20];\n+\t\tgit_SHA1_Final(paranoid_sha1, &ctx);\n+\t\tif (hashcmp(paranoid_sha1, sha1))\n+\t\t\tdie(\"hashed file is volatile\");\n+\t}\n \tif (write_buffer(fd, compressed, size) < 0)\n \t\tdie(\"unable to write sha1 file\");\n \tclose_sha1_file(fd);\n@@ -2344,7 +2381,7 @@ static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,\n \treturn move_temp_to_file(tmpfile, filename);\n }\n \n-int write_sha1_file(void *buf, unsigned long len, const char *type, unsigned char *returnsha1)\n+static int write_sha1_file_paranoid(void *buf, unsigned long len, const char *type, unsigned char *returnsha1, int paranoid)\n {\n \tunsigned char sha1[20];\n \tchar hdr[32];\n@@ -2358,7 +2395,12 @@ int write_sha1_file(void *buf, unsigned long len, const char *type, unsigned cha\n \t\thashcpy(returnsha1, sha1);\n \tif (has_sha1_file(sha1))\n \t\treturn 0;\n-\treturn write_loose_object(sha1, hdr, hdrlen, buf, len, 0);\n+\treturn write_loose_object(sha1, hdr, hdrlen, buf, len, 0, paranoid);\n+}\n+\n+int write_sha1_file(void *buf, unsigned long len, const char *type, unsigned char *returnsha1)\n+{\n+\treturn write_sha1_file_paranoid(buf, len, type, returnsha1, 0);\n }\n \n int force_object_loose(const unsigned char *sha1, time_t mtime)\n@@ -2376,7 +2418,7 @@ int force_object_loose(const unsigned char *sha1, time_t mtime)\n \tif (!buf)\n \t\treturn error(\"cannot read sha1_file for %s\", sha1_to_hex(sha1));\n \thdrlen = sprintf(hdr, \"%s %lu\", typename(type), len) + 1;\n-\tret = write_loose_object(sha1, hdr, hdrlen, buf, len, mtime);\n+\tret = write_loose_object(sha1, hdr, hdrlen, buf, len, mtime, 0);\n \tfree(buf);\n \n \treturn ret;\n@@ -2405,10 +2447,15 @@ int has_sha1_file(const unsigned char *sha1)\n \treturn has_loose_object(sha1);\n }\n \n+#define INDEX_MEM_WRITE_OBJECT 01\n+#define INDEX_MEM_PARANOID 02\n+\n static int index_mem(unsigned char *sha1, void *buf, size_t size,\n-\t\t int write_object, enum object_type type, const char *path)\n+\t\t enum object_type type, const char *path, int flag)\n {\n \tint ret, re_allocated = 0;\n+\tint write_object = flag & INDEX_MEM_WRITE_OBJECT;\n+\tint paranoid = flag & INDEX_MEM_PARANOID;\n \n \tif (!type)\n \t\ttype = OBJ_BLOB;\n@@ -2426,9 +2473,11 @@ static int index_mem(unsigned char *sha1, void *buf, size_t size,\n \t}\n \n \tif (write_object)\n-\t\tret = write_sha1_file(buf, size, typename(type), sha1);\n+\t\tret = write_sha1_file_paranoid(buf, size, typename(type),\n+\t\t\t\t\t sha1, paranoid);\n \telse\n \t\tret = hash_sha1_file(buf, size, typename(type), sha1);\n+\n \tif (re_allocated)\n \t\tfree(buf);\n \treturn ret;\n@@ -2437,23 +2486,25 @@ static int index_mem(unsigned char *sha1, void *buf, size_t size,\n int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_object,\n \t enum object_type type, const char *path)\n {\n-\tint ret;\n+\tint ret, flag;\n \tsize_t size = xsize_t(st->st_size);\n \n+\tflag = write_object ? INDEX_MEM_WRITE_OBJECT : 0;\n \tif (!S_ISREG(st->st_mode)) {\n \t\tstruct strbuf sbuf = STRBUF_INIT;\n \t\tif (strbuf_read(&sbuf, fd, 4096) >= 0)\n-\t\t\tret = index_mem(sha1, sbuf.buf, sbuf.len, write_object,\n-\t\t\t\t\ttype, path);\n+\t\t\tret = index_mem(sha1, sbuf.buf, sbuf.len,\n+\t\t\t\t\ttype, path, flag);\n \t\telse\n \t\t\tret = -1;\n \t\tstrbuf_release(&sbuf);\n \t} else if (size) {\n \t\tvoid *buf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);\n-\t\tret = index_mem(sha1, buf, size, write_object, type, path);\n+\t\tflag |= INDEX_MEM_PARANOID;\n+\t\tret = index_mem(sha1, buf, size, type, path, flag);\n \t\tmunmap(buf, size);\n \t} else\n-\t\tret = index_mem(sha1, NULL, size, write_object, type, path);\n+\t\tret = index_mem(sha1, NULL, size, type, path, flag);\n \tclose(fd);\n \treturn ret;\n }\n-- \n1.7.0.81.g58679\n"},{"id":"134908","messageId":"7vzl37z6f3.fsf@alter.siamese.dyndns.org","threadId":"22620","inReplyTo":"7vljer1gyg.fsf_-_@alter.siamese.dyndns.org","subject":"Re: [PATCH] Teach \"git add\" and friends to be paranoid","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-18T01:20:00Z","receivedAt":"2010-02-18T01:20:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> But the nature of breakage caused by such an abuse is rather bad. We will\n> end up with loose object files, whose names do not match what are stored\n> and recovered when uncompressed.\n>\n> This teaches the index_mem() codepath to be paranoid and hash and compress\n> the data after reading it in core. The contents hashed may not match the\n> contents of the file in an insane use case, but at least this way the\n> result will be internally consistent.\n\nWith a small fix to the test program earlier in the thread, this seems to\nprotect the repository; I didn't bother to assess the performance impact\nof the patch, though.\n\nHere is the corrected test.\n\n-- >8 --\n#!/bin/sh\nset -e\n\n# Create an empty git repo in /tmp/git-test\nrm -fr /tmp/git-test\nmkdir /tmp/git-test\ncd /tmp/git-test\ngit init\n\n# Create a file named foo and add it to the repo\ntouch foo\ngit add foo\n\n# Thread 1: continuously modify foo:\nwhile echo -n .; do\n\tdd if=/dev/urandom of=foo count=1024 bs=1k conv=notrunc >/dev/null 2>&1\ndone &\n\n# Thread 2: loop until the repo is corrupted\nwhile git fsck; do\n\t# Note the implied 'git add' in 'commit -a'\n\t# It will do the same with explicit 'git add'\n\tgit commit -a -m'Test' || break\ndone\n\n# Kill thread 1, we don't need it any more\nkill $!\n\n# Success! Well, sort of.\nif git fsck\nthen\n\techo Repository is corrupted. Have a nice day.\nelse\n\techo Repository is still healthy. You are stupid.\nfi\n"},{"id":"134909","messageId":"20100218013822.GB15870@coredump.intra.peff.net","threadId":"22620","inReplyTo":"7vljer1gyg.fsf_-_@alter.siamese.dyndns.org","subject":"Re: [PATCH] Teach \"git add\" and friends to be paranoid","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-02-18T01:38:22Z","receivedAt":"2010-02-18T01:38:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 17, 2010 at 05:16:23PM -0800, Junio C Hamano wrote:\n\n> +\tif (paranoid) {\n> +\t\tunsigned char stablebuf[262144];\n\nIs 256K a bit big for allocating on the stack? Modern OS's seem to give\nus at least a couple of megabytes (my Linux boxen all have 8M, and\neven Solaris 8 seems to have that much). But PTHREAD_STACK_MIN is only\n16K (I don't think it is possible to hit this code path in a thread\nright now, but I'm not sure). And I have no idea what the situation is\non Windows.\n\nI dunno if it is worth worrying about, but maybe somebody more clueful\nthan me can comment.\n\n-Peff\n"},{"id":"134926","messageId":"alpine.LFD.2.00.1002172350080.1946@xanadu.home","threadId":"22620","inReplyTo":"20100218013822.GB15870@coredump.intra.peff.net","subject":"Re: [PATCH] Teach \"git add\" and friends to be paranoid","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2010-02-18T04:55:01Z","receivedAt":"2010-02-18T04:55:01Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Wed, 17 Feb 2010, Jeff King wrote:\n\n> On Wed, Feb 17, 2010 at 05:16:23PM -0800, Junio C Hamano wrote:\n> \n> > +\tif (paranoid) {\n> > +\t\tunsigned char stablebuf[262144];\n> \n> Is 256K a bit big for allocating on the stack? Modern OS's seem to give\n> us at least a couple of megabytes (my Linux boxen all have 8M, and\n> even Solaris 8 seems to have that much). But PTHREAD_STACK_MIN is only\n> 16K (I don't think it is possible to hit this code path in a thread\n> right now, but I'm not sure). And I have no idea what the situation is\n> on Windows.\n> \n> I dunno if it is worth worrying about, but maybe somebody more clueful\n> than me can comment.\n\nIt is likely to have better performance if the buffer is small enough to \nfit in the CPU L1 cache. There are two sequencial passes over the \nbuffer: one for the SHA1 computation, and another for the compression, \nand currently they're sure to trash the L1 cache on each pass.\n\nOf course that requires big enough objects to matter.\n\n\nNicolas\n"},{"id":"134929","messageId":"7vocjnqf5c.fsf@alter.siamese.dyndns.org","threadId":"22620","inReplyTo":"alpine.LFD.2.00.1002172350080.1946@xanadu.home","subject":"Re: [PATCH] Teach \"git add\" and friends to be paranoid","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-18T05:36:15Z","receivedAt":"2010-02-18T05:36:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Pitre writes:\n\n> It is likely to have better performance if the buffer is small enough to \n> fit in the CPU L1 cache. There are two sequencial passes over the \n> buffer: one for the SHA1 computation, and another for the compression, \n> and currently they're sure to trash the L1 cache on each pass.\n\nI did a very unscientific test to hash about 14k paths (arch/ and fs/ from\nthe kernel source) using \"git-hash-object -w --stdin-paths\" into an empty\nrepository with varying sizes of paranoia buffer (quarter, 1, 4, 8 and\n256kB) and saw 8-30% overhead. 256kB did hurt and around 4kB seemed to be\noptimal for my this small sample load.\n\nIn any case, with any size of paranoia, this hurts the sane use case, so\nI'd introduce an expert switch to disable it, like this.\n\n-- >8 --\nWhen creating a loose object, we normally mmap(2) the entire file, and\nhash and then compress to write it out in two separate steps for\nefficiency.\n\nThis is perfectly good for the intended use of git---nobody is supposed to\nbe insane enough to expect that it won't break anything to muck with the\ncontents of a file after telling git to index it and before getting the\ncontrol back from git.\n\nBut the nature of breakage caused by such an abuse is rather bad. We will\nend up with loose object files, whose names do not match what are stored\nand recovered when uncompressed.\n\nThis teaches the index_mem() codepath to be paranoid and hash and compress\nthe data after reading it in core. The contents hashed may not match the\ncontents of the file in an insane use case, but at least this way the\nresult will be internally consistent.\n\nPeople with saner use of git can regain performance by setting a new\nconfiguration variable 'core.volatilefiles' to false to disable this\ncheck.\n\nSigned-off-by: Junio C Hamano \n---\n Documentation/config.txt | 7 ++++\n cache.h | 1 +\n config.c | 6 +++\n environment.c | 1 +\n sha1_file.c | 83 +++++++++++++++++++++++++++++++++++++--------\n 5 files changed, 83 insertions(+), 15 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 52786c7..0295aee 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -117,6 +117,14 @@ core.fileMode::\n \tthe working copy are ignored; useful on broken filesystems like FAT.\n \tSee linkgit:git-update-index[1]. True by default.\n \n+core.volatilefiles::\n+\tIf you modify a file after telling git to record it (e.g. with\n+\t\"git add\") but before git finishes the request and gives the\n+\tcontrol back to you, you may create a broken object (and of course\n+\tyou can keep both halves ;-). Setting this option to true will\n+\ttell git to be extra careful to detect the situation and abort.\n+\tDefaults to true.\n+\n core.ignoreCygwinFSTricks::\n \tThis option is only used by Cygwin implementation of Git. If false,\n \tthe Cygwin stat() and lstat() functions are used. This may be useful\ndiff --git a/cache.h b/cache.h\nindex 231c06d..e5a87cf 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -497,6 +497,7 @@ extern int trust_ctime;\n extern int quote_path_fully;\n extern int has_symlinks;\n extern int ignore_case;\n+extern int worktree_files_are_volatile;\n extern int assume_unchanged;\n extern int prefer_symlink_refs;\n extern int log_all_ref_updates;\ndiff --git a/config.c b/config.c\nindex 790405a..9898041 100644\n--- a/config.c\n+++ b/config.c\n@@ -360,6 +360,12 @@ static int git_default_core_config(const char *var, const char *value)\n \t\ttrust_executable_bit = git_config_bool(var, value);\n \t\treturn 0;\n \t}\n+\n+\tif (!strcmp(var, \"core.volatilefiles\")) {\n+\t\tworktree_files_are_volatile = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\n \tif (!strcmp(var, \"core.trustctime\")) {\n \t\ttrust_ctime = git_config_bool(var, value);\n \t\treturn 0;\ndiff --git a/environment.c b/environment.c\nindex e278bce..5d0faf3 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -22,6 +22,7 @@ int is_bare_repository_cfg = -1; /* unspecified */\n int log_all_ref_updates = -1; /* unspecified */\n int warn_ambiguous_refs = 1;\n int repository_format_version;\n+int worktree_files_are_volatile = 1; /* yuck */\n const char *git_commit_encoding;\n const char *git_log_output_encoding;\n int shared_repository = PERM_UMASK;\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 52d1ead..e126179 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2335,14 +2335,18 @@ static int create_tmpfile(char *buffer, size_t bufsiz, const char *filename)\n }\n \n static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,\n-\t\t\t void *buf, unsigned long len, time_t mtime)\n+\t\t\t void *buf, unsigned long len, time_t mtime,\n+\t\t\t int paranoid)\n {\n \tint fd, size, ret;\n \tunsigned char *compressed;\n \tz_stream stream;\n \tchar *filename;\n \tstatic char tmpfile[PATH_MAX];\n+\tgit_SHA_CTX ctx;\n \n+\tif (!worktree_files_are_volatile)\n+\t\tparanoid = 0;\n \tfilename = sha1_file_name(sha1);\n \tfd = create_tmpfile(tmpfile, sizeof(tmpfile), filename);\n \tif (fd < 0) {\n@@ -2366,12 +2370,41 @@ static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,\n \tstream.next_in = (unsigned char *)hdr;\n \tstream.avail_in = hdrlen;\n \twhile (deflate(&stream, 0) == Z_OK)\n-\t\t/* nothing */;\n+\t\t; /* nothing */\n \n \t/* Then the data itself.. */\n-\tstream.next_in = buf;\n-\tstream.avail_in = len;\n-\tret = deflate(&stream, Z_FINISH);\n+\tif (paranoid) {\n+\t\tunsigned char stablebuf[4096];\n+\t\tchar *bufptr = buf;\n+\t\tunsigned long remainder = len;\n+\n+\t\tgit_SHA1_Init(&ctx);\n+\t\tgit_SHA1_Update(&ctx, hdr, hdrlen);\n+\n+\t\tret = Z_OK;\n+\t\twhile (remainder) {\n+\t\t\tunsigned long chunklen = remainder;\n+\n+\t\t\tif (sizeof(stablebuf) <= chunklen)\n+\t\t\t\tchunklen = sizeof(stablebuf);\n+\t\t\tmemcpy(stablebuf, bufptr, chunklen);\n+\t\t\tgit_SHA1_Update(&ctx, stablebuf, chunklen);\n+\t\t\tstream.next_in = stablebuf;\n+\t\t\tstream.avail_in = chunklen;\n+\t\t\tdo {\n+\t\t\t\tret = deflate(&stream, Z_NO_FLUSH);\n+\t\t\t} while (ret == Z_OK);\n+\t\t\tbufptr += chunklen;\n+\t\t\tremainder -= chunklen;\n+\t\t}\n+\t\tif (ret != Z_STREAM_END)\n+\t\t\tret = deflate(&stream, Z_FINISH);\n+\t} else {\n+\t\tstream.next_in = buf;\n+\t\tstream.avail_in = len;\n+\t\tret = deflate(&stream, Z_FINISH);\n+\t}\n+\n \tif (ret != Z_STREAM_END)\n \t\tdie(\"unable to deflate new object %s (%d)\", sha1_to_hex(sha1), ret);\n \n@@ -2381,6 +2414,12 @@ static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,\n \n \tsize = stream.total_out;\n \n+\tif (paranoid) {\n+\t\tunsigned char paranoid_sha1[20];\n+\t\tgit_SHA1_Final(paranoid_sha1, &ctx);\n+\t\tif (hashcmp(paranoid_sha1, sha1))\n+\t\t\tdie(\"hashed file is volatile\");\n+\t}\n \tif (write_buffer(fd, compressed, size) < 0)\n \t\tdie(\"unable to write sha1 file\");\n \tclose_sha1_file(fd);\n@@ -2398,7 +2437,7 @@ static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,\n \treturn move_temp_to_file(tmpfile, filename);\n }\n \n-int write_sha1_file(void *buf, unsigned long len, const char *type, unsigned char *returnsha1)\n+static int write_sha1_file_paranoid(void *buf, unsigned long len, const char *type, unsigned char *returnsha1, int paranoid)\n {\n \tunsigned char sha1[20];\n \tchar hdr[32];\n@@ -2412,7 +2451,12 @@ int write_sha1_file(void *buf, unsigned long len, const char *type, unsigned cha\n \t\thashcpy(returnsha1, sha1);\n \tif (has_sha1_file(sha1))\n \t\treturn 0;\n-\treturn write_loose_object(sha1, hdr, hdrlen, buf, len, 0);\n+\treturn write_loose_object(sha1, hdr, hdrlen, buf, len, 0, paranoid);\n+}\n+\n+int write_sha1_file(void *buf, unsigned long len, const char *type, unsigned char *returnsha1)\n+{\n+\treturn write_sha1_file_paranoid(buf, len, type, returnsha1, 0);\n }\n \n int force_object_loose(const unsigned char *sha1, time_t mtime)\n@@ -2430,7 +2474,7 @@ int force_object_loose(const unsigned char *sha1, time_t mtime)\n \tif (!buf)\n \t\treturn error(\"cannot read sha1_file for %s\", sha1_to_hex(sha1));\n \thdrlen = sprintf(hdr, \"%s %lu\", typename(type), len) + 1;\n-\tret = write_loose_object(sha1, hdr, hdrlen, buf, len, mtime);\n+\tret = write_loose_object(sha1, hdr, hdrlen, buf, len, mtime, 0);\n \tfree(buf);\n \n \treturn ret;\n@@ -2467,10 +2511,15 @@ int has_sha1_file(const unsigned char *sha1)\n \treturn has_loose_object(sha1);\n }\n \n+#define INDEX_MEM_WRITE_OBJECT 01\n+#define INDEX_MEM_PARANOID 02\n+\n static int index_mem(unsigned char *sha1, void *buf, size_t size,\n-\t\t int write_object, enum object_type type, const char *path)\n+\t\t enum object_type type, const char *path, int flag)\n {\n \tint ret, re_allocated = 0;\n+\tint write_object = flag & INDEX_MEM_WRITE_OBJECT;\n+\tint paranoid = flag & INDEX_MEM_PARANOID;\n \n \tif (!type)\n \t\ttype = OBJ_BLOB;\n@@ -2488,9 +2537,11 @@ static int index_mem(unsigned char *sha1, void *buf, size_t size,\n \t}\n \n \tif (write_object)\n-\t\tret = write_sha1_file(buf, size, typename(type), sha1);\n+\t\tret = write_sha1_file_paranoid(buf, size, typename(type),\n+\t\t\t\t\t sha1, paranoid);\n \telse\n \t\tret = hash_sha1_file(buf, size, typename(type), sha1);\n+\n \tif (re_allocated)\n \t\tfree(buf);\n \treturn ret;\n@@ -2499,23 +2550,25 @@ static int index_mem(unsigned char *sha1, void *buf, size_t size,\n int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_object,\n \t enum object_type type, const char *path)\n {\n-\tint ret;\n+\tint ret, flag;\n \tsize_t size = xsize_t(st->st_size);\n \n+\tflag = write_object ? INDEX_MEM_WRITE_OBJECT : 0;\n \tif (!S_ISREG(st->st_mode)) {\n \t\tstruct strbuf sbuf = STRBUF_INIT;\n \t\tif (strbuf_read(&sbuf, fd, 4096) >= 0)\n-\t\t\tret = index_mem(sha1, sbuf.buf, sbuf.len, write_object,\n-\t\t\t\t\ttype, path);\n+\t\t\tret = index_mem(sha1, sbuf.buf, sbuf.len,\n+\t\t\t\t\ttype, path, flag);\n \t\telse\n \t\t\tret = -1;\n \t\tstrbuf_release(&sbuf);\n \t} else if (size) {\n \t\tvoid *buf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);\n-\t\tret = index_mem(sha1, buf, size, write_object, type, path);\n+\t\tflag |= INDEX_MEM_PARANOID;\n+\t\tret = index_mem(sha1, buf, size, type, path, flag);\n \t\tmunmap(buf, size);\n \t} else\n-\t\tret = index_mem(sha1, NULL, size, write_object, type, path);\n+\t\tret = index_mem(sha1, NULL, size, type, path, flag);\n \tclose(fd);\n \treturn ret;\n }\n-- \n1.7.0.81.g58679\n"},{"id":"134931","messageId":"5DDD89A9-900F-40AD-8F3F-F756D6E0AD6C@wincent.com","threadId":"22620","inReplyTo":"7vocjnqf5c.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Teach \"git add\" and friends to be paranoid","fromName":"Wincent Colaiuta","fromEmail":"win@wincent.com","sentAt":"2010-02-18T07:27:28Z","receivedAt":"2010-02-18T07:27:28Z","isPatch":true,"sender":{"key":"greg@hurrell.net","avatar":"https://avatars.githubusercontent.com/u/7074?v=4"},"body":"El 18/02/2010, a las 06:36, Junio C Hamano escribió:\n\n> Nicolas Pitre writes:\n> \n>> It is likely to have better performance if the buffer is small enough to \n>> fit in the CPU L1 cache. There are two sequencial passes over the \n>> buffer: one for the SHA1 computation, and another for the compression, \n>> and currently they're sure to trash the L1 cache on each pass.\n> \n> I did a very unscientific test to hash about 14k paths (arch/ and fs/ from\n> the kernel source) using \"git-hash-object -w --stdin-paths\" into an empty\n> repository with varying sizes of paranoia buffer (quarter, 1, 4, 8 and\n> 256kB) and saw 8-30% overhead. 256kB did hurt and around 4kB seemed to be\n> optimal for my this small sample load.\n> \n> In any case, with any size of paranoia, this hurts the sane use case, so\n> I'd introduce an expert switch to disable it, like this.\n\nShouldn't a switch that hurts performance and is only needed for insane use cases default to off rather than on?\n\nCheers,\nWincent\n"},{"id":"134943","messageId":"201002181114.19984.trast@student.ethz.ch","threadId":"22620","inReplyTo":"7vljer1gyg.fsf_-_@alter.siamese.dyndns.org","subject":"Re: [PATCH] Teach \"git add\" and friends to be paranoid","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2010-02-18T10:14:19Z","receivedAt":"2010-02-18T10:14:19Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"On Thursday 18 February 2010 02:16:23 Junio C Hamano wrote:\n> When creating a loose object, we normally mmap(2) the entire file, and\n> hash and then compress to write it out in two separate steps for\n> efficiency.\n> \n> This is perfectly good for the intended use of git---nobody is supposed to\n> be insane enough to expect that it won't break anything to muck with the\n> contents of a file after telling git to index it and before getting the\n> control back from git.\n\nThis makes it sound as if the user is to blame, but IMHO we're just\nnot checking the input well enough. The user should never be able to\ncorrupt the repository (without git noticing!) just by running a git\ncommand and manipulating the worktree in parallel. The file data at\nany given time is just user input, and you also cannot (I hope;\notherwise let's fix it!) corrupt the repository merely by typoing some\ncommand arguments.\n\n(Mucking around in .git is an entirely different matter, but that is\noff limits.)\n\n> This teaches the index_mem() codepath to be paranoid and hash and compress\n> the data after reading it in core. The contents hashed may not match the\n> contents of the file in an insane use case, but at least this way the\n> result will be internally consistent.\n\nDoesn't that trigger on windows, where xmmap() already makes a copy?\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"134975","messageId":"20100218153249.GA11733@gibbs.hungrycats.org","threadId":"22620","inReplyTo":"7vzl37z6f3.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Teach \"git add\" and friends to be paranoid","fromName":"Zygo Blaxell","fromEmail":"zblaxell@gibbs.hungrycats.org","sentAt":"2010-02-18T15:32:49Z","receivedAt":"2010-02-18T15:32:49Z","isPatch":true,"sender":{"key":"zblaxell@gibbs.hungrycats.org","avatar":null},"body":"On Wed, Feb 17, 2010 at 05:20:00PM -0800, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> With a small fix to the test program earlier in the thread, this seems to\n> protect the repository; I didn't bother to assess the performance impact\n> of the patch, though.\n> \n> Here is the corrected test.\n\nDepends on what you mean by \"corrected\" I suppose.\n\n> # Thread 2: loop until the repo is corrupted\n> while git fsck; do\n> \t# Note the implied 'git add' in 'commit -a'\n> \t# It will do the same with explicit 'git add'\n> \tgit commit -a -m'Test' || break\n\nThis is at least partly wrong--it will terminate prematurely if Thread\n1 gets stalled and fails to modify 'foo' during the loop (git commit\nnormally refuses to commit a tree with no changes). This can cause\nthe test for the corruption bug to return false success results.\n\nIf you add '--allow-empty' to the git commit command you will fix that\ncase, but there might be others. \n\nIf git commit runs out of disk space, for example, the commit should\nfail, but the repository should still not be corrupt. Future commits\n(for example after freeing some disk space) should eventually succeed.\nReally, the original loop was correct, and this new one isn't.\n\n> else\n> \techo Repository is still healthy. You are stupid.\n\nIf git is working, you should never reach this line, because git\nfsck should not fail after executing any sequence of git porcelain\noperations--and this particular sequence is nothing but 'git commit'\nin a single thread.\n"},{"id":"134978","messageId":"20100218161843.GB11733@gibbs.hungrycats.org","threadId":"22620","inReplyTo":"5DDD89A9-900F-40AD-8F3F-F756D6E0AD6C@wincent.com","subject":"Re: [PATCH] Teach \"git add\" and friends to be paranoid","fromName":"Zygo Blaxell","fromEmail":"zblaxell@esightcorp.com","sentAt":"2010-02-18T16:18:43Z","receivedAt":"2010-02-18T16:18:43Z","isPatch":true,"sender":{"key":"zblaxell@esightcorp.com","avatar":null},"body":"On Thu, Feb 18, 2010 at 08:27:28AM +0100, Wincent Colaiuta wrote:\n> Shouldn't a switch that hurts performance and is only needed for insane use cases default to off rather than on?\n\nWhile I don't disagree that default off might(*) be a good idea,\nI do object to the categorization of this use case as 'insane'.\n\nNeither the documentation for 'git add' nor its various aliases (e.g. 'git\ncommit' with paths or -a, etc) mentions that any use of 'git add'\nmight cause repository corruption under any circumstances. Contrast with\nexamples of repository-corrupting pitfalls in the man pages of tools\nsuch as 'git clone' and 'git gc'.\n\nIn fact, the language in the git add man page seems to suggest the\nopposite, using words like \"snapshot\" and pointing out several times\nthat the index is intentionally immune to changes interleaved between\n'git add' and 'git commit' commands.\n\nCommon sense (for Unix users) is that the index is not immune to changes\n*during* git add, but nowhere in my wildest nightmares would I conceive\nthat changes in file contents during git add would *corrupt the\nrepository* and git would *fail to notice or give useful diagnostics*\nuntil *days or weeks later* after the corruption has already *spread to\nmultiple repositories* through *git push with default options*.\n\nNow, if you want to put that text in the man pages of 'git add' and\nfriends, and point out the paranoia switch there, I have nothing to\nobject to.\n\nI also see nothing prohibiting concurrent file modification in some\nreasonable revision-control use cases. What happens if I do a 'git\ncommit -a' on, say, proprietary EDA tool data files or Microsoft Office\ndocuments, and those tools choose an unfortunate moment to automatically\nupdate files under revision control? Granted, I can't really expect the\nrepo to contain usable data, but what I do expect is another commit, or\na rebased/amended commit, that fixes the mangled file's contents--not to\nbe required to rebase on the commit's parent everything that comes\nafter it, then purge my reflogs so 'git gc' will work again.\n\nWorking directories on network filesystems might do all kinds of strange\nthings, most of which aren't intentional. It's one thing to commit a\nuseless tree, and quite another to unintentionally commit an irretrievable\none.\n\n(*) \"might\" be a good idea because there's been some evidence to suggest\nthat a paranoid implementation of git add might perform better than the\nmmap-based one in all cases, if more work was done than anyone seems\nwilling to do.\n"},{"id":"134983","messageId":"20100218181247.GA1052@progeny.tock","threadId":"22620","inReplyTo":"20100218161843.GB11733@gibbs.hungrycats.org","subject":"Re: [PATCH] Teach \"git add\" and friends to be paranoid","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-02-18T18:12:47Z","receivedAt":"2010-02-18T18:12:47Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Zygo Blaxell wrote:\n> On Thu, Feb 18, 2010 at 08:27:28AM +0100, Wincent Colaiuta wrote:\n>> Shouldn't a switch that hurts performance and is only needed for\n>> insane use cases default to off rather than on?\n>\n> While I don't disagree that default off might(*) be a good idea,\n> I do object to the categorization of this use case as 'insane'.\n\nFWIW I think default off would not be a good idea. This talk of\ninsane uses started from the idea that git is not so great for taking\nautomatic snapshots, but as you pointed out, other situations can\ntrigger this and the failure mode is pretty bad.\n\n> (*) \"might\" be a good idea because there's been some evidence to suggest\n> that a paranoid implementation of git add might perform better than the\n> mmap-based one in all cases, if more work was done than anyone seems\n> willing to do.\n\nWhat you are saying here seems a bit handwavy. If you have some\nconcrete ideas about what this paranoid implementation should look\nlike, I encourage you to write a rough patch. The two patches so far\nhave indicated the relevant parts of sha1_file.c (index_fd at the\nbeginning and write_sha1_file at the end of the pipeline,\nrespectively). Special cases include:\n\n - The blob being added to the repository is a special file (e.g.,\n pipe) specified on the 'git hash-object' command line: I think it’s\n fine if this is slow, but it should keep working.\n\n - The blob was generated in memory (e.g. 'git apply --cached').\n\n - autocrlf conversion is on. This means scanning through the file to\n collect statistics on the dominant line ending, then scanning\n through again to convert the file.\n\n - some other filter is on. This means sending the file as input to\n a command, then slurping it up somewhere until its length has been\n determined for the beginning of the blob header\n\n - The blob being added to the repository is already in the repository,\n so it would be a waste of time to compress and write it again.\n\nSome of these already don’t have great performance for large files\n(autocrlf and filters), and I suspect there is room for improvement\nfor many of them.\n\nJonathan\n"},{"id":"134985","messageId":"7vtytee7ff.fsf@alter.siamese.dyndns.org","threadId":"22620","inReplyTo":"201002181114.19984.trast@student.ethz.ch","subject":"Re: [PATCH] Teach \"git add\" and friends to be paranoid","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-18T18:16:04Z","receivedAt":"2010-02-18T18:16:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast writes:\n\n> This makes it sound as if the user is to blame, but IMHO we're just\n> not checking the input well enough.\n\nHonesty is very good. An alternative implementation that does not hurt\nperformance as much as the \"paranoia\" would, and checks \"the input well\nenough\" would be very welcome.\n"},{"id":"134986","messageId":"7vsk8ycryc.fsf@alter.siamese.dyndns.org","threadId":"22620","inReplyTo":"20100218181247.GA1052@progeny.tock","subject":"Re: [PATCH] Teach \"git add\" and friends to be paranoid","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-18T18:35:39Z","receivedAt":"2010-02-18T18:35:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Zygo Blaxell wrote:\n>> On Thu, Feb 18, 2010 at 08:27:28AM +0100, Wincent Colaiuta wrote:\n>>> Shouldn't a switch that hurts performance and is only needed for\n>>> insane use cases default to off rather than on?\n>>\n>> While I don't disagree that default off might(*) be a good idea,\n>> I do object to the categorization of this use case as 'insane'.\n>\n> FWIW I think default off would not be a good idea. This talk of\n> insane uses started from the idea that git is not so great for taking\n> automatic snapshots,...\n\nBut git is not so great for taking automatic snapshots, and that is a\nfact. You shouldn't be expecting such a thing, but more importantly, we\nshouldn't be dishonest about it either. git fanboys who spread \"you can\nuse it to snapshot automatically\" without thinking are actively doing\ndisservice to the users by making them even more confused.\n\nIf we make this \"safety\" an opt-in feature, it would give people an excuse\nto claim that git _by default_ stores a corrupt object, and when they make\nsuch a claim, they may not reveal that it happens only when they abuse git\nin a way it it was not designed to be used to begin with. And it may not\nbe because they are malicious, but merely because they are uninformed.\n\nThe approach to use paranoia by default is to regard \"safety\" as not about\nprotecting the users from such an abuse of their own, but primarily as a\nway to protect us from potential FUD.\n\nWhat Wincent suggested would work very well if there are only honest and\ninformed people around in the world. People who use git as intended would\nnot have to do anything special. People who abuse git for their special\nuse case would be very aware of the fact that they are abusing git, and\nmore importantly, would also be honest about it. They would not complain\nthat \"git will store corrupt objects by default\", and just flip the option\nthat is designed to support their use case, and they will get their\ndesired result. Everybody is happy.\n\nBut such a happy ending would happen only in an ideal world, in which\nsadly we do not live in. It is not 2005 anymore, and the risk of FUD\narising from uninformed abuses is very real.\n"},{"id":"134995","messageId":"alpine.LFD.2.00.1002181456230.1946@xanadu.home","threadId":"22620","inReplyTo":"7vtytee7ff.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Teach \"git add\" and friends to be paranoid","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2010-02-18T19:58:06Z","receivedAt":"2010-02-18T19:58:06Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Thu, 18 Feb 2010, Junio C Hamano wrote:\n\n> Thomas Rast writes:\n> \n> > This makes it sound as if the user is to blame, but IMHO we're just\n> > not checking the input well enough.\n> \n> Honesty is very good. An alternative implementation that does not hurt\n> performance as much as the \"paranoia\" would, and checks \"the input well\n> enough\" would be very welcome.\n\nCan't we rely on the mtime of the source file? Sample it before \nstarting hashing it, then make sure it didn't change when done.\n\n\nNicolas\n"},{"id":"134998","messageId":"19325.40682.729141.973125@blake.zopyra.com","threadId":"22620","inReplyTo":"alpine.LFD.2.00.1002181456230.1946@xanadu.home","subject":"16 gig, 350,000 file repository","fromName":"Bill Lear","fromEmail":"rael@zopyra.com","sentAt":"2010-02-18T20:11:22Z","receivedAt":"2010-02-18T20:11:22Z","isPatch":false,"sender":{"key":"rael@zopyra.com","avatar":"https://gravatar.com/avatar/c4f2d2790ca3828d3b4e7dfebabf61d2fe94fd82fa49cdac2a5295dd2d46a874?d=mp&s=160"},"body":"I'm starting a new, large project and would like a quick bit of advice.\n\nBringing in a set of test cases and other files from a ClearCase\nrepository resulted in a 350,000 file git repo of about 16 gigabytes.\n\nThe time to clone over a fast network was about 250 minutes. I could\nnot verify if the repo had been packed properly, etc.\n\nHowever, we are thinking of using submodules or subtrees, to allow a\nperson to selectively clone only a part of the repo they need for\ntheir work. Is there a way to do this without submodules/subtrees?\n\nWe also need to be able to branch the entire repo, which I think would\nmake submodules kind of a pain, but don't know...\n\nWhat is the current thinking on these issues in the git community?\n\n\nBill\n"},{"id":"135002","messageId":"eaa105841002181214g380754fl9f7763ace4b2b457@mail.gmail.com","threadId":"22620","inReplyTo":"alpine.LFD.2.00.1002181456230.1946@xanadu.home","subject":"Re: [PATCH] Teach \"git add\" and friends to be paranoid","fromName":"Peter Harris","fromEmail":"git@peter.is-a-geek.org","sentAt":"2010-02-18T20:14:06Z","receivedAt":"2010-02-18T20:14:06Z","isPatch":true,"sender":{"key":"git@peter.is-a-geek.org","avatar":null},"body":"On Thu, Feb 18, 2010 at 2:58 PM, Nicolas Pitre wrote:\n>\n> Can't we rely on the mtime of the source file?  Sample it before\n> starting hashing it, then make sure it didn't change when done.\n\nSome filesystems (eg FAT) have an abysmal mtime granularity (2\nseconds). The same race exists on other filesystems, albeit with a\nmuch narrower window of opportunity.\n\nPeter Harris\n"},{"id":"135001","messageId":"7v635ub8oa.fsf@alter.siamese.dyndns.org","threadId":"22620","inReplyTo":"alpine.LFD.2.00.1002181456230.1946@xanadu.home","subject":"Re: [PATCH] Teach \"git add\" and friends to be paranoid","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-18T20:17:25Z","receivedAt":"2010-02-18T20:17:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Pitre writes:\n\n>> Honesty is very good. An alternative implementation that does not hurt\n>> performance as much as the \"paranoia\" would, and checks \"the input well\n>> enough\" would be very welcome.\n>\n> Can't we rely on the mtime of the source file? Sample it before \n> starting hashing it, then make sure it didn't change when done.\n\nI suspect that opening to mmap(2), hashing once to compute the object\nname, and deflating it to write it out, will all happen within the same\nsecond, unless you are talking about a really huge file, or you started at\nvery near a second boundary.\n\nI am perfectly Ok that it will have false negatives that way, but if the\nprobability of it catching problems is too low because git is too fast\nrelative to the file timestamp granularity, then it doesn't sound very\nuseful in practice, unless of course you are on better filesystems.\n\nIt won't have false positives and that is a very good thing, though.\n"},{"id":"135008","messageId":"alpine.LFD.2.00.1002181556320.1946@xanadu.home","threadId":"22620","inReplyTo":"19325.40682.729141.973125@blake.zopyra.com","subject":"Re: 16 gig, 350,000 file repository","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2010-02-18T20:58:42Z","receivedAt":"2010-02-18T20:58:42Z","isPatch":false,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Thu, 18 Feb 2010, Bill Lear wrote:\n\n> I'm starting a new, large project and would like a quick bit of advice.\n> \n> Bringing in a set of test cases and other files from a ClearCase\n> repository resulted in a 350,000 file git repo of about 16 gigabytes.\n> \n> The time to clone over a fast network was about 250 minutes. I could\n> not verify if the repo had been packed properly, etc.\n\nI'd start from there. If you didn't do a 'git gc --aggressive' after \nthe import then it is quite likely that your repo isn't well packed.\n\nOf course you'll need a big machine to repack this. But that should be \nneeded only once.\n\n\nNicolas\n"},{"id":"135011","messageId":"alpine.LFD.2.00.1002181604310.1946@xanadu.home","threadId":"22620","inReplyTo":"7v635ub8oa.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Teach \"git add\" and friends to be paranoid","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2010-02-18T21:30:20Z","receivedAt":"2010-02-18T21:30:20Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Thu, 18 Feb 2010, Junio C Hamano wrote:\n\n> Nicolas Pitre writes:\n> \n> >> Honesty is very good. An alternative implementation that does not hurt\n> >> performance as much as the \"paranoia\" would, and checks \"the input well\n> >> enough\" would be very welcome.\n> >\n> > Can't we rely on the mtime of the source file? Sample it before \n> > starting hashing it, then make sure it didn't change when done.\n> \n> I suspect that opening to mmap(2), hashing once to compute the object\n> name, and deflating it to write it out, will all happen within the same\n> second, unless you are talking about a really huge file, or you started at\n> very near a second boundary.\n\nHow is the index dealing with this? Surely if a file is added to the \nindex and modified within the same second then 'git status' will fail to \nnotice the changes. I'm not familiar enough with that part of Git.\n\nAlternatively, you could use the initial mtime sample to determine the \nfilesystem's time granularity by noticing how many LSBs are zero. \nLet's say FAT should have a granularity of one second. Then if the \nmtime of the file is less than one second away before starting to hash \nthen just wait for one second. If one second later the mtime has \nchanged and still less than a second away then abort. If after the hash \nthe mtime has changed then abort.\n\nOn a recent filesystem, it is likely that the mtime granularity is a \nnanosecond. Nevertheless the above algorithm should just work all the \nsame, although it is unlikely that the mtime will be within the current \nnanosecond, hence the probability for having to do an initial wait is \nalmost zero. On kernels without hires timers the granularity will be \nlike 10 ms.\n\nOf course you might be unlucky and the initial mtime sample happens to \nbe right on a whole second even on a high resolution mtime filesystem, \nin which case the delay test will consider one second instead of 10 ms \nor whatever. but the probability is rather small that you'll end up \nwith all sub-second bits to be all zeroes causing a longer delay than \nactually necessary, and this would matter only for files that would have \nbeen modified within that second. I don't think there is a reliable way \nto enquire a filesystem+OS time stamping granularity.\n\n\nNicolas\n"},{"id":"135029","messageId":"20100219010456.GA1789@progeny.tock","threadId":"22620","inReplyTo":"alpine.LFD.2.00.1002181604310.1946@xanadu.home","subject":"Re: [PATCH] Teach \"git add\" and friends to be paranoid","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-02-19T01:04:56Z","receivedAt":"2010-02-19T01:04:56Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Nicolas Pitre wrote:\n> On Thu, 18 Feb 2010, Junio C Hamano wrote:\n\n>> I suspect that opening to mmap(2), hashing once to compute the object\n>> name, and deflating it to write it out, will all happen within the same\n>> second, unless you are talking about a really huge file, or you started at\n>> very near a second boundary.\n>\n> How is the index dealing with this? Surely if a file is added to the \n> index and modified within the same second then 'git status' will fail to \n> notice the changes. I'm not familiar enough with that part of Git.\n\nSee Documentation/technical/racy-git.txt and t/t0010-racy-git.sh.\n\nShort version: in the awful case, the timestamp of the index is the\nsame as (or before) the timestamp of the file. Git will notice this\nand re-hash the tracked file.\n\n> Alternatively, you could use the initial mtime sample to determine the \n> filesystem's time granularity by noticing how many LSBs are zero.\n\nYuck.\n\nIf such detection is going to happen, I would prefer to see it used\nonce to determine the initial value of a per-repository configuration\nvariable asking to speed up ‘git add’ and friends.\n\nNote that we are currently not using the nsec timestamps to make any\nimportant decisions, probably because in some filesystems they are\nunreliable when inode cache entries are evicted (not sure about the\ncurrent status; does this work in NFS, for example?). Within the\nshort runtime of ‘git add’, I guess this would not be as much of a\nproblem.\n\nJonathan\n"},{"id":"135064","messageId":"20100219082813.GB17952@dpotapov.dyndns.org","threadId":"22620","inReplyTo":"7vljer1gyg.fsf_-_@alter.siamese.dyndns.org","subject":"Re: [PATCH] Teach \"git add\" and friends to be paranoid","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2010-02-19T08:28:13Z","receivedAt":"2010-02-19T08:28:13Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"Hi Junio,\n\nI am sorry I have not had time to reply earlier. I think it is possible\nto avoid the overhead of being on the safe side in a few common cases.\nHere is a patch. I have not had time to test it, but changes appear to\ntrivial.\n\n-- >8 --\nFrom 3e53610a41c4aad458dff13135a73bb4944f456b Mon Sep 17 00:00:00 2001\nFrom: Dmitry Potapov \nDate: Fri, 19 Feb 2010 11:00:51 +0300\nSubject: [PATCH] speed up \"git add\" by avoiding the paranoid mode\n\nWhile the paranoid mode preserve the git repository from corruption in the\ncase when the added file is changed simultaneously with running \"git add\",\nit has some overhead. However, in a few common cases, it is possible to\navoid this mode and still be on the safe side:\n\n1. If mmap() is implemented as reading the whole file in memory.\n\n2. If the whole file was read in memory as result of applying some filter.\n\n3. If the added file is small, it is faster to use read() than mmap().\n\nSigned-off-by: Dmitry Potapov \n---\n sha1_file.c | 5 ++++-\n 1 files changed, 4 insertions(+), 1 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex d8a7722..4efeb21 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2469,6 +2469,7 @@ static int index_mem(unsigned char *sha1, void *buf, size_t size,\n \t\t write_object ? safe_crlf : 0)) {\n \t\t\tbuf = strbuf_detach(&nbuf, &size);\n \t\t\tre_allocated = 1;\n+\t\t\tparanoid = 0;\n \t\t}\n \t}\n \n@@ -2490,7 +2491,7 @@ int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_object,\n \tsize_t size = xsize_t(st->st_size);\n \n \tflag = write_object ? INDEX_MEM_WRITE_OBJECT : 0;\n-\tif (!S_ISREG(st->st_mode)) {\n+\tif (!S_ISREG(st->st_mode) || size < 262144) {\n \t\tstruct strbuf sbuf = STRBUF_INIT;\n \t\tif (strbuf_read(&sbuf, fd, 4096) >= 0)\n \t\t\tret = index_mem(sha1, sbuf.buf, sbuf.len,\n@@ -2500,7 +2501,9 @@ int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_object,\n \t\tstrbuf_release(&sbuf);\n \t} else if (size) {\n \t\tvoid *buf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);\n+#ifndef NO_MMAP\n \t\tflag |= INDEX_MEM_PARANOID;\n+#endif\n \t\tret = index_mem(sha1, buf, size, type, path, flag);\n \t\tmunmap(buf, size);\n \t} else\n-- \n1.7.0\n\n-- >8 --\n"},{"id":"135067","messageId":"40aa078e1002190127m4c9d5565obb792c77e29baf28@mail.gmail.com","threadId":"22620","inReplyTo":"alpine.LFD.2.00.1002181556320.1946@xanadu.home","subject":"Re: 16 gig, 350,000 file repository","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2010-02-19T09:27:03Z","receivedAt":"2010-02-19T09:27:03Z","isPatch":false,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Thu, Feb 18, 2010 at 9:58 PM, Nicolas Pitre wrote:\n> On Thu, 18 Feb 2010, Bill Lear wrote:\n>\n>> I'm starting a new, large project and would like a quick bit of advice.\n>>\n>> Bringing in a set of test cases and other files from a ClearCase\n>> repository resulted in a 350,000 file git repo of about 16 gigabytes.\n>>\n>> The time to clone over a fast network was about 250 minutes.  I could\n>> not verify if the repo had been packed properly, etc.\n>\n> I'd start from there.  If you didn't do a 'git gc --aggressive' after\n> the import then it is quite likely that your repo isn't well packed.\n>\n> Of course you'll need a big machine to repack this.\n\nSomething like this? http://www.gadgetopia.com/images/big_machine.jpg\n\n-- \nErik \"kusma\" Faye-Lund\n"},{"id":"135085","messageId":"20100219152609.GC11733@gibbs.hungrycats.org","threadId":"22620","inReplyTo":"20100219010456.GA1789@progeny.tock","subject":"Re: [PATCH] Teach \"git add\" and friends to be paranoid","fromName":"Zygo Blaxell","fromEmail":"zblaxell@gibbs.hungrycats.org","sentAt":"2010-02-19T15:26:09Z","receivedAt":"2010-02-19T15:26:09Z","isPatch":true,"sender":{"key":"zblaxell@gibbs.hungrycats.org","avatar":null},"body":"On Thu, Feb 18, 2010 at 07:04:56PM -0600, Jonathan Nieder wrote:\n> Nicolas Pitre wrote:\n> > On Thu, 18 Feb 2010, Junio C Hamano wrote:\n> >> I suspect that opening to mmap(2), hashing once to compute the object\n> >> name, and deflating it to write it out, will all happen within the same\n> >> second, unless you are talking about a really huge file, or you started at\n> >> very near a second boundary.\n> >\n> > How is the index dealing with this? Surely if a file is added to the \n> > index and modified within the same second then 'git status' will fail to \n> > notice the changes. I'm not familiar enough with that part of Git.\n> \n> See Documentation/technical/racy-git.txt and t/t0010-racy-git.sh.\n> \n> Short version: in the awful case, the timestamp of the index is the\n> same as (or before) the timestamp of the file. Git will notice this\n> and re-hash the tracked file.\n\nAs far as I can tell, the index doesn't handle this case at all.\n\nSuppose the file is modified during git add near the beginning of the\nfile, after git add has read that part of the file, but the modifications\nfinish before git add does. Now the mtime of the file is earlier\nthan the index timestamp, but the file contents don't match the index.\nThis holds even if the objects git adds to the index aren't corrupted.\nActually right now you can have all four combinations: index up to date\nor not, and object matching its sha1 hash or not, depending on where and\nwhen you modify data during an index update.\n\nracy-git.txt doesn't discuss concurrent modification of files with the\nindex. It only discusses low-resolution file timestamps and modifications\nat times that are close to, but not concurrent with, index modifications.\n\nGit probably also doesn't handle things like NTP time corrections\n(especially those where time moves backward by sub-second intervals) and\nmismatched server/client clocks on remote filesystems either (mind you,\nI know of no SCM that currently handles that case, and CVS in particular\nis unusually bad at it).\n\nPersonally, I find the combination of nanosecond-precision timestamps\nand network file systems amusing. At nanosecond precision, relativistic\neffects start to matter across a volume of space the size of my laptop.\nI'm not sure how timestamps at any resolution could be a reliable metric\nfor detecting changes to file contents in the general case. A valuable\nhint in many cases, but not authoritative (unless they all come from a\nsingle monotonic high-resolution clock guaranteed to increment faster than\ngit--but they don't).\n\nrsync solves this sort of problem with a 'modification window' parameter,\nwhich is a time interval that is \"close enough\" to consider two timestamps\nto be equal. Some of rsync's use cases set that window to six months.\nGit would use a modification window for the opposite reason rsync\ndoes--rsync uses the window to avoid unnecessarily examining files that\nhave different timestamps, while git would use it to re-examine files\neven when it appears to be unnecessary.\n\nGit probably wants the modification window to be the maximum clock\noffset between a network filesystem client and server plus the minimum\nrepresentable interval in the filesystem's timestamp data type--which\nis a value git couldn't possibly know for some cases, so it needs input\nfrom the user.\n"},{"id":"135099","messageId":"7vbpflktaf.fsf@alter.siamese.dyndns.org","threadId":"22620","inReplyTo":"20100218153249.GA11733@gibbs.hungrycats.org","subject":"Re: [PATCH] Teach \"git add\" and friends to be paranoid","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-19T17:51:52Z","receivedAt":"2010-02-19T17:51:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Zygo Blaxell writes:\n\n> If git commit runs out of disk space, for example, the commit should\n> fail, but the repository should still not be corrupt. Future commits\n> (for example after freeing some disk space) should eventually succeed.\n\nThat is true. It is Ok to create a corrupt object as long as running an\nequivalent of fsck immediately after an object creation to catch the\nbreakage to prevent it from propagating further.\n\nThat is essentially what \"paranoid\" switch does, but it adds overhead that\nis unnecessary for the use case we primarily target in git.\n"},{"id":"135100","messageId":"7v635tkta7.fsf@alter.siamese.dyndns.org","threadId":"22620","inReplyTo":"20100219082813.GB17952@dpotapov.dyndns.org","subject":"Re: [PATCH] Teach \"git add\" and friends to be paranoid","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-19T17:52:00Z","receivedAt":"2010-02-19T17:52:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dmitry Potapov writes:\n\n> ... I think it is possible\n> to avoid the overhead of being on the safe side in a few common cases.\n> Here is a patch. I have not had time to test it, but changes appear to\n> trivial.\n\nYeah, these are obvious \"paranoia not needed\" cases.\n\nHow much \"speed-up\" are we talking about, though? Can we quantify? I\npersonally think it is not even worth to quantify it but instead simply\nsay \"avoid unnecessary computation\" without saying \"speed up\", though ;-)\n\nThanks.\n\n> -- >8 --\n> From 3e53610a41c4aad458dff13135a73bb4944f456b Mon Sep 17 00:00:00 2001\n> From: Dmitry Potapov \n> Date: Fri, 19 Feb 2010 11:00:51 +0300\n> Subject: [PATCH] speed up \"git add\" by avoiding the paranoid mode\n>\n> While the paranoid mode preserve the git repository from corruption in the\n> case when the added file is changed simultaneously with running \"git add\",\n> it has some overhead. However, in a few common cases, it is possible to\n> avoid this mode and still be on the safe side:\n>\n> 1. If mmap() is implemented as reading the whole file in memory.\n>\n> 2. If the whole file was read in memory as result of applying some filter.\n>\n> 3. If the added file is small, it is faster to use read() than mmap().\n>\n> Signed-off-by: Dmitry Potapov \n> ---\n> sha1_file.c | 5 ++++-\n> 1 files changed, 4 insertions(+), 1 deletions(-)\n>\n> diff --git a/sha1_file.c b/sha1_file.c\n> index d8a7722..4efeb21 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -2469,6 +2469,7 @@ static int index_mem(unsigned char *sha1, void *buf, size_t size,\n> \t\t write_object ? safe_crlf : 0)) {\n> \t\t\tbuf = strbuf_detach(&nbuf, &size);\n> \t\t\tre_allocated = 1;\n> +\t\t\tparanoid = 0;\n> \t\t}\n> \t}\n> \n> @@ -2490,7 +2491,7 @@ int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_object,\n> \tsize_t size = xsize_t(st->st_size);\n> \n> \tflag = write_object ? INDEX_MEM_WRITE_OBJECT : 0;\n> -\tif (!S_ISREG(st->st_mode)) {\n> +\tif (!S_ISREG(st->st_mode) || size < 262144) {\n> \t\tstruct strbuf sbuf = STRBUF_INIT;\n> \t\tif (strbuf_read(&sbuf, fd, 4096) >= 0)\n> \t\t\tret = index_mem(sha1, sbuf.buf, sbuf.len,\n> @@ -2500,7 +2501,9 @@ int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_object,\n> \t\tstrbuf_release(&sbuf);\n> \t} else if (size) {\n> \t\tvoid *buf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);\n> +#ifndef NO_MMAP\n> \t\tflag |= INDEX_MEM_PARANOID;\n> +#endif\n> \t\tret = index_mem(sha1, buf, size, type, path, flag);\n> \t\tmunmap(buf, size);\n> \t} else\n> -- \n> 1.7.0\n>\n> -- >8 --\n"},{"id":"135101","messageId":"7vzl35jeph.fsf@alter.siamese.dyndns.org","threadId":"22620","inReplyTo":"20100219152609.GC11733@gibbs.hungrycats.org","subject":"Re: [PATCH] Teach \"git add\" and friends to be paranoid","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-19T17:52:10Z","receivedAt":"2010-02-19T17:52:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Zygo Blaxell writes:\n\n> As far as I can tell, the index doesn't handle this case at all.\n> ...\n> racy-git.txt doesn't discuss concurrent modification of files with the\n> index. It only discusses low-resolution file timestamps and modifications\n> at times that are close to, but not concurrent with, index modifications.\n\nCorrect. As I said a few times in this thread, a use case with concurrent\nmodifications is outside of the original design scope of git.\n\nAs you may have realized, racy-git solution actually _relies_ on lack of\nconcurrent modifications. The document does not even _talk_ about this\nassumption, exactly because at least back then it was a common knowledge\nshared by everybody that users are not supposed to muck with files in the\nwork tree until they get control back from git and they can keep both\nhalves if they get a new broken loose object if they did so ;-).\n"},{"id":"135107","messageId":"20100219190825.GD11733@gibbs.hungrycats.org","threadId":"22620","inReplyTo":"7vzl35jeph.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Teach \"git add\" and friends to be paranoid","fromName":"Zygo Blaxell","fromEmail":"zblaxell@gibbs.hungrycats.org","sentAt":"2010-02-19T19:08:26Z","receivedAt":"2010-02-19T19:08:26Z","isPatch":true,"sender":{"key":"zblaxell@gibbs.hungrycats.org","avatar":null},"body":"On Fri, Feb 19, 2010 at 09:52:10AM -0800, Junio C Hamano wrote:\n> As you may have realized, racy-git solution actually _relies_ on lack of\n> concurrent modifications. \n\nIt relies on 1) no concurrent modifications, 2) strictly increasing\ntimestamps, and 3) consistent timestamps between the working directory\nand index. Believe it or not, out of those three I actually think the\nfirst assumption is the most reasonable, because it's something a\nuser could prevent without using administrative privileges or changing\nfilesystems.\n\nFor performance reasons I frequently work with GIT_DIR on ext3 and working\ndirectory on tmpfs (though a mix of ext3 and ext4 has the same issues).\nOne has second resolution, the other nanosecond. If two event timestamps\nwith different precisions are compared as values at maximum precision,\nthe later event might appear to occur before the earlier one.\n\nI try to avoid using anything that relies on mtime on network filesystems\nbecause a lot more than just Git breaks in those cases.\n\nNTP breakage is rarer, mostly because NTP step events are rare, and\nnegative ones ever rarer. I've seen negative steps on laptops when they\nlose time accuracy during suspend or when Internet access is unavailable\n(or asymmetrically laggy), then get it back later. On the other hand,\nI don't actually test for effects of this event anywhere, so I can't say\nit hasn't caused breakage I don't know about, although I can say it hasn't\ncause breakage I do know about either.\n"},{"id":"135181","messageId":"7v8waniue8.fsf@alter.siamese.dyndns.org","threadId":"22620","inReplyTo":"7v635tkta7.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Teach \"git add\" and friends to be paranoid","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-20T19:23:11Z","receivedAt":"2010-02-20T19:23:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Dmitry Potapov writes:\n>\n>> ... I think it is possible\n>> to avoid the overhead of being on the safe side in a few common cases.\n>> Here is a patch. I have not had time to test it, but changes appear to\n>> trivial.\n>\n> Yeah, these are obvious \"paranoia not needed\" cases.\n>\n\nActually the \"if it is smaller than 256k\" part is not quite obvious.\n\n>> @@ -2490,7 +2491,7 @@ int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_object,\n>> \tsize_t size = xsize_t(st->st_size);\n>> \n>> \tflag = write_object ? INDEX_MEM_WRITE_OBJECT : 0;\n>> -\tif (!S_ISREG(st->st_mode)) {\n>> +\tif (!S_ISREG(st->st_mode) || size < 262144) {\n>> \t\tstruct strbuf sbuf = STRBUF_INIT;\n>> \t\tif (strbuf_read(&sbuf, fd, 4096) >= 0)\n>> \t\t\tret = index_mem(sha1, sbuf.buf, sbuf.len,\n\nINDEX_MEM_PARANOID is never given to index_mem() in this codepath, so\ntrade-offs look like this:\n\n * In non-paranoia mode, your conjecture is that between\n\n - malloc, read, SHA-1, deflate, and then free; and\n - mmap, SHA-1, deflate and then munmap\n\n the former is faster for small files that can fit in core without\n thrashing.\n\n * In paranoia mode, your conjecture is that between\n\n - malloc, read, SHA-1, deflate, and then free; and\n - mmap, SHA-1, SHA-1 and deflate in chunks, and then munmap\n\n the former is faster for small files that can fit in core without\n thrashing.\n\nThe \"mmap\" strategy has larger cost in paranoia mode compared to its cost\nin non-paranoia mode. The \"read\" strategy on the other hand has the same\ncost in both modes. If this \"read small files\" is good for non-paranoia\nmode, it is obvious that it is also good (better) for paranoia mode.\n\nWhich means that this hunk addresses an unrelated issue. \"paranoid\navoidance\" falls naturally as a side effect of doing this, but that is not\nthe primary effect of this change.\n\nThere needs some benchmarking to justify it, I think.\n\nSo I'd split this hunk out when queuing.\n\nThanks.\n"},{"id":"135228","messageId":"20100221072142.GA5829@dpotapov.dyndns.org","threadId":"22620","inReplyTo":"7v8waniue8.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Teach \"git add\" and friends to be paranoid","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2010-02-21T07:21:42Z","receivedAt":"2010-02-21T07:21:42Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"On Sat, Feb 20, 2010 at 11:23:11AM -0800, Junio C Hamano wrote:\n> \n> There needs some benchmarking to justify it, I think.\n> \n> So I'd split this hunk out when queuing.\n\nI completely argee with your reasoning that it is a separate issue and\nit needs some benchmarking to prove its usefulness. So, I have done it\ntoday.\n\nFor that, I have created a repository up to 512Mb size and containing up\nto 100,000 files (for small files the file number was the limiting\nfactor, for big files, the total size limit was used). I intentionally\nlimit the total size to 512Mb to make sure that they are in the FS cache\nand no disk related effects. The content of files have been generated\nrandomly by /dev/urandom and then used in all tests for one file size.\nI have made 5 runs using mmap() (with git 1.7.0) and 5 runs with using\nread() (after applying the patch below).\n\nBelow is the best result of 5 runs for each size. \"Before\" marks the\noriginal version, which uses mmap(). \"After\" marks the modified version\nusing read(). The command used to measure time was:\n\ncat list | time git hash-object --stdin-paths >/dev/null\n\nSo it is just calculating SHA-1 without deflating and writing the\nresult on the disk, which significantly depends on fsync() speed.\n\nTested on: Intel(R) Core(TM)2 Quad CPU Q9300 @ 2.50GHz\n\nHere are results:\n\nfile size = 1Kb; Hashing 100000 files\nBefore:\n0.63user 0.86system 0:01.49elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+100428minor)pagefaults 0swaps\nAfter:\n0.54user 0.53system 0:01.07elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+421minor)pagefaults 0swaps\n\nfile size = 2Kb; Hashing 100000 files\nBefore:\n1.04user 0.79system 0:01.82elapsed 100%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+100428minor)pagefaults 0swaps\nAfter:\n0.95user 0.48system 0:01.43elapsed 100%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+422minor)pagefaults 0swaps\n\nfile size = 4Kb; Hashing 100000 files\nBefore:\n1.73user 0.74system 0:02.47elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+100428minor)pagefaults 0swaps\nAfter:\n1.54user 0.57system 0:02.11elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+424minor)pagefaults 0swaps\n\nfile size = 8Kb; Hashing 64000 files\nBefore:\n1.86user 0.63system 0:02.48elapsed 100%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+128428minor)pagefaults 0swaps\nAfter:\n1.75user 0.50system 0:02.23elapsed 100%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+424minor)pagefaults 0swaps\n\nfile size = 16Kb; Hashing 32000 files\nBefore:\n1.73user 0.41system 0:02.14elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+128428minor)pagefaults 0swaps\nAfter:\n1.68user 0.32system 0:02.00elapsed 100%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+425minor)pagefaults 0swaps\n\nfile size = 32Kb; Hashing 16000 files\nBefore:\n1.65user 0.32system 0:01.96elapsed 100%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+128428minor)pagefaults 0swaps\nAfter:\n1.64user 0.24system 0:01.87elapsed 100%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+431minor)pagefaults 0swaps\n\nfile size = 64Kb; Hashing 8000 files\nBefore:\n1.71user 0.17system 0:01.87elapsed 100%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+128428minor)pagefaults 0swaps\nAfter:\n1.63user 0.18system 0:01.81elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+438minor)pagefaults 0swaps\n\nfile size = 128Kb; Hashing 4000 files\nBefore:\n1.60user 0.20system 0:01.79elapsed 100%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+128428minor)pagefaults 0swaps\nAfter:\n1.55user 0.20system 0:01.75elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+454minor)pagefaults 0swaps\n\nfile size = 256Kb; Hashing 2000 files\nBefore:\n1.62user 0.15system 0:01.77elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+128429minor)pagefaults 0swaps\nAfter:\n1.56user 0.16system 0:01.71elapsed 100%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+551minor)pagefaults 0swaps\n\nfile size = 512Kb; Hashing 1000 files\nBefore:\n1.59user 0.17system 0:01.76elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+128428minor)pagefaults 0swaps\nAfter:\n1.56user 0.15system 0:01.71elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+679minor)pagefaults 0swaps\n\nfile size = 1024Kb; Hashing 500 files\nBefore:\n1.64user 0.15system 0:01.78elapsed 100%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+128429minor)pagefaults 0swaps\nAfter:\n1.61user 0.15system 0:01.76elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+934minor)pagefaults 0swaps\n\n\nAs you can see, in all tests the read() version performed better than\nmmap() though the difference decreases with increase of the file size.\nWhile for 1Kb files, the speed up is 39% (based on the elapsed time),\nit is mere 1% for 1Mb file size.\n\nNote: I do not use strbuf_read(), because it is suboptimal to deal with\nthis case, because we know the size ahead. (In fact, strbuf_read() is\nnot so good even for unknown size as it does redundant strbuf_grow()\nalmost in every use case, which probably needs to be fixed).\n\n-- >8 --\nFrom 6b3f8335dece7c9b9f810b1ab08f1bcb090e4d5e Mon Sep 17 00:00:00 2001\nFrom: Dmitry Potapov \nDate: Sun, 21 Feb 2010 09:32:19 +0300\nSubject: [PATCH] hash-object: don't use mmap() for small files\n\nUsing read() instead of mmap() can be 39% speed up for 1Kb files and is\n1% speed up 1Mb files. For larger files, it is better to use mmap(),\nbecause the difference between is not significant, and when there is not\nenough memory, mmap() performs much better, because it avoids swapping.\n\nSigned-off-by: Dmitry Potapov \n---\n sha1_file.c | 10 ++++++++++\n 1 files changed, 10 insertions(+), 0 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 657825e..8a83e56 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2434,6 +2434,8 @@ static int index_mem(unsigned char *sha1, void *buf, size_t size,\n \treturn ret;\n }\n \n+#define SMALL_FILE_SIZE (1024*1024)\n+\n int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_object,\n \t enum object_type type, const char *path)\n {\n@@ -2448,6 +2450,14 @@ int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_object,\n \t\telse\n \t\t\tret = -1;\n \t\tstrbuf_release(&sbuf);\n+\t} else if (size <= SMALL_FILE_SIZE) {\n+\t\tchar *buf = xmalloc(size);\n+\t\tif (size == read_in_full(fd, buf, size))\n+\t\t\tret = index_mem(sha1, buf, size, write_object, type,\n+\t\t\t\t\tpath);\n+\t\telse\n+\t\t\tret = error(\"short read %s\", strerror(errno));\n+\t\tfree(buf);\n \t} else if (size) {\n \t\tvoid *buf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);\n \t\tret = index_mem(sha1, buf, size, write_object, type, path);\n-- \n1.7.0\n\n-- >8 --\n\n\nThanks,\nDmitry\n"},{"id":"299151","messageId":"7vhbpas7ut.fsf@alter.siamese.dyndns.org","threadId":"22620","inReplyTo":"20100221072142.GA5829@dpotapov.dyndns.org","subject":"Re: [PATCH] Teach \"git add\" and friends to be paranoid","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-21T19:32:10Z","receivedAt":"2010-02-21T19:32:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dmitry Potapov writes:\n\n> file size = 1Kb; Hashing 100000 files\n> Before:\n> 0.63user 0.86system 0:01.49elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k\n> 0inputs+0outputs (0major+100428minor)pagefaults 0swaps\n> After:\n> 0.54user 0.53system 0:01.07elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k\n> 0inputs+0outputs (0major+421minor)pagefaults 0swaps\n>\n> As you can see, in all tests the read() version performed better than\n> mmap() though the difference decreases with increase of the file size.\n> While for 1Kb files, the speed up is 39% (based on the elapsed time),\n> it is mere 1% for 1Mb file size.\n\nSounds good. Summarizing your numbers,\n\n 1Kb 39.25%\n 2Kb 27.27%\n 4Kb 17.06%\n 8Kb 11.21%\n 16Kb 7.00%\n 32Kb 4.81%\n 64Kb 3.31%\n 128Kb 2.29%\n 256Kb 3.51%\n 512Kb 2.92%\n 1024Kb 1.14%\n\n32*1024 sounds like a better cut-off to me. After that, doubling the size\ndoes not get comparable gain, and numbers get unstable (notice the glitch\naround 256kB).\n"},{"id":"135205","messageId":"20100222033553.GA10191@dpotapov.dyndns.org","threadId":"22620","inReplyTo":"7vhbpas7ut.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Teach \"git add\" and friends to be paranoid","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2010-02-22T03:35:54Z","receivedAt":"2010-02-22T03:35:54Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"On Sun, Feb 21, 2010 at 11:32:10AM -0800, Junio C Hamano wrote:\n> \n> 32*1024 sounds like a better cut-off to me. After that, doubling the size\n> does not get comparable gain, and numbers get unstable (notice the glitch\n> around 256kB).\n\nThe reduction of speed-up after 32Kb is most likely due to L1 cache\nsize, which is 32Kb data per core on Core 2, and L2 cache is shared\namong cores and is considerably slow. I have run my test a few more\ntimes, and here are results:\n\n 1 - 39.25%\n 2 - 30.00%\n 4 - 17.79%\n 8 - 11.76%\n 16 - 7.58%\n 32 - 5.38%\n 64 - 3.89%\n 128 - 2.87%\n 256 - 2.31%\n 512 - 2.92%\n1024 - 1.14%\n\nand here is one more re-run starting with 32Kb:\n\n 32 - 5.38%\n 64 - 3.89%\n 128 - 2.29%\n 256 - 2.91%\n 512 - 2.92%\n1024 - 1.14%\n\nIf you look at speed-up numbers, you can think that the numbers are\nunstable, but in fact, the best time in 5 runs does not differ more\nthan 0.01s between those trials. But because difference for >=128Kb\nis 0.05s or less, the accuracy of the above numbers is less than 25%.\nBut overall the outcome is clear -- read() is always a winner.\n\nIt would be interesting to see what difference Nehalem, which has a\nsmaller but much faster L2 cache than Core 2. It may perform better\nat larger sizes up to 256Kb.\n\nAnyway, based on above data, I believe that the proper cut-off should be\nat least 64Kb, because additional 32Kb (from 32Kb to 64Kb) is about of\n2.5% of total memory that git consumes anyway, and it gives you speed-up\naround 3.5%...\n\n\n\nDmitry\n"},{"id":"135268","messageId":"7vwry5pxg8.fsf@alter.siamese.dyndns.org","threadId":"22620","inReplyTo":"20100222033553.GA10191@dpotapov.dyndns.org","subject":"Re: [PATCH] Teach \"git add\" and friends to be paranoid","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-22T06:59:51Z","receivedAt":"2010-02-22T06:59:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dmitry Potapov writes:\n\n> If you look at speed-up numbers, you can think that the numbers are\n> unstable, but in fact, the best time in 5 runs does not differ more\n> than 0.01s between those trials. But because difference for >=128Kb\n> is 0.05s or less, the accuracy of the above numbers is less than 25%.\n\nThen wouldn't it make the following statement...\n\n> But overall the outcome is clear -- read() is always a winner.\n\n\"... a winner, below 128kB; above that the difference is within noise and\nmeasurement error\"?\n\n> It would be interesting to see what difference Nehalem, which has a\n> smaller but much faster L2 cache than Core 2. It may perform better\n> at larger sizes up to 256Kb.\n\nInteresting.\n"},{"id":"135304","messageId":"20100222122505.GF10191@dpotapov.dyndns.org","threadId":"22620","inReplyTo":"7vwry5pxg8.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Teach \"git add\" and friends to be paranoid","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2010-02-22T12:25:05Z","receivedAt":"2010-02-22T12:25:05Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"On Sun, Feb 21, 2010 at 10:59:51PM -0800, Junio C Hamano wrote:\n> \n> \"... a winner, below 128kB; above that the difference is within noise and\n> measurement error\"?\n\nWhat I was trying to say is that if you see consistently four or five\npoints win (based on many trials), it is clear a win; but you have 25%\nerror bar for the gain number.\n\n\nDmitry\n"},{"id":"135308","messageId":"4B827FC6.1090905@gnu.org","threadId":"22620","inReplyTo":"7vocjnqf5c.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Teach \"git add\" and friends to be paranoid","fromName":"Paolo Bonzini","fromEmail":"bonzini@gnu.org","sentAt":"2010-02-22T12:59:50Z","receivedAt":"2010-02-22T12:59:50Z","isPatch":true,"sender":{"key":"bonzini@gnu.org","avatar":"https://avatars.githubusercontent.com/u/42082?v=4"},"body":"On 02/18/2010 06:36 AM, Junio C Hamano wrote:\n> Nicolas Pitre writes:\n>\n>> It is likely to have better performance if the buffer is small enough to\n>> fit in the CPU L1 cache. There are two sequencial passes over the\n>> buffer: one for the SHA1 computation, and another for the compression,\n>> and currently they're sure to trash the L1 cache on each pass.\n>\n> I did a very unscientific test to hash about 14k paths (arch/ and fs/ from\n> the kernel source) using \"git-hash-object -w --stdin-paths\" into an empty\n> repository with varying sizes of paranoia buffer (quarter, 1, 4, 8 and\n> 256kB) and saw 8-30% overhead. 256kB did hurt and around 4kB seemed to be\n> optimal for my this small sample load.\n>\n> In any case, with any size of paranoia, this hurts the sane use case\n\nBecause by mmaping + memcpying you are getting the worst of both cases: \nyou get a page fault per page like with mmap, and touch memory twice \nlike with read.\n\nPaolo\n"},{"id":"135310","messageId":"20100222133327.GH10191@dpotapov.dyndns.org","threadId":"22620","inReplyTo":"4B827FC6.1090905@gnu.org","subject":"Re: [PATCH] Teach \"git add\" and friends to be paranoid","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2010-02-22T13:33:27Z","receivedAt":"2010-02-22T13:33:27Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"On Mon, Feb 22, 2010 at 01:59:50PM +0100, Paolo Bonzini wrote:\n> On 02/18/2010 06:36 AM, Junio C Hamano wrote:\n> >\n> >In any case, with any size of paranoia, this hurts the sane use case\n> \n> Because by mmaping + memcpying you are getting the worst of both\n> cases: you get a page fault per page like with mmap, and touch\n> memory twice like with read.\n\nand there is also an extra round of SHA-1 calculation, which I believe\nis more expensive than memcpy().\n\n\nDmitry\n"},{"id":"135320","messageId":"alpine.LFD.2.00.1002221033120.1946@xanadu.home","threadId":"22620","inReplyTo":"7vwry5pxg8.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Teach \"git add\" and friends to be paranoid","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2010-02-22T15:40:59Z","receivedAt":"2010-02-22T15:40:59Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Sun, 21 Feb 2010, Junio C Hamano wrote:\n\n> Dmitry Potapov writes:\n> \n> > If you look at speed-up numbers, you can think that the numbers are\n> > unstable, but in fact, the best time in 5 runs does not differ more\n> > than 0.01s between those trials. But because difference for >=128Kb\n> > is 0.05s or less, the accuracy of the above numbers is less than 25%.\n> \n> Then wouldn't it make the following statement...\n> \n> > But overall the outcome is clear -- read() is always a winner.\n> \n> \"... a winner, below 128kB; above that the difference is within noise and\n> measurement error\"?\n\nread() is not always a winner. A read() call will always have the data \nduplicated in memory. Especially with large files, it is more efficient \non the system as a whole to mmap() a 50 MB file rather than allocating \nan extra 50 MB of anonymous memory that cannot be paged out (except to \nthe swap file which would be yet another data duplication). With mmap() \nwhen there is memory pressure the read-only mapped memory is simply \ndropped with no extra IO.\n\nSo when read() is not _significantly_ faster than mmap() then it should \nnot be used.\n\n\nNicolas\n"},{"id":"135322","messageId":"37fcd2781002220801q2bdb553g15c0a99301a27283@mail.gmail.com","threadId":"22620","inReplyTo":"alpine.LFD.2.00.1002221033120.1946@xanadu.home","subject":"Re: [PATCH] Teach \"git add\" and friends to be paranoid","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2010-02-22T16:01:54Z","receivedAt":"2010-02-22T16:01:54Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"On Mon, Feb 22, 2010 at 6:40 PM, Nicolas Pitre wrote:\n>\n> read() is not always a winner.  A read() call will always have the data\n> duplicated in memory.  Especially with large files, it is more efficient\n> on the system as a whole to mmap() a 50 MB file rather than allocating\n\nAgreed. I meant it was faster on data that I measured, which was <=1Mb.\nIMHO, read() should be used up to 64Kb.\n\nDmitry\n"},{"id":"135326","messageId":"20100222173122.GG11733@gibbs.hungrycats.org","threadId":"22620","inReplyTo":"alpine.LFD.2.00.1002221033120.1946@xanadu.home","subject":"Re: [PATCH] Teach \"git add\" and friends to be paranoid","fromName":"Zygo Blaxell","fromEmail":"zblaxell@gibbs.hungrycats.org","sentAt":"2010-02-22T17:31:22Z","receivedAt":"2010-02-22T17:31:22Z","isPatch":true,"sender":{"key":"zblaxell@gibbs.hungrycats.org","avatar":null},"body":"On Mon, Feb 22, 2010 at 10:40:59AM -0500, Nicolas Pitre wrote:\n> On Sun, 21 Feb 2010, Junio C Hamano wrote:\n> > Dmitry Potapov writes:\n> > > But overall the outcome is clear -- read() is always a winner.\n> > \n> > \"... a winner, below 128kB; above that the difference is within noise and\n> > measurement error\"?\n> \n> read() is not always a winner. A read() call will always have the data \n> duplicated in memory. Especially with large files, it is more efficient \n> on the system as a whole to mmap() a 50 MB file rather than allocating \n> an extra 50 MB of anonymous memory that cannot be paged out (except to \n> the swap file which would be yet another data duplication). With mmap() \n> when there is memory pressure the read-only mapped memory is simply \n> dropped with no extra IO.\n\nThat holds if you're comparing read() and mmap() of the entire file as a\nsingle chunk, instead of in fixed-size chunks at the sweet spot between\nsyscall overhead and CPU cache size.\n\nIf you're read()ing a chunk at a time into a fixed size buffer, and\ndoing sha1 and deflate in chunks, the data should be copied once into CPU\ncache, processed with both algorithms, and replaced with new data from\nthe next chunk. The data will be copied from the page cache instead\nof directly mapped, which is a small overhead, but setting up the page\nmap in mmap() also a small overhead, so you have to use benchmarks to\nknow which of the overheads is smaller. It might be that there's no\none answer that applies to all CPU configurations.\n\nIf you're doing mmap() and sha1 and deflate of a 50MB file in two\nseparate passes that are the same size as the file, you load 50MB of\ndata into CPU cache at least twice, you get two sets of associated\nthings like TLB misses, and if the file is very large, you page it from\ndisk twice. So it might make sense to process in chunks regardless\nof read() vs mmap() fetching the data.\n\nIf you're malloc()ing 50MB, you're wasting memory and CPU bandwidth\nmaking up pages full of zeros before you've even processed the first byte.\nI don't see how that could ever be faster for large file cases.\n"},{"id":"135328","messageId":"alpine.LFD.2.00.1002221238110.1946@xanadu.home","threadId":"22620","inReplyTo":"20100222173122.GG11733@gibbs.hungrycats.org","subject":"Re: [PATCH] Teach \"git add\" and friends to be paranoid","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2010-02-22T18:01:13Z","receivedAt":"2010-02-22T18:01:13Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Mon, 22 Feb 2010, Zygo Blaxell wrote:\n\n> On Mon, Feb 22, 2010 at 10:40:59AM -0500, Nicolas Pitre wrote:\n> > On Sun, 21 Feb 2010, Junio C Hamano wrote:\n> > > Dmitry Potapov writes:\n> > > > But overall the outcome is clear -- read() is always a winner.\n> > > \n> > > \"... a winner, below 128kB; above that the difference is within noise and\n> > > measurement error\"?\n> > \n> > read() is not always a winner. A read() call will always have the data \n> > duplicated in memory. Especially with large files, it is more efficient \n> > on the system as a whole to mmap() a 50 MB file rather than allocating \n> > an extra 50 MB of anonymous memory that cannot be paged out (except to \n> > the swap file which would be yet another data duplication). With mmap() \n> > when there is memory pressure the read-only mapped memory is simply \n> > dropped with no extra IO.\n> \n> That holds if you're comparing read() and mmap() of the entire file as a\n> single chunk, instead of in fixed-size chunks at the sweet spot between\n> syscall overhead and CPU cache size.\n\nObviously. But we currently don't have the infrastructure to do chunked \nread of the input data. I think we should do that eventually, by \napplying the pack windowing code to input files as well. That would \nmake memory usage constant even for huge files, but this is much more \ncomplicated to support especially for data fed through stdin.\n\n> If you're read()ing a chunk at a time into a fixed size buffer, and\n> doing sha1 and deflate in chunks, the data should be copied once into CPU\n> cache, processed with both algorithms, and replaced with new data from\n> the next chunk. The data will be copied from the page cache instead\n> of directly mapped, which is a small overhead, but setting up the page\n> map in mmap() also a small overhead, so you have to use benchmarks to\n> know which of the overheads is smaller. It might be that there's no\n> one answer that applies to all CPU configurations.\n\nNormally mmap() has more overhead than read(). However mmap() provides \nmuch nicer properties than read() by simplifying the code a lot, and by \nletting the OS manage memory pressure much more gracefully.\n\n> If you're doing mmap() and sha1 and deflate of a 50MB file in two\n> separate passes that are the same size as the file, you load 50MB of\n> data into CPU cache at least twice, you get two sets of associated\n> things like TLB misses, and if the file is very large, you page it from\n> disk twice. So it might make sense to process in chunks regardless\n> of read() vs mmap() fetching the data.\n\nWe do have to make two separate passes anyway. The first pass is to \nhash the data only, and if that hash already exists in the object store \nthen we call it done and skip over the deflate process which is still \nthe dominant cost. And that happens quite often.\n\nHowever, with a really large file, then it becomes advantageous to \nsimply do the hash and deflate in parallel one chunk at a time, and \nsimply discard the newly created objects if it happens to already \nexists. That's the whole idea behind the newly introduced \ncore.bigFileThreshold config variable (but the code to honor it in \nsha1_file.c doesn't exist yet).\n\n> If you're malloc()ing 50MB, you're wasting memory and CPU bandwidth\n> making up pages full of zeros before you've even processed the first byte.\n> I don't see how that could ever be faster for large file cases.\n\nIt can't. This is why read() is not much better than mmap() in those \ncases.\n\n\nNicolas\n"},{"id":"135330","messageId":"37fcd2781002221005l2a8ecb64y3be84eaaacd27cdc@mail.gmail.com","threadId":"22620","inReplyTo":"20100222173122.GG11733@gibbs.hungrycats.org","subject":"Re: [PATCH] Teach \"git add\" and friends to be paranoid","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2010-02-22T18:05:45Z","receivedAt":"2010-02-22T18:05:45Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"On Mon, Feb 22, 2010 at 8:31 PM, Zygo Blaxell\n wrote:\n>\n> If you're read()ing a chunk at a time into a fixed size buffer, and\n> doing sha1 and deflate in chunks, the data should be copied once into CPU\n> cache, processed with both algorithms, and replaced with new data from\n> the next chunk.\n\nCurrently, we calculate SHA-1, then lookup whether the object with this\nSHA-1 exists, and if it does not, then deflate and write it to the\nobject storage. So, we avoid deflate and write cost if this object\nalready exists. Moreover, when we deflate data, we create the temporary\nfile in the same directory where the target object will be stored, thus\navoiding cross-directory rename (which is important for some reason, but\nI don't remember why). So, creating the temporary file requires the\nknowledge first two digits of SHA-1, which you cannot know without\ncalculation SHA-1.\n\nSo, the idea of processing file in chunks is very attractive, but it has\ntwo drawbacks:\n1. extra cost (deflating+writing) when the object is already stored\n2. some issues with cross-directory renaming\n\n\nDmitry\n"},{"id":"135332","messageId":"alpine.LFD.2.00.1002221310130.1946@xanadu.home","threadId":"22620","inReplyTo":"37fcd2781002221005l2a8ecb64y3be84eaaacd27cdc@mail.gmail.com","subject":"Re: [PATCH] Teach \"git add\" and friends to be paranoid","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2010-02-22T18:14:29Z","receivedAt":"2010-02-22T18:14:29Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Mon, 22 Feb 2010, Dmitry Potapov wrote:\n\n> Currently, we calculate SHA-1, then lookup whether the object with this\n> SHA-1 exists, and if it does not, then deflate and write it to the\n> object storage. So, we avoid deflate and write cost if this object\n> already exists. Moreover, when we deflate data, we create the temporary\n> file in the same directory where the target object will be stored, thus\n> avoiding cross-directory rename (which is important for some reason, but\n> I don't remember why). So, creating the temporary file requires the\n> knowledge first two digits of SHA-1, which you cannot know without\n> calculation SHA-1.\n\nEven that initial SHA1 calculation can be done in chunks. Worth doing \nfor large enough files only though.\n\n\nNicolas\n"},{"id":"299152","messageId":"7vtyt9cad2.fsf@alter.siamese.dyndns.org","threadId":"22620","inReplyTo":"alpine.LFD.2.00.1002221238110.1946@xanadu.home","subject":"Re: [PATCH] Teach \"git add\" and friends to be paranoid","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-22T19:56:57Z","receivedAt":"2010-02-22T19:56:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Pitre writes:\n\n> We do have to make two separate passes anyway. The first pass is to \n> hash the data only, and if that hash already exists in the object store \n> then we call it done and skip over the deflate process which is still \n> the dominant cost. And that happens quite often.\n>\n> However, with a really large file, then it becomes advantageous to \n> simply do the hash and deflate in parallel one chunk at a time, and \n> simply discard the newly created objects if it happens to already \n> exists. That's the whole idea behind the newly introduced \n> core.bigFileThreshold config variable (but the code to honor it in \n> sha1_file.c doesn't exist yet).\n\nThe core.bigFileThreshold could be used in sha1_file.c to decide writing\nstraight into a new packfile; that would avoid both the later repacking\ncost and the cross directory rename issue for loose object files.\n"},{"id":"135340","messageId":"alpine.LFD.2.00.1002221552030.1946@xanadu.home","threadId":"22620","inReplyTo":"7vtyt9cad2.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Teach \"git add\" and friends to be paranoid","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2010-02-22T20:52:52Z","receivedAt":"2010-02-22T20:52:52Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Mon, 22 Feb 2010, Junio C Hamano wrote:\n\n> Nicolas Pitre writes:\n> \n> > We do have to make two separate passes anyway. The first pass is to \n> > hash the data only, and if that hash already exists in the object store \n> > then we call it done and skip over the deflate process which is still \n> > the dominant cost. And that happens quite often.\n> >\n> > However, with a really large file, then it becomes advantageous to \n> > simply do the hash and deflate in parallel one chunk at a time, and \n> > simply discard the newly created objects if it happens to already \n> > exists. That's the whole idea behind the newly introduced \n> > core.bigFileThreshold config variable (but the code to honor it in \n> > sha1_file.c doesn't exist yet).\n> \n> The core.bigFileThreshold could be used in sha1_file.c to decide writing\n> straight into a new packfile; that would avoid both the later repacking\n> cost and the cross directory rename issue for loose object files.\n\nYep, that's the plan. Let's find the time now.\n\n\nNicolas\n"},{"id":"135348","messageId":"19331.792.84192.53690@blake.zopyra.com","threadId":"22620","inReplyTo":"alpine.LFD.2.00.1002181556320.1946@xanadu.home","subject":"Re: 16 gig, 350,000 file repository","fromName":"Bill Lear","fromEmail":"rael@zopyra.com","sentAt":"2010-02-22T22:20:08Z","receivedAt":"2010-02-22T22:20:08Z","isPatch":false,"sender":{"key":"rael@zopyra.com","avatar":"https://gravatar.com/avatar/c4f2d2790ca3828d3b4e7dfebabf61d2fe94fd82fa49cdac2a5295dd2d46a874?d=mp&s=160"},"body":"On Thursday, February 18, 2010 at 15:58:42 (-0500) Nicolas Pitre writes:\n>On Thu, 18 Feb 2010, Bill Lear wrote:\n>\n>> I'm starting a new, large project and would like a quick bit of advice.\n>> \n>> Bringing in a set of test cases and other files from a ClearCase\n>> repository resulted in a 350,000 file git repo of about 16 gigabytes.\n>> \n>> The time to clone over a fast network was about 250 minutes. I could\n>> not verify if the repo had been packed properly, etc.\n>\n>I'd start from there. If you didn't do a 'git gc --aggressive' after \n>the import then it is quite likely that your repo isn't well packed.\n>\n>Of course you'll need a big machine to repack this. But that should be \n>needed only once.\n\nOk, well they have a \"big machine\", but not big enough. It's running\nout of memory on the gc. I believe they have a fair amount of memory:\n\n% free\n total used free shared buffers cached\nMem: 16629680 16051444 578236 0 28332 14385948\n-/+ buffers/cache: 1637164 14992516\nSwap: 8289500 1704 8287796\n\nand they are using git 1.6.6.\n\nAssuming we can figure out how to gc this puppy (is there any way on a\nmachine without 64 gigabytes?), there is still a question that\nremains: how to organize a project that has a very large amount of\ntest cases (and test data) that we might not want to pull across the\nwire each time. Instead of shallow clone, as sort of slicing clone\noperation?\n\nWe thought of using submodules. That is, code (say) goes in a separate\nrepo 'src' and functional tests go in another, called 'ftests'. Then,\nwe add 'ftests' as a submodule to 'src'. Great. However, we need to\nbe able to branch 'src' and 'ftests' together. Example: I am working on\na new feature in a branch \"GLX-473_incremental_compression\". I would like\nto be able to create the branch in both the 'src' repo and the 'ftests'\nrepo at the same time, make changes, commit, and push to that branch for\nboth. When developers check out the repo, they move to that branch, but\ndo NOT want the cloned ftests. However, the QA team wants both the source\nand the tests that I have checked in and pushed.\n\nIs there an easy way to support this?\n\n\nBill\n"},{"id":"135350","messageId":"alpine.LFD.2.00.1002221725380.1946@xanadu.home","threadId":"22620","inReplyTo":"19331.792.84192.53690@blake.zopyra.com","subject":"Re: 16 gig, 350,000 file repository","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2010-02-22T22:31:16Z","receivedAt":"2010-02-22T22:31:16Z","isPatch":false,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Mon, 22 Feb 2010, Bill Lear wrote:\n\n> On Thursday, February 18, 2010 at 15:58:42 (-0500) Nicolas Pitre writes:\n> >On Thu, 18 Feb 2010, Bill Lear wrote:\n> >\n> >> I'm starting a new, large project and would like a quick bit of advice.\n> >> \n> >> Bringing in a set of test cases and other files from a ClearCase\n> >> repository resulted in a 350,000 file git repo of about 16 gigabytes.\n> >> \n> >> The time to clone over a fast network was about 250 minutes. I could\n> >> not verify if the repo had been packed properly, etc.\n> >\n> >I'd start from there. If you didn't do a 'git gc --aggressive' after \n> >the import then it is quite likely that your repo isn't well packed.\n> >\n> >Of course you'll need a big machine to repack this. But that should be \n> >needed only once.\n> \n> Ok, well they have a \"big machine\", but not big enough. It's running\n> out of memory on the gc. I believe they have a fair amount of memory:\n> \n> % free\n> total used free shared buffers cached\n> Mem: 16629680 16051444 578236 0 28332 14385948\n> -/+ buffers/cache: 1637164 14992516\n> Swap: 8289500 1704 8287796\n> \n> and they are using git 1.6.6.\n\nHmmm. OK.\n\nYou might try:\n\n\tgit repack -a -f -d --depth=200 --window=100 --window-memory=1g\n\n\nNicolas\n"}]}