{"thread":{"id":"43076","subject":"Re: [RFC \\ WISH] Add -o option to git-rev-list","startedAt":"2006-12-10T11:38:42Z","lastAt":"2006-12-15T21:04:17Z","messageCount":36,"participants":["Andreas Ericsson","Marco Costalba","Josef Weidendorfer","Linus Torvalds","Alex Riesen"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"294415","messageId":"e5bfff550612100338ye2ca2a0u1c8f29bbc59c5431@mail.gmail.com","threadId":"43076","inReplyTo":null,"subject":"[RFC \\ WISH] Add -o option to git-rev-list","fromName":"Marco Costalba","fromEmail":"mcostalba@gmail.com","sentAt":"2006-12-10T11:38:42Z","receivedAt":"2006-12-10T11:38:42Z","isPatch":false,"sender":{"key":"mcostalba@gmail.com","avatar":null},"body":"I have done some tests about reading git-rev-list output with\ndifferent IPC facilities:\n\n- Native Qt QProcess socket based IPC\n\n- pipe based: popen() and fread()\n\n- redirecting git-rev-list to a file (under tmpfs) and 'block reading'\nback the file: read() under Qt QFile class. The file, always in\nmemory, is deleted at the end of loading.\n\n\nI have tested with different block sizes and different CPU speed on\nlinux and git trees.\n\nThe averaged results on linux tree (about 30MB of data) and CPU set at\n1.2GHz are:\n\n- QProcess 6734ms\n\n- pipe and fread() with 64KB blocks 4832ms (38% faster then QProcess)\n\n- temporary file and read() with 64KB blocks 4321ms (10% faster then pipe)\n\n\nI have not enough knowledge to understand why temporary file is faster\nthen pipe. My guess is, after reading some docs around, fread() uses a\nC standard I/O buffer, while read() is unbuffered.\n\nTo make git-rev-list writing a temporary file I create and run a\nscript more or less like:\n\ngit rev-list --header --boundary --parents --topo-order HEAD >\n/tmp/qgit_135902672.txt\n\nThere is also some additional kludge to get git-rev-list pid, so to\nuse in case of a cancel request arrives while loading.\n\n\nSo my request is if it is possible to make git-rev-list write\n_directly_ to a file, without shell redirection, I would ask if it is\npossible:\n\n- Add a -o, --output option to git-rev-list to specify output file\ninstead of stdout\n\n- Use an unbuffered binary block write to be as fastest as possible\n\n- Do *not* flush ever the file, it's only a waste under this scenario.\n\n\nThis will let me to avoid the shell launching crap and probably gain\nalso some more speed.\n\nI understand this could be not exactly a top priority feature for git\npeople, but I would really like to get the best possible interface\nwith the plumbing git and the -o options is also a very common one.\n\nThanks\nMarco\n\nP.S: On another thread I explained why I see problematic linking\ndirectly against libgit.a\nI rewrite here for completeness:\n\n> I've looked again to Shawn idea (and code) of linking qgit\n> against libgit.a but I found these two difficult points:\n>\n> - traverse_commit_list(&revs, show_commit, show_object) is blocking,\n> i.e. the GUI will stop responding for few seconds while traversing the\n> list. This is easily and transparently solved by the OS scheduler if\n> an external process is used for git-rev-list. To solve this in qgit I\n> have two ways: 1) call QEventLoop() once in a while from inside\n> show_commit()/ show_object() to process pending events 2) Use a\n> separate thread (QThread class). The first idea is not nice, the\n> second opens a whole a new set of problems and it's a big amount of\n> not trivial new code to add.\n>\n> - traverse_commit_list() having an internal state it's not\n> re-entrant. git-rev-list it's used to load main view data but also\n> file history in another tab, and the two calls _could_ be ran\n> concurrently. With external process I simply run two instances of\n> DataLoader class and consequently two external git-rev-list processes,\n> but If I link against libgit.a that would be a big problem.\n"},{"id":"295527","messageId":"20061210145444.GB4836@steel.home","threadId":"43076","inReplyTo":"e5bfff550612100338ye2ca2a0u1c8f29bbc59c5431@mail.gmail.com","subject":"Re: [RFC \\ WISH] Add -o option to git-rev-list","fromName":"Alex Riesen","fromEmail":"fork0@t-online.de","sentAt":"2006-12-10T14:54:44Z","receivedAt":"2006-12-10T14:54:44Z","isPatch":false,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Marco Costalba, Sun, Dec 10, 2006 12:38:42 +0100:\n> So my request is if it is possible to make git-rev-list write\n> _directly_ to a file, without shell redirection, I would ask if it is\n> possible:\n\nWell, it is usually possible to redirect stdout directly into a file\n(see dup2). \"Usually\", unless you want windows which as always has\nit's own stupid way of doing simple things. Nevertheless, it's\npossible to do it without ever touching rev-list.\n\n> I understand this could be not exactly a top priority feature for git\n> people, but I would really like to get the best possible interface\n> with the plumbing git and the -o options is also a very common one.\n\nSadly, you're right. Almost every command-line program got the option.\nWhat education could have caused this, I wonder...\n\n> P.S: On another thread I explained why I see problematic linking\n> directly against libgit.a\n\nIt still is the fastest you can get.\n"},{"id":"297477","messageId":"Pine.LNX.4.64.0612101009040.12500@woody.osdl.org","threadId":"43076","inReplyTo":"e5bfff550612100338ye2ca2a0u1c8f29bbc59c5431@mail.gmail.com","subject":"Re: [RFC \\ WISH] Add -o option to git-rev-list","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-12-10T18:16:40Z","receivedAt":"2006-12-10T18:16:40Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sun, 10 Dec 2006, Marco Costalba wrote:\n> \n> The averaged results on linux tree (about 30MB of data) and CPU set at\n> 1.2GHz are:\n> \n> - QProcess 6734ms\n> \n> - pipe and fread() with 64KB blocks 4832ms (38% faster then QProcess)\n> \n> - temporary file and read() with 64KB blocks 4321ms (10% faster then pipe)\n> \n> I have not enough knowledge to understand why temporary file is faster\n> then pipe. My guess is, after reading some docs around, fread() uses a\n> C standard I/O buffer, while read() is unbuffered.\n\nWhy don't you use the pipe and standard read()?\n\nEven if you use \"popen()\" and get a \"FILE *\" back, you can still do\n\n\tint fd = fileno(file);\n\nand use the raw IO capabilities.\n\nThe thing is, temporary files can actually be faster under Linux just \nbecause the Linux page-cache simply kicks ass. But it's not going to be \n_that_ big of a difference, and you need all that crazy \"wait for rev-list \nto finish\" and the \"clean up temp-file on errors\" etc crap, so there's no \nway it's a better solution.\n\nIf it really is stdio overhead (possibly locking), using \"fileno()\" and \nthe raw unistd.h interfaces is going to avoid it.\n\n(You still need to use \"fclose()\" to close the struct file afterwards, \notherwise you'll leak memory, so you shouldn't _forget_ the original \n\"struct FILE *\", but you don't need to use it for anything else).\n\nUsing popen() and pipes also means that if the parent dies, the child will \nget a nice EPIPE on the writing side, which is what you want.\n\n"},{"id":"294860","messageId":"e5bfff550612101151r5e77ddc0w8f61f36fc1f2920e@mail.gmail.com","threadId":"43076","inReplyTo":"Pine.LNX.4.64.0612101009040.12500@woody.osdl.org","subject":"Re: [RFC \\ WISH] Add -o option to git-rev-list","fromName":"Marco Costalba","fromEmail":"mcostalba@gmail.com","sentAt":"2006-12-10T19:51:38Z","receivedAt":"2006-12-10T19:51:38Z","isPatch":false,"sender":{"key":"mcostalba@gmail.com","avatar":null},"body":"On 12/10/06, Linus Torvalds wrote:\n>\n> Why don't you use the pipe and standard read()?\n>\n> Even if you use \"popen()\" and get a \"FILE *\" back, you can still do\n>\n> int fd = fileno(file);\n>\n> and use the raw IO capabilities.\n>\n> The thing is, temporary files can actually be faster under Linux just\n> because the Linux page-cache simply kicks ass. But it's not going to be\n> _that_ big of a difference, and you need all that crazy \"wait for rev-list\n> to finish\" and the \"clean up temp-file on errors\" etc crap, so there's no\n> way it's a better solution.\n>\n\nTwo things.\n\n- memory use: the next natural step with files is, instead of loading\nthe file content in memory and *keep it there*, we could load one\nchunk at a time, index the chunk and discard. At the end we keep in\nmemory only indexing info to quickly get to the data when needed, but\nthe big part of data stay on the file.\n\n- This is probably my ignorance, but experimenting with popen() I\nfound I could not know *when* git-rev-list ends because both feof()\nand ferror() give 0 after a fread() with git-rev-list already defunct.\nNot having a reference to the process (it is hidden behind popen() ),\nI had to check for 0 bytes read after a successful read (to avoid\nracing in case I ask the pipe before the first data it's ready) to\nknow that job is finished and call pclose().\n\n"},{"id":"295365","messageId":"Pine.LNX.4.64.0612101159530.12500@woody.osdl.org","threadId":"43076","inReplyTo":"e5bfff550612101151r5e77ddc0w8f61f36fc1f2920e@mail.gmail.com","subject":"Re: [RFC \\ WISH] Add -o option to git-rev-list","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-12-10T20:08:53Z","receivedAt":"2006-12-10T20:08:53Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sun, 10 Dec 2006, Marco Costalba wrote:\n> \n> - memory use: the next natural step with files is, instead of loading\n> the file content in memory and *keep it there*, we could load one\n> chunk at a time, index the chunk and discard. At the end we keep in\n> memory only indexing info to quickly get to the data when needed, but\n> the big part of data stay on the file.\n\nWell, that's still just going slower than swapping.\n\nThe reason loading things into memory is nice is that:\n - in the common case, you don't need to do anything else.\n - if the machine is low on memory, it can page things out just about as \n easily as you could write things to a file anyway.\n\nSo don't worry too much about low-memory situations. Yes, there are cases \nwhere it's better to keep things in files and simply not have a big \nworking set AT ALL, but if you keep something in a file and the file data \nis still part of the working set (ie you read it several times, but at the \nbeginning and the end), that really isn't any better than having it in \nmemory.\n\nSo the time to try to optimize memory usage is really only for \"streaming \nbehaviour\" - where you need to touch something only once. Then the best \noption is to actually use a pipe and re-use the memory, but if you have \nfile data, you can use things like fadvise(DONTNEED).\n\n> - This is probably my ignorance, but experimenting with popen() I\n> found I could not know *when* git-rev-list ends because both feof()\n> and ferror() give 0 after a fread() with git-rev-list already defunct.\n\nI suspect you had a bug somewhere. It could be a bug in stdio, but I doubt \nit.\n\nYou do realize that the correct way to check \"feof()\" is only _after_ \nfread() returns 0? \n\nStdio ferror/feof is horrible to use corrrectly, and few people get it \nright. Mostly because it's such a crap interface thanks to being \"easy\" \nto use and thus hiding all the error handling details on purpose.\n\n"},{"id":"297464","messageId":"Pine.LNX.4.64.0612101213520.12500@woody.osdl.org","threadId":"43076","inReplyTo":"Pine.LNX.4.64.0612101159530.12500@woody.osdl.org","subject":"Re: [RFC \\ WISH] Add -o option to git-rev-list","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-12-10T20:19:55Z","receivedAt":"2006-12-10T20:19:55Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sun, 10 Dec 2006, Linus Torvalds wrote:\n> \n> So don't worry too much about low-memory situations.\n\nBtw, I should obviously clarify that. You should _always_ worry about \nlow-memory situations, but the only real issue is really \"working set \nsize\", not \"data in files or in memory\".\n\nIf the file representation is very dense (like the git pack-files are, for \nexample), then it may well make sense to keep the data in a file, just \nbecause it's smaller there than if you keep it in expanded form in memory.\n\nAlso, it's nice to keep stuff in the filesystem rather than in process VM \nmemory, because filesystem data that is cached in memory is useful for \n_other_ processes, ie it has a lifetime that is longer than the process \nitself. However, that's obviously only true for long-lived files that are \nshared among processes, it's _not_ true for temporary files.\n\nFor temporary files, the memory footprint of a temp-file is usually \n_larger_ than the memory footprint of the same data kept in memory. You \nhave things like page-cache alignment, and often the issue of marshalling \ndata into ASCII etc. So temp-files are almost never a better solution than \nkeeping things in memory (unless you use those temp-files to truly \n_share_ data between processes, ie you do a shared mmap and they can \nre-use the same pages actively in a way they couldn't otherwise).\n\n"},{"id":"296593","messageId":"e5bfff550612101405h507cc877k87491264d11b0aa8@mail.gmail.com","threadId":"43076","inReplyTo":"Pine.LNX.4.64.0612101213520.12500@woody.osdl.org","subject":"Re: [RFC \\ WISH] Add -o option to git-rev-list","fromName":"Marco Costalba","fromEmail":"mcostalba@gmail.com","sentAt":"2006-12-10T22:05:36Z","receivedAt":"2006-12-10T22:05:36Z","isPatch":false,"sender":{"key":"mcostalba@gmail.com","avatar":null},"body":"On 12/10/06, Linus Torvalds wrote:\n>\n\n> data into ASCII etc. So temp-files are almost never a better solution than\n> keeping things in memory (unless you use those temp-files to truly\n> _share_ data between processes, ie you do a shared mmap and they can\n> re-use the same pages actively in a way they couldn't otherwise).\n>\n\nOk. Perhaps I'm doing something wrong but the following code it's\nalways 10% slower then the temporary file one (4.7s against 4.3s for\nlinux tree)\n\nbool DataLoader::start(const QStringList& args, const QString& workDir) {\n\n\tQDir::setCurrent(workDir);\n\n\t_file = popen(args.join(\" \").ascii(), \"r\");\n\tif (!_file)\n\t\treturn false;\n\t\n\tloadTime.start();\n\tguiUpdateTimer.start(10, true); // will call on_timeout() in 10ms\n\treturn true;\n}\n\nvoid DataLoader::on_timeout() {\n\n\tif (canceling)\n\t\tdeleteLater();\n\n// \tint fd = fileno(_file); // read() case\n\n\tssize_t len = 0;\n\twhile (1) {\n\n\t\tQByteArray* ba = new QByteArray(FILE_BLOCK_SIZE); // 64KB\n\n// \t\tlen = read(fd, ba->data(), ba->size()); // read() case\n\n\t\tlen = fread(ba->data(), 1, ba->size(), _file); // fread() case\n\t\tif (len <= 0) {\n\t\t\tdelete ba;\n\t\t\tbreak;\n\n\t\t} else if (len < (ssize_t)ba->size()) // very rare, 4 out of 40000\non Linux tree\n\t\t\tba->resize(len);\n\n\t\tloadedBytes += len;\n\t\tfh->rowData.append(ba); // fh->rowData it's a pointer's list\n\t\tparseSingleBuffer(ba);\n\n\t\t// avoid reading small chunks if data producer is still running\n\t\tif (len < FILE_BLOCK_SIZE)\n\t\t\tbreak;\n\t}\n\n// \tif (len == 0) { // read() case\n\tif (feof(_file)) { // fread() case\n\t\n\t\temit loaded(fh, loadedBytes, loadTime.elapsed(), true, \"\", \"\"); //\nshow some stats\n\n\t\tpclose(_file);\n\t\t_file = NULL;\n\t\tdeleteLater();\n\t} else\n\t\tguiUpdateTimer.start(100, true); // next read in 100ms\t\n}\n\nUncomment 'read() case' lines and comment out the 'fread case()' ones\nand you have a way slooooower code, about 10 times slower!\n\n\n"},{"id":"297002","messageId":"e5bfff550612101409sf2b8b0dl37567b6c7e875b14@mail.gmail.com","threadId":"43076","inReplyTo":"e5bfff550612101405h507cc877k87491264d11b0aa8@mail.gmail.com","subject":"Re: [RFC \\ WISH] Add -o option to git-rev-list","fromName":"Marco Costalba","fromEmail":"mcostalba@gmail.com","sentAt":"2006-12-10T22:09:55Z","receivedAt":"2006-12-10T22:09:55Z","isPatch":false,"sender":{"key":"mcostalba@gmail.com","avatar":null},"body":"On 12/10/06, Marco Costalba wrote:\n> On 12/10/06, Linus Torvalds wrote:\n> >\n> void DataLoader::on_timeout() {\n>\n> if (canceling)\n> deleteLater();\n>\n\nOf course is\n\n\tif (canceling) {\n\t\tdeleteLater();\n\t\treturn;\n\t}\n\n"},{"id":"294970","messageId":"Pine.LNX.4.64.0612101410220.12500@woody.osdl.org","threadId":"43076","inReplyTo":"e5bfff550612101405h507cc877k87491264d11b0aa8@mail.gmail.com","subject":"Re: [RFC \\ WISH] Add -o option to git-rev-list","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-12-10T22:16:30Z","receivedAt":"2006-12-10T22:16:30Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sun, 10 Dec 2006, Marco Costalba wrote:\n> \n> Ok. Perhaps I'm doing something wrong but the following code it's\n> always 10% slower then the temporary file one (4.7s against 4.3s for\n> linux tree)\n\nWhy do you seem to be doing a \"new\" on every iteration inside the loop?\n\nAlso, why do you have that strange FILE_BLOCK_SIZE thing, and in \nparticular the \"if (len < FILE_BLOCK_SIZE)\" check? One thing that pipes vs \nfiles do is the blocking factor.\n\nEspecially with older kernels, I _guarantee_ you that you'll only ever get \n4kB at a time, so because of that \"if (len < 64kB) break\" thing, the only \nthing you're doing is to make sure things suck performance-wise, and you \nwon't be reading the rest of the data until 100ms later.\n\nIOW, your code is written for a file, and makes no sense there either \n(checking \"feof(file)\" is wrong, since you may well have hit the EOF \n*at*that*time*, but the file may GROW since you are doing it all in the \nbackground, so you can't rely on feof() anyway).\n\nFor a pipe, what you should do is to make sure it's in nonblocking mode, \nand just continue reading until it gets no more. And instead of using a \ntimeout, you should use poll() or something to get notified when there is \nmore data.\n\nIOW, the reason it's slow is because you're doing the wrong thing.\n\n"},{"id":"297703","messageId":"e5bfff550612101435o6bc938acmac28ad6adf0c8844@mail.gmail.com","threadId":"43076","inReplyTo":"Pine.LNX.4.64.0612101410220.12500@woody.osdl.org","subject":"Re: [RFC \\ WISH] Add -o option to git-rev-list","fromName":"Marco Costalba","fromEmail":"mcostalba@gmail.com","sentAt":"2006-12-10T22:35:33Z","receivedAt":"2006-12-10T22:35:33Z","isPatch":false,"sender":{"key":"mcostalba@gmail.com","avatar":null},"body":"On 12/10/06, Linus Torvalds wrote:\n>\n>\n> On Sun, 10 Dec 2006, Marco Costalba wrote:\n> >\n> > Ok. Perhaps I'm doing something wrong but the following code it's\n> > always 10% slower then the temporary file one (4.7s against 4.3s for\n> > linux tree)\n>\n> Why do you seem to be doing a \"new\" on every iteration inside the loop?\n>\n\nBecuase it's there where I store the file content.\n\nFunction parseSingleBuffer(ba) does only the indexing. But the file\ncontent is stored in QByteArray objects (little wrappers around a\nconst char* []). So the fread() in the byte array object is the _only_\ndata copy operation in whole qgit.\n\n\n> Also, why do you have that strange FILE_BLOCK_SIZE thing, and in\n> particular the \"if (len < FILE_BLOCK_SIZE)\" check? One thing that pipes vs\n> files do is the blocking factor.\n>\n> Especially with older kernels, I _guarantee_ you that you'll only ever get\n> 4kB at a time, so because of that \"if (len < 64kB) break\" thing, the only\n> thing you're doing is to make sure things suck performance-wise, and you\n> won't be reading the rest of the data until 100ms later.\n>\n\nI consistently have len == 65536 bytes until the last fread() where\nit's less. See below run against qgit own repository with 'len'\nprinted inside while loop.\n\n$ ./qgit\nFound GNU source-highlight 2.5\nlen is <65536>\nlen is <65536>\nlen is <65536>\nlen is <65536>\nlen is <65536>\nlen is <65536>\nlen is <65536>\nlen is <54479>\nbash-3.1$\n\n> IOW, your code is written for a file, and makes no sense there either\n> (checking \"feof(file)\" is wrong, since you may well have hit the EOF\n> *at*that*time*, but the file may GROW since you are doing it all in the\n> background, so you can't rely on feof() anyway).\n>\n\nYes. feof() it's difficult to handle correctly.\n\n> For a pipe, what you should do is to make sure it's in nonblocking mode,\n> and just continue reading until it gets no more. And instead of using a\n> timeout, you should use poll() or something to get notified when there is\n> more data.\n>\n\nHow can open in nonblocking mode with popen() ?\n\nFILE *popen(const char *command, const char *type);\n\n\n> IOW, the reason it's slow is because you're doing the wrong thing.\n>\n\nThat's for sure :-) My problem is to guess what's is wrong.\n\n\n"},{"id":"295258","messageId":"Pine.LNX.4.64.0612101439540.12500@woody.osdl.org","threadId":"43076","inReplyTo":"e5bfff550612101435o6bc938acmac28ad6adf0c8844@mail.gmail.com","subject":"Re: [RFC \\ WISH] Add -o option to git-rev-list","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-12-10T22:53:43Z","receivedAt":"2006-12-10T22:53:43Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sun, 10 Dec 2006, Marco Costalba wrote:\n> \n> I consistently have len == 65536 bytes until the last fread() where\n> it's less.\n\nThat's because fread() will block until it gets all data. Did you actually \never try this with a uncached tree and did you compare what you got with a \nplain \"read()\". \n\nOn older kernels, I guarantee that you get 4kB at a time for reads, even \nfor a blocking pipe. Because we have bigger pipe buffers these days, it \n_may_ return 64kB at a time every time, but only if the writer is much \nfaster than the reader.\n\nBased on the fact that you say that \"read()\" was ten times slower than \nfread(), I very much suspect you got 4kB at a time, and then slept 100ms \neach time or something.\n\nAnyway, you should seriously also check the case when \"git rev-list\" is \nslow because you have cold caches and unpacked objects. You can't wait for \nit to synchronously write a thousand lines or so - that could take \nseconds.\n\n> How can open in nonblocking mode with popen() ?\n> \n> FILE *popen(const char *command, const char *type);\n\nsomething like\n\n\tfcntl(fileno(file), F_SETFL, O_NONBLOCK);\n\nwill do it, and then your loop should look something like\n\n\tfor (;;) {\n\t\tint count = read(fileno(file), buf, BUFSIZE);\n\t\tif (!count) {\n\t\t\t/* All done, no more to read */\n\t\t\treturn 0;\n\t\t}\n\t\tif (count < 0) {\n\t\t\tif (errno == EAGAIN)\n\t\t\t\tbreak;\n\t\t\tif (errno == EINTR)\n\t\t\t\tcontinue;\n\t\t\t/* Anything else is fatal - report error */\n\t\t\treturn -1;\n\t\t}\n\t\t... handle 'count' new bytes here ...\n\t}\n\t.. this is the EAGAIN case ..\n\teither set polling on the file descriptor, or use a\n\ttimer here yo get back to this loop eventually.\n\nlooks about right.\n\n"},{"id":"298159","messageId":"e5bfff550612101615u5f588513n499e3b88ce14a6f6@mail.gmail.com","threadId":"43076","inReplyTo":"Pine.LNX.4.64.0612101439540.12500@woody.osdl.org","subject":"Re: [RFC \\ WISH] Add -o option to git-rev-list","fromName":"Marco Costalba","fromEmail":"mcostalba@gmail.com","sentAt":"2006-12-11T00:15:20Z","receivedAt":"2006-12-11T00:15:20Z","isPatch":false,"sender":{"key":"mcostalba@gmail.com","avatar":null},"body":"On 12/10/06, Linus Torvalds wrote:\n>\n> looks about right.\n>\n\nYes it's right. Thanks!\n\nBut it's still slow. Almost one second (985ms) to read the little qgit repo:\n\n$ ./qgit HEAD\nFound GNU source-highlight 2.5\ncount is <-1>\ncount is <60169>\ncount is <-1>\ncount is <60505>\ncount is <-1>\ncount is <61462>\ncount is <-1>\ncount is <61911>\ncount is <-1>\ncount is <61392>\ncount is <-1>\ncount is <61880>\ncount is <-1>\ncount is <62009>\ncount is <-1>\ncount is <62549>\ncount is <-1>\ncount is <21354>\ncount is <0>\n$\n\nAs a compare the temporary file version needs a mere 105ms (1030 revs).\n\nThis is the code under test:\n\nbool DataLoader::start(const QStringList& args, const QString& workDir) {\n\n\tQDir::setCurrent(workDir);\n\t_file = popen(args.join(\" \").ascii(), \"r\");\n\tif (!_file)\n\t\treturn false;\n\n\tfcntl(fileno(_file), F_SETFL, O_NONBLOCK);\n\t\n\tloadTime.start();\n\tguiUpdateTimer.start(10, true);\n\treturn true;\n}\n\nvoid DataLoader::on_timeout() {\n\n\tif (canceling) {\n\t\tdeleteLater();\n\t\treturn;\n\t}\n\n\tint count;\n\tfor (;;) {\n\n\t\tQByteArray* ba = new QByteArray(FILE_BLOCK_SIZE); // 64KB\n\n\t\t// this is the ONLY deep copy involved in the whole loading\n\t\tcount = read(fileno(_file), ba->data(), ba->size());\n\n\t\tdbg(count); // DEBUG print\n\n\t\tif (count == 0) {\n\t\t\t/* All done, no more to read */\n\t\t\tdelete ba;\n\t\t\tbreak;\n\t\t}\n\t\tif (count < 0) {\n\t\t\tdelete ba;\n\t\t\tif (errno == EAGAIN)\n\t\t\t\tbreak;\n\t\t\tif (errno == EINTR)\n\t\t\t\tcontinue;\n\n /* Anything else is fatal - report error */\n dbg(\"Fatal error\");\n on_cancel();\n deleteLater();\n return;\n\t\t}\n\t\tif (count < (int)ba->size()) // very rare\n\t\t\tba->resize(count);\n\n\t\tloadedBytes += count;\n\t\tfh->rowData.append(ba);\n\t\tparseSingleBuffer(*ba);\n\t}\n\n\tif (count == 0) {\n\t\temit loaded(fh, loadedBytes, loadTime.elapsed(), true, \"\", \"\");\n\t\tpclose(_file);\n\t\t_file = NULL;\n\t\tdeleteLater();\n\t} else\n\t\tguiUpdateTimer.start(100, true);\n"},{"id":"295663","messageId":"Pine.LNX.4.64.0612101647310.12500@woody.osdl.org","threadId":"43076","inReplyTo":"e5bfff550612101615u5f588513n499e3b88ce14a6f6@mail.gmail.com","subject":"Re: [RFC \\ WISH] Add -o option to git-rev-list","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-12-11T00:51:54Z","receivedAt":"2006-12-11T00:51:54Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 11 Dec 2006, Marco Costalba wrote:\n\n> \n> But it's still slow. Almost one second (985ms) to read the little qgit repo:\n\nRight. Because every time you sleep, you sleep for 100 ms.\n\nThat's why I was saying that you need to add polling to the thing. I don't \nknow what the QT interfaces to asynchronous polling file descriptors are, \nbut as long as you just blindly wait for 100ms whenever you run out of \ndata, things will always suck.\n\nUsing \"fread()\" hid this problem, because the thing would block in \nfread(), and thus you'd nor see as many of these events.\n\n> As a compare the temporary file version needs a mere 105ms (1030 revs).\n\nHow about you just compare something simpler:\n\n\tgit-rev-list | cat > /dev/null\n\nvs \n\n\tgit-rev-list > tmpfile ; cat tmpfile > /dev/null\n\nand see which one works better.\n\nIf the pipe works better, that means that it's your code that is buggy and \nbroken.\n\nWhich gets us back to the basic issue: you're asking for a bad interface.\n\nThis is your problem:\n\n> \t\tguiUpdateTimer.start(100, true);\n\nrather than just blindly starting a timer, you should ask it to wait until \nmore data is available.\n\n"},{"id":"295561","messageId":"e5bfff550612102317w331f3b0fyc3c6b095ff21191a@mail.gmail.com","threadId":"43076","inReplyTo":"Pine.LNX.4.64.0612101647310.12500@woody.osdl.org","subject":"Re: [RFC \\ WISH] Add -o option to git-rev-list","fromName":"Marco Costalba","fromEmail":"mcostalba@gmail.com","sentAt":"2006-12-11T07:17:15Z","receivedAt":"2006-12-11T07:17:15Z","isPatch":false,"sender":{"key":"mcostalba@gmail.com","avatar":null},"body":"On 12/11/06, Linus Torvalds wrote:\n\n>\n> How about you just compare something simpler:\n>\n> git-rev-list | cat > /dev/null\n>\n> vs\n>\n> git-rev-list > tmpfile ; cat tmpfile > /dev/null\n>\n> and see which one works better.\n>\n\nThese are tipical values (warm cache):\n\n $ time git rev-list --header --boundary --parents --topo-order HEAD /dev/null\n3.04user 0.05system 0:03.09elapsed 100%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+10141minor)pagefaults 0swaps\n\n$ time git rev-list --header --boundary --parents --topo-order HEAD |\ncat > /dev/null\n3.67user 0.36system 0:04.29elapsed 93%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+18033minor)pagefaults 0swaps\n\n$ time git rev-list --header --boundary --parents --topo-order HEAD >\n/tmp/tmp.txt; cat /tmp/tmp.txt > /dev/null\n3.44user 0.28system 0:03.74elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+18033minor)pagefaults 0swaps\n\nFor some reason the CPU *never* goes up 93% with pipe (while it's easy\nin the other two cases) and I repeated that test at lest 10 times\nconsecutively. Is it perhaps the signature of some blocking around? Or\ntoo much high frequency of receiver's read call (see below) ?\n\n>\n> This is your problem:\n>\n> > guiUpdateTimer.start(100, true);\n>\n> rather than just blindly starting a timer, you should ask it to wait until\n> more data is available.\n>\n\nOK. I just don't understand how after waiting 100ms I get only 60KB\nand stay in the loop only one cycle instead of reading, for many\ncycles, much more then 60KB and then wait another 100ms and found\nanother big (about 1MB) amount of data ready to be read as with the\nfile case.\n\nPerhaps the pipe buffers are small and block the writer when full. In\nthe file case when I come back in the loop after 100ms I found _a lot_\nof data to be read and I stay in the loop for much more then 1 cycle\nbefore to wait again 100ms.\n\nOn the other hand, going to read each say less then 10ms is exactly\nwhat QProcess (socket based) was doing and I ended up with my read\nfunction being called at furious pace slowing down everything.\nExperimenting with QProcess I found that, for performance reasons,\nit's better to read big chunks few times then small chunks a lot of\ntimes.\n\n Marco\n\nP.S: 30MB for 64KB each chunk it's 468, in 3.67s it's 1 call each\n7.8ms. If the pipe calls the receiver for data ready after each 4KB\n(old kernels) then we should have 7.500 calls. Impossible to read in\n3.67s, it would be a theoretical 1 call each 0.48ms average.\n\nSo IMHO bigger buffers for each read call could be the way to get\n"},{"id":"295772","messageId":"200612111026.23656.Josef.Weidendorfer@gmx.de","threadId":"43076","inReplyTo":"e5bfff550612101615u5f588513n499e3b88ce14a6f6@mail.gmail.com","subject":"Re: [RFC \\ WISH] Add -o option to git-rev-list","fromName":"Josef Weidendorfer","fromEmail":"josef.weidendorfer@gmx.de","sentAt":"2006-12-11T09:26:22Z","receivedAt":"2006-12-11T09:26:22Z","isPatch":false,"sender":{"key":"josef.weidendorfer@gmx.de","avatar":null},"body":"On Monday 11 December 2006 01:15, Marco Costalba wrote:\n> \t\tguiUpdateTimer.start(100, true);\n\nWhat is the result with\t\"guiUpdateTimer.start(0, true);\" ?\nThere is no need to put in any time interval at all, because\nthe timeout is a normal event which will be queued in the GUI\nevent queue.\n\nIf there were X events in the mean time, they are queued and handled\nbefore your timeOut function is called again. So the GUI will be\nresponsive, even if you have a 0 ms timer. \n\n"},{"id":"297791","messageId":"81b0412b0612110200j7c27e962va63381ea3c976050@mail.gmail.com","threadId":"43076","inReplyTo":"e5bfff550612102317w331f3b0fyc3c6b095ff21191a@mail.gmail.com","subject":"Re: [RFC \\ WISH] Add -o option to git-rev-list","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2006-12-11T10:00:16Z","receivedAt":"2006-12-11T10:00:16Z","isPatch":false,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On 12/11/06, Marco Costalba wrote:\n> $ time git rev-list --header --boundary --parents --topo-order HEAD\n> /dev/null\n> 3.04user 0.05system 0:03.09elapsed 100%CPU (0avgtext+0avgdata 0maxresident)k\n> 0inputs+0outputs (0major+10141minor)pagefaults 0swaps\n\nhere you filtered output of git-rev-list by /dev/null, which makes no sense\n\n> $ time git rev-list --header --boundary --parents --topo-order HEAD >\n> /tmp/tmp.txt; cat /tmp/tmp.txt > /dev/null\n> 3.44user 0.28system 0:03.74elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k\n> 0inputs+0outputs (0major+18033minor)pagefaults 0swaps\n\nshould be: time { git-rev-list >/tmp/tmp; cat /tmp/tmp >/dev/null; }\n"},{"id":"298428","messageId":"457D4386.70408@op5.se","threadId":"43076","inReplyTo":"e5bfff550612101151r5e77ddc0w8f61f36fc1f2920e@mail.gmail.com","subject":"Re: [RFC \\ WISH] Add -o option to git-rev-list","fromName":"Andreas Ericsson","fromEmail":"ae@op5.se","sentAt":"2006-12-11T11:39:50Z","receivedAt":"2006-12-11T11:39:50Z","isPatch":false,"sender":{"key":"ae@op5.se","avatar":"https://gravatar.com/avatar/426e89595c75a8f5252dd0c989e5fabe5bcac616e68557427ad9aef6b0ca342a?d=mp&s=160"},"body":"Marco Costalba wrote:\n> On 12/10/06, Linus Torvalds wrote:\n>>\n>> Why don't you use the pipe and standard read()?\n>>\n>> Even if you use \"popen()\" and get a \"FILE *\" back, you can still do\n>>\n>> int fd = fileno(file);\n>>\n>> and use the raw IO capabilities.\n>>\n>> The thing is, temporary files can actually be faster under Linux just\n>> because the Linux page-cache simply kicks ass. But it's not going to be\n>> _that_ big of a difference, and you need all that crazy \"wait for \n>> rev-list\n>> to finish\" and the \"clean up temp-file on errors\" etc crap, so there's no\n>> way it's a better solution.\n>>\n> \n> Two things.\n> \n> - memory use: the next natural step with files is, instead of loading\n> the file content in memory and *keep it there*, we could load one\n> chunk at a time, index the chunk and discard. At the end we keep in\n> memory only indexing info to quickly get to the data when needed, but\n> the big part of data stay on the file.\n> \n\nmemory usage vs speed tradeoff. Since qgit is a pure user-app, I think \nit's safe to opt for the memory hungry option. If people run it on too \nlowbie hardware they'll just have to make do with other ways of viewing \nthe DAG or shutting down some other programs.\n\n> - This is probably my ignorance, but experimenting with popen() I\n> found I could not know *when* git-rev-list ends because both feof()\n> and ferror() give 0 after a fread() with git-rev-list already defunct.\n> Not having a reference to the process (it is hidden behind popen() ),\n> I had to check for 0 bytes read after a successful read (to avoid\n> racing in case I ask the pipe before the first data it's ready) to\n> know that job is finished and call pclose().\n> \n\n(coding in MUA, so highly untested)\n\npid_t runcmd(const char **argv, const char **env, int *out, *int err)\n{\n\tint pid;\n\n\tpipe(out);\n\tpipe(err);\n\n\tpid = fork();\n\n\n\t/* parent returns upon forking */\n\tif (pid) {\n\t\t/* close childs file-descriptors in our address space */\n\t\tclose(out[0]);\n\t\tclose(err[0]);\n\t\treturn pid;\n\t}\n\n\t/* child */\n\t/* close parents file-descriptors */\n\tclose(out[0]);\n\tclose(err[0]);\n\n\t/* stdout and stderr writes to childs ends of the pipes */\n\tdup2(out[1], STDOUT_FILENO);\n\tdup2(err[1], STDERR_FILENO);\n\texecve(argv[0], argv, NULL);\n\n\t_exit(0); /* not reached unless execve() failed */\n}\n\nThe caller can now read out[0] and err[0] as regular file-descriptors \nand has the pid of the child-process in the return value. The parent \nreceives SIGCHILD if the child exits, the child receives EPIPE when \nwriting if the parent crashes.\n\nIn a prudent implementation, the parent should waitpid(pid_t pid, int \n*status, WNOHANG) for the child every once in a while to collect its \nexit status gracefully.\n\nDo some experimenting and I'm sure you'll find something suitable for you.\n\n-- \nAndreas Ericsson andreas.ericsson@op5.se\nOP5 AB www.op5.se\n"},{"id":"297972","messageId":"e5bfff550612110452t5bb18517yc0cf8278d4d9948c@mail.gmail.com","threadId":"43076","inReplyTo":"200612111026.23656.Josef.Weidendorfer@gmx.de","subject":"Re: [RFC \\ WISH] Add -o option to git-rev-list","fromName":"Marco Costalba","fromEmail":"mcostalba@gmail.com","sentAt":"2006-12-11T12:52:39Z","receivedAt":"2006-12-11T12:52:39Z","isPatch":false,"sender":{"key":"mcostalba@gmail.com","avatar":null},"body":"On 12/11/06, Josef Weidendorfer wrote:\n> On Monday 11 December 2006 01:15, Marco Costalba wrote:\n> > guiUpdateTimer.start(100, true);\n>\n> What is the result with \"guiUpdateTimer.start(0, true);\" ?\n> There is no need to put in any time interval at all, because\n> the timeout is a normal event which will be queued in the GUI\n> event queue.\n>\n> If there were X events in the mean time, they are queued and handled\n> before your timeOut function is called again. So the GUI will be\n> responsive, even if you have a 0 ms timer.\n>\n\nIOW you suggest to use a brute force polling of the pipe.\n\nI'will try this, just to test the pipe, not the application because I\nwould really need to read data in big chunks.\n\nData read from pipe/file or something else is read in an array (char*\n[]) and do not touched anymore, no more copies are made. For every\nread a new array is allocated on the heap.\n\nWith chunks of 64KB, because each rev descriptions is about few\nhundered bytes only few revs (about 1% with current linux tree) fall\nat the boundary of the chunk, half in one and the rest in the next.\nThis boundary revs require a special handling that at the end involves\nan additional copy.\n\nIf I get from the pipe very small data (few KB) probably it would be\nbetter to read always in a local array of fixed size and then copy to\nfinal destination in memory. But this imply that all the data work\ncopy is duplicated: instead of 30MB of data we'll going to move 60MB\n(linux tree)\n\nWith big chunks of 64KB only about 1% of data (revs that fall at the\narray boundary) is duplicated.\n\n"},{"id":"294169","messageId":"e5bfff550612110459w205cb9b3lf735359012f84f7c@mail.gmail.com","threadId":"43076","inReplyTo":"457D4386.70408@op5.se","subject":"Re: [RFC \\ WISH] Add -o option to git-rev-list","fromName":"Marco Costalba","fromEmail":"mcostalba@gmail.com","sentAt":"2006-12-11T12:59:10Z","receivedAt":"2006-12-11T12:59:10Z","isPatch":false,"sender":{"key":"mcostalba@gmail.com","avatar":null},"body":"On 12/11/06, Andreas Ericsson wrote:\n> Marco Costalba wrote:\n> > On 12/10/06, Linus Torvalds wrote:\n> >>\n> >> Why don't you use the pipe and standard read()?\n> >>\n> >> Even if you use \"popen()\" and get a \"FILE *\" back, you can still do\n> >>\n> >> int fd = fileno(file);\n> >>\n> >> and use the raw IO capabilities.\n> >>\n> >> The thing is, temporary files can actually be faster under Linux just\n> >> because the Linux page-cache simply kicks ass. But it's not going to be\n> >> _that_ big of a difference, and you need all that crazy \"wait for\n> >> rev-list\n> >> to finish\" and the \"clean up temp-file on errors\" etc crap, so there's no\n> >> way it's a better solution.\n> >>\n> >\n> > Two things.\n> >\n> > - memory use: the next natural step with files is, instead of loading\n> > the file content in memory and *keep it there*, we could load one\n> > chunk at a time, index the chunk and discard. At the end we keep in\n> > memory only indexing info to quickly get to the data when needed, but\n> > the big part of data stay on the file.\n> >\n>\n> memory usage vs speed tradeoff. Since qgit is a pure user-app, I think\n> it's safe to opt for the memory hungry option. If people run it on too\n> lowbie hardware they'll just have to make do with other ways of viewing\n> the DAG or shutting down some other programs.\n>\n> > - This is probably my ignorance, but experimenting with popen() I\n> > found I could not know *when* git-rev-list ends because both feof()\n> > and ferror() give 0 after a fread() with git-rev-list already defunct.\n> > Not having a reference to the process (it is hidden behind popen() ),\n> > I had to check for 0 bytes read after a successful read (to avoid\n> > racing in case I ask the pipe before the first data it's ready) to\n> > know that job is finished and call pclose().\n> >\n>\n> (coding in MUA, so highly untested)\n>\n\nThanks Andreas, I will do some tests with your code. But at first\nsight I fail to see (I'm not an expert on this tough ;-) ) where is\nthe difference from using popen() and fileno() to get the file\ndescriptors.\n\nThanks\n"},{"id":"293874","messageId":"200612111428.29390.Josef.Weidendorfer@gmx.de","threadId":"43076","inReplyTo":"e5bfff550612110452t5bb18517yc0cf8278d4d9948c@mail.gmail.com","subject":"Re: [RFC \\ WISH] Add -o option to git-rev-list","fromName":"Josef Weidendorfer","fromEmail":"josef.weidendorfer@gmx.de","sentAt":"2006-12-11T13:28:28Z","receivedAt":"2006-12-11T13:28:28Z","isPatch":false,"sender":{"key":"josef.weidendorfer@gmx.de","avatar":null},"body":"On Monday 11 December 2006 13:52, Marco Costalba wrote:\n> On 12/11/06, Josef Weidendorfer wrote:\n> > On Monday 11 December 2006 01:15, Marco Costalba wrote:\n> > > guiUpdateTimer.start(100, true);\n> >\n> > What is the result with \"guiUpdateTimer.start(0, true);\" ?\n> ...\n> IOW you suggest to use a brute force polling of the pipe.\n\nAh, yes. That is probably not what you want.\n\nWhy did you introduce the timer at all? What was the problem\nwith QProcess and handling its signal \"data available\" ?\n\nIf you do\n\n\tgit-rev-list | cat > /dev/null\n\nthe consuming \"cat\" will do a blocking read, and this should have\nthe same property \"handle data as fast as possible when they\ncome available\" as QProcess with handling the data in\nthe handler of QProcess' signal \"data available\".\n\nIf it is about postprocessing the small data chunks you get (and the\ndouble copy in memory), I do not think this is a problem: a machine\nnoadays usually should handle 1 GB/s bandwidth to memory. You probably\nare loosing more with your 100ms timer.\n\nOr am I missing something?\n\n"},{"id":"293815","messageId":"457D5FE7.3010309@op5.se","threadId":"43076","inReplyTo":"e5bfff550612110459w205cb9b3lf735359012f84f7c@mail.gmail.com","subject":"Re: [RFC \\ WISH] Add -o option to git-rev-list","fromName":"Andreas Ericsson","fromEmail":"ae@op5.se","sentAt":"2006-12-11T13:40:55Z","receivedAt":"2006-12-11T13:40:55Z","isPatch":false,"sender":{"key":"ae@op5.se","avatar":"https://gravatar.com/avatar/426e89595c75a8f5252dd0c989e5fabe5bcac616e68557427ad9aef6b0ca342a?d=mp&s=160"},"body":"Marco Costalba wrote:\n> On 12/11/06, Andreas Ericsson wrote:\n>> Marco Costalba wrote:\n>> > On 12/10/06, Linus Torvalds wrote:\n>> >>\n>> >> Why don't you use the pipe and standard read()?\n>> >>\n>> >> Even if you use \"popen()\" and get a \"FILE *\" back, you can still do\n>> >>\n>> >> int fd = fileno(file);\n>> >>\n>> >> and use the raw IO capabilities.\n>> >>\n>> >> The thing is, temporary files can actually be faster under Linux just\n>> >> because the Linux page-cache simply kicks ass. But it's not going \n>> to be\n>> >> _that_ big of a difference, and you need all that crazy \"wait for\n>> >> rev-list\n>> >> to finish\" and the \"clean up temp-file on errors\" etc crap, so \n>> there's no\n>> >> way it's a better solution.\n>> >>\n>> >\n>> > Two things.\n>> >\n>> > - memory use: the next natural step with files is, instead of loading\n>> > the file content in memory and *keep it there*, we could load one\n>> > chunk at a time, index the chunk and discard. At the end we keep in\n>> > memory only indexing info to quickly get to the data when needed, but\n>> > the big part of data stay on the file.\n>> >\n>>\n>> memory usage vs speed tradeoff. Since qgit is a pure user-app, I think\n>> it's safe to opt for the memory hungry option. If people run it on too\n>> lowbie hardware they'll just have to make do with other ways of viewing\n>> the DAG or shutting down some other programs.\n>>\n>> > - This is probably my ignorance, but experimenting with popen() I\n>> > found I could not know *when* git-rev-list ends because both feof()\n>> > and ferror() give 0 after a fread() with git-rev-list already defunct.\n>> > Not having a reference to the process (it is hidden behind popen() ),\n>> > I had to check for 0 bytes read after a successful read (to avoid\n>> > racing in case I ask the pipe before the first data it's ready) to\n>> > know that job is finished and call pclose().\n>> >\n>>\n>> (coding in MUA, so highly untested)\n>>\n> \n> Thanks Andreas, I will do some tests with your code. But at first\n> sight I fail to see (I'm not an expert on this tough ;-) ) where is\n> the difference from using popen() and fileno() to get the file\n> descriptors.\n> \n\nread() vs fread(), so no libc buffers. When I did comparisons with this \n(a long time ago, I don't have the test-program around) in style of\n\n\tread(out[0], buf, sizeof(buf));\n\twrite(fileno(stdout), buf, sizeof(buf));\n\nwith a command line like this;\n\n\tcat any-file | test-program > /dev/null\n\nI saw a static ~10ms increase in execution time compared to\n\n\tcat any-file > /dev/null\n\nregardless of the size of \"any-file\", so I assume this overhead comes \nfrom the extra fork(), which you'll never get rid of unless you use \nlibgit.a.\n\n-- \nAndreas Ericsson andreas.ericsson@op5.se\nOP5 AB www.op5.se\n"},{"id":"296695","messageId":"Pine.LNX.4.64.0612110855550.12500@woody.osdl.org","threadId":"43076","inReplyTo":"e5bfff550612102317w331f3b0fyc3c6b095ff21191a@mail.gmail.com","subject":"Re: [RFC \\ WISH] Add -o option to git-rev-list","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-12-11T16:59:01Z","receivedAt":"2006-12-11T16:59:01Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 11 Dec 2006, Marco Costalba wrote:\n> \n> These are tipical values (warm cache):\n> \n> $ time git rev-list --header --boundary --parents --topo-order HEAD | cat > /dev/null\n> 3.67user 0.36system 0:04.29elapsed 93%CPU (0avgtext+0avgdata 0maxresident)k\n> 0inputs+0outputs (0major+18033minor)pagefaults 0swaps\n\nThat's not timing what I asked. That's just timing the \"git-rev-list\". You \nneed to time the \"cat\" part too. Either use a script, or do something like \n\n\ttime sh -c \"git-rev-list ... | cat > /dev/null\".\n\n> $ time git rev-list --header --boundary --parents --topo-order HEAD >\n> /tmp/tmp.txt; cat /tmp/tmp.txt > /dev/null\n> 3.44user 0.28system 0:03.74elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k\n> 0inputs+0outputs (0major+18033minor)pagefaults 0swaps\n\nAgain - you're only timing the _writer_, not the \"cat\" at all.\n\n> For some reason the CPU *never* goes up 93% with pipe (while it's easy\n> in the other two cases)\n\nThat's because you're only timing 93% of the work (ignoring the \"cat\" \npart), and in the other cases you're ignoring the \"cat\" (that happens \n_afterwards_, not concurrently) entirely.\n\n> OK. I just don't understand how after waiting 100ms I get only 60KB\n> and stay in the loop only one cycle instead of reading,\n\n\nThat's because the _writer_ will block. It notices that the reader cannot\nread ass fast as it can write, so it blocks when its buffers fill up.\n\nIf you want to be fast, you need to be fast at reading. It's that simple.\n\n"},{"id":"293918","messageId":"Pine.LNX.4.64.0612110905180.12500@woody.osdl.org","threadId":"43076","inReplyTo":"Pine.LNX.4.64.0612110855550.12500@woody.osdl.org","subject":"Re: [RFC \\ WISH] Add -o option to git-rev-list","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-12-11T17:07:43Z","receivedAt":"2006-12-11T17:07:43Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 11 Dec 2006, Linus Torvalds wrote:\n> \n> That's not timing what I asked. That's just timing the \"git-rev-list\". You \n> need to time the \"cat\" part too. Either use a script, or do something like \n> \n> \ttime sh -c \"git-rev-list ... | cat > /dev/null\".\n\nBtw, if you do this, you'll actually see one of the _real_ advantages of \npipes.\n\nOn an SMP system, you'll easily get output like \"110%CPU\", since now the \ngit-rev-list and the user can run in parallel. Of course, they can do so \nwith temp-files too, but then you have all the \"is it a true EOF, or is it \njust the fact that the writer hasn't written everything yet\" problem (and \nthe _normal_ solution for that is to simply not allow any overlapping \nwork).\n\n"},{"id":"298612","messageId":"e5bfff550612110928h41e05acah4f835caa3fa6fea5@mail.gmail.com","threadId":"43076","inReplyTo":"200612111428.29390.Josef.Weidendorfer@gmx.de","subject":"Re: [RFC \\ WISH] Add -o option to git-rev-list","fromName":"Marco Costalba","fromEmail":"mcostalba@gmail.com","sentAt":"2006-12-11T17:28:46Z","receivedAt":"2006-12-11T17:28:46Z","isPatch":false,"sender":{"key":"mcostalba@gmail.com","avatar":null},"body":"On 12/11/06, Josef Weidendorfer wrote:\n> On Monday 11 December 2006 13:52, Marco Costalba wrote:\n> > On 12/11/06, Josef Weidendorfer wrote:\n> > > On Monday 11 December 2006 01:15, Marco Costalba wrote:\n> > > > guiUpdateTimer.start(100, true);\n> > >\n> > > What is the result with \"guiUpdateTimer.start(0, true);\" ?\n> > ...\n> > IOW you suggest to use a brute force polling of the pipe.\n>\n> Ah, yes. That is probably not what you want.\n>\n> Why did you introduce the timer at all? What was the problem\n> with QProcess and handling its signal \"data available\" ?\n>\n\nI did. Actually the released 1.5.3 version and all the verions before\nthat do use QProcess, but I experimented an increase in performance\nreading the data on a timer instead of on \"data available\" signal.\nBTW that observation led to all this stuff ;-)\n\nThe problem is that the frequency of the signal is very very high\nbecause the producer can write a big amount of data with great speed.\nWe are talking of a signal \"data available\" each few ms.\n\nBy the way of experimenting with QProcess I have found it's better a\ntimer when the stream is very very fast, _then_ I have update the\nreading code to take _more_ adavntage of the big chunks we were\nreading with a low frequency timer and avoid a double copy, but the\nincrease was _already_ present also with the double copy needed by the\noriginal 'small chunks handling' code activated by \"data available\"\nsignal.\n\nIf you take a look at released qgit you will see, in dataloader.cpp\nthe reading slot called on the signal.\n\n"},{"id":"293971","messageId":"e5bfff550612110939r58df1a73ud28983633c12e487@mail.gmail.com","threadId":"43076","inReplyTo":"Pine.LNX.4.64.0612110905180.12500@woody.osdl.org","subject":"Re: [RFC \\ WISH] Add -o option to git-rev-list","fromName":"Marco Costalba","fromEmail":"mcostalba@gmail.com","sentAt":"2006-12-11T17:39:44Z","receivedAt":"2006-12-11T17:39:44Z","isPatch":false,"sender":{"key":"mcostalba@gmail.com","avatar":null},"body":"On 12/11/06, Linus Torvalds wrote:\n>\n>\n> On Mon, 11 Dec 2006, Linus Torvalds wrote:\n> >\n> > That's not timing what I asked. That's just timing the \"git-rev-list\". You\n> > need to time the \"cat\" part too. Either use a script, or do something like\n> >\n> > time sh -c \"git-rev-list ... | cat > /dev/null\".\n>\n> Btw, if you do this, you'll actually see one of the _real_ advantages of\n> pipes.\n>\n> On an SMP system, you'll easily get output like \"110%CPU\", since now the\n> git-rev-list and the user can run in parallel. Of course, they can do so\n> with temp-files too, but then you have all the \"is it a true EOF, or is it\n> just the fact that the writer hasn't written everything yet\" problem (and\n> the _normal_ solution for that is to simply not allow any overlapping\n> work).\n>\n\nSorry for wrong data, I will repost as soon as I can.\n\nRegarding the _normal_ solution we have one more hipotesys to take\nadvantage of: git-rev-list when has nothing more to read..exits.\n\nThis is what lead to the exit loop condition based on git-rev-list is\nstill alive, instead of messing with an EOF.\n\n eof = (no more data && !rev_list_is_running);\n\nWith this we could enter the loop one more time then necessary, but\nnothing else bad should happen.\n\n"},{"id":"298198","messageId":"Pine.LNX.4.64.0612111003140.12500@woody.osdl.org","threadId":"43076","inReplyTo":"e5bfff550612110939r58df1a73ud28983633c12e487@mail.gmail.com","subject":"Re: [RFC \\ WISH] Add -o option to git-rev-list","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-12-11T18:15:27Z","receivedAt":"2006-12-11T18:15:27Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 11 Dec 2006, Marco Costalba wrote:\n> \n> Regarding the _normal_ solution we have one more hipotesys to take\n> advantage of: git-rev-list when has nothing more to read..exits.\n\nYes. You can just wait for the child exit signal.\n\nHowever, you seem to continually ignore the thing I've asked you to do \nseveral times: try with a cold-cache situation.\n\nThe thing is, using pipes and poll()/select()/epoll()/whatever will not \nonly be efficient, but it will also WORK CORRECTLY in the presense of a \nwriter that is _slower_ than the reader.\n\nRight now, you're testing the exact opposite. You're testing the case \nwhere the reader is slower than the writer, which is actually not that \ninteresting - because if \"git-rev-list\" is instantaneous, then the fastest \nthing to do is to simply _always_ just read the result buffer directly \ninto memory without any select() loop etc what-so-ever.\n\nSo just by testing that case (and you've slowed down the reader \nartificially even _apart_ from the fact that qgit will probably always be \nslower than git-rev-list off a packed and hot-cache environment), you're \nalways going to skew your results into the \"do everything in one go\" \ndirection.\n\nBut the point of the pipe and the poll() is that it works nicely even when \nthe data trickles in slowly.\n\n[ Of course, as long as you ask for \"--topo-order\", you'll never see a lot \n of trickling, and you'll never be able to do really well for the \n cold-cache case. To see the _real_ advantage of pipes, you should void \n \"--topo-order\" entirely, and do it dynamically within qgit, repainting \n the graph as needed.\n\n At that point, you'd actually do something that gitk can't do at all, \n namely work well for the cold-cache not-very-packed large-repository \n case. ]\n\nTo see this in practice (even with hot-caches), do something like the \nfollowing on the full historic Linux archive:\n\n\ttime sh -c \"git rev-list HEAD | head\"\n\ttime sh -c \"git rev-list --topo-order HEAD | head\"\n\nwhere for me, the firstone takes 0.002s, and the second one takes 0.878s.\n\nNow THAT is an optimization. Not just \"10%\" or even \"ten times\", but \"four \nHUNDRED times\" faster.\n\nAnd why is that? Simply because it only needed to look at _part_ of the \ndata. For the exact same reason, if you were to do the topological sort \nonly on the part that you had _looked_ at first, you'd be able to do these \nkinds of several-orders-of-magnitude improvements.\n\n"},{"id":"294182","messageId":"e5bfff550612111059g35f1aa2dmdead17a60f5dfca8@mail.gmail.com","threadId":"43076","inReplyTo":"Pine.LNX.4.64.0612111003140.12500@woody.osdl.org","subject":"Re: [RFC \\ WISH] Add -o option to git-rev-list","fromName":"Marco Costalba","fromEmail":"mcostalba@gmail.com","sentAt":"2006-12-11T18:59:22Z","receivedAt":"2006-12-11T18:59:22Z","isPatch":false,"sender":{"key":"mcostalba@gmail.com","avatar":null},"body":"On 12/11/06, Linus Torvalds wrote:\n>\n>\n>\n> However, you seem to continually ignore the thing I've asked you to do\n> several times: try with a cold-cache situation.\n\nYes. I will test it. I was testing with warm cache also to my\ncuriosity about comparing pipes with temporary file reading as data\nexchange facility. So I needed to avoid HD artifacts.\n\n>\n>\n> At that point, you'd actually do something that gitk can't do at all,\n> namely work well for the cold-cache not-very-packed large-repository\n> case. ]\n>\n\nRemoving --topo-order from launch parameters is very simple, see from\ngitstartup.cpp:\n\nbool Git::startRevList(SCRef args, FileHistory* fh) {\n\n\tQString initCmd(\"git rev-list --header --boundary --parents \");\n\tif (!isMainHistory(fh)) {\n\t\t// fetch history from all branches so any revision in\n\t\t// main view that changes the file is always found\n\t\tSCRef allBranches = getAllRefSha(BRANCH).join(\" \");\n\t\tinitCmd.append(\"--remove-empty \" + allBranches + \" -- \");\n\t} else\n\t\tinitCmd.append(\"--topo-order \");\n\n\treturn startParseProc(initCmd + args, fh);\n}\n\nIt is already removed from file history fetching because there the\nhistory it's _almost_ linear and you gain the speed without noticing\nthe different graph layout. But for main view I fear that complicated\nbranch structures could became difficult to follow without\n--topo-order. Perhaps an option in settings?\n\n> To see this in practice (even with hot-caches), do something like the\n> following on the full historic Linux archive:\n>\n> time sh -c \"git rev-list HEAD | head\"\n> time sh -c \"git rev-list --topo-order HEAD | head\"\n>\n> where for me, the firstone takes 0.002s, and the second one takes 0.878s.\n>\n\nThis is the data:\n\n$ pwd\n/git/linux-2.6\nbash-3.1$ time sh -c \"git rev-list HEAD | head\"\n200d018eff4be3a1fb9823441cfcebb7de86a677\nf0647a52974daccbe20990fb6341f07792445fe0\nc25c79d80e02db1bd993426f979c5f1b42a0f132\n9fd32cfbb602f3e5e898faa61d83c4a7897bd48a\ned99e2bc1dc5dc54eb5a019f4975562dbef20103\n773ff78838ca3c07245e45c06235e0baaa5f710a\n52ffe760ea9ec407292d093c3f06c1cda5187228\n14b36af46a1d3652aff6734ea24816995dff8123\neb991b39385c7b04923d701764a34694ec54b53d\n88032b322a38b37335c8cb2e3473a45c81d280eb\n0.08user 0.02system 0:00.10elapsed 100%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+3631minor)pagefaults 0swaps\n\nbash-3.1$ time sh -c \"git rev-list --topo-order HEAD | head\"\n200d018eff4be3a1fb9823441cfcebb7de86a677\nf0647a52974daccbe20990fb6341f07792445fe0\nc25c79d80e02db1bd993426f979c5f1b42a0f132\n9fd32cfbb602f3e5e898faa61d83c4a7897bd48a\ned99e2bc1dc5dc54eb5a019f4975562dbef20103\n773ff78838ca3c07245e45c06235e0baaa5f710a\n52ffe760ea9ec407292d093c3f06c1cda5187228\n14b36af46a1d3652aff6734ea24816995dff8123\neb991b39385c7b04923d701764a34694ec54b53d\n33ec32fae0e2c4433bfd1e74cbde6cb16604a719\n2.84user 0.12system 0:03.05elapsed 97%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+12086minor)pagefaults 0swaps\n\nAlso the long awaited ;-)\n\n$ time sh -c \"git rev-list --header --boundary --parents --topo-order\nHEAD | cat > /dev/null\"\n3.75user 0.55system 0:04.42elapsed 97%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+18822minor)pagefaults 0swaps\n\n$ time sh -c \"git rev-list --header --boundary --parents --topo-order\nHEAD > /tmp/tmp.txt; cat /tmp/tmp.txt > /dev/null\"\n3.50user 0.30system 0:03.92elapsed 97%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+18800minor)pagefaults 0swaps\n\n\n"},{"id":"297405","messageId":"Pine.LNX.4.64.0612111120190.12500@woody.osdl.org","threadId":"43076","inReplyTo":"e5bfff550612111059g35f1aa2dmdead17a60f5dfca8@mail.gmail.com","subject":"Re: [RFC \\ WISH] Add -o option to git-rev-list","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-12-11T19:25:46Z","receivedAt":"2006-12-11T19:25:46Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 11 Dec 2006, Marco Costalba wrote:\n> \n> $ time sh -c \"git rev-list --header --boundary --parents --topo-order HEAD | cat > /dev/null\"\n> $ time sh -c \"git rev-list --header --boundary --parents --topo-order HEAD > /tmp/tmp.txt; cat /tmp/tmp.txt > /dev/null\"\n\nBtw, for me the numbers aren't stable enough to be al lthat final, but \ndoing it multiple times on the kernel archive I get:\n\n\treal 0m0.947s\n\treal 0m1.046s\n\treal 0m0.943s\n\treal 0m0.868s\n\nfor the pipe case, and\n\n\treal 0m0.953s\n\treal 0m1.056s\n\treal 0m1.060s\n\treal 0m0.968s\n\nfor the temp-file. IOW, no real difference. Just doing the \"> /dev/null\" \ndirectly is more consistent (no task-switching, SMP effects, or cache \nconflicts) at\n\n\treal 0m0.806s\n\treal 0m0.806s\n\treal 0m0.805s\n\treal 0m0.808s\n\nbut one recurrent theme is definitely true: the \"cat\" part is a noticeable \noverhead. But that in itself means that git-rev-list is just so efficient \nthat even just basically copying its output is going to be measurable. \n\nWhich is obviously a good thing, but it means that it's not very \ninteresting in the big picture (ie I bet the real overheads are going to \nbe elsewhere if you actually expect to draw anything on the screen).\n\n"},{"id":"297257","messageId":"200612112128.06485.Josef.Weidendorfer@gmx.de","threadId":"43076","inReplyTo":"e5bfff550612111059g35f1aa2dmdead17a60f5dfca8@mail.gmail.com","subject":"Re: [RFC \\ WISH] Add -o option to git-rev-list","fromName":"Josef Weidendorfer","fromEmail":"josef.weidendorfer@gmx.de","sentAt":"2006-12-11T20:28:05Z","receivedAt":"2006-12-11T20:28:05Z","isPatch":false,"sender":{"key":"josef.weidendorfer@gmx.de","avatar":null},"body":"On Monday 11 December 2006 19:59, Marco Costalba wrote:\n> On 12/11/06, Linus Torvalds wrote:\n> >\n> >\n> >\n> > However, you seem to continually ignore the thing I've asked you to do\n> > several times: try with a cold-cache situation.\n> \n> Yes. I will test it. I was testing with warm cache also to my\n> curiosity about comparing pipes with temporary file reading as data\n> exchange facility. So I needed to avoid HD artifacts.\n\nHi,\n\nI just looked at the QProcess implementation in Qt 3.3.6, as I was\ncurious.\n\nQt does a big select() call in the event loop. If there is data\navailable from the child process, it is reading in chunks of a\nmaximum of 4096 bytes, with a select() call inbetween to see if\nthere is still data available. After every read, the read data\nis concatenated into the read buffer.\n\nFor the slow/cold cache case, this probably is the best *if* the\nconsumer application can act as fast as possible when data is\nsent. Which makes it a good fit to avoid --topo-order and do\nsome redrawing of the graph yourself if needed. For sure, this\ngives the fastest visual appeareance. You could start filling\nthe list after e.g. 30 revs are read.\n\nObviously, there is some possibility for improvement _when_ you\nknow that you want to read in large amounts of data in big\nchunks, given that QProcess uses at least two system calls every\n4 kB.\n\nA general question: How many context switches are involved in such\na producer/consumer scenario, given that the consumer writes one\nline at a time, and the consumer uses poll/select to wait for the\ndata?\nIs there some possibility to make the kernel write-combine single\nsmall producer writes into bigger chunks, which will be delivered\nat once (or smaller data only after a small timeout)?\n\n"},{"id":"297393","messageId":"Pine.LNX.4.64.0612111238560.3515@woody.osdl.org","threadId":"43076","inReplyTo":"200612112128.06485.Josef.Weidendorfer@gmx.de","subject":"Re: [RFC \\ WISH] Add -o option to git-rev-list","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-12-11T20:40:37Z","receivedAt":"2006-12-11T20:40:37Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 11 Dec 2006, Josef Weidendorfer wrote:\n> \n> A general question: How many context switches are involved in such\n> a producer/consumer scenario, given that the consumer writes one\n> line at a time, and the consumer uses poll/select to wait for the\n> data?\n> Is there some possibility to make the kernel write-combine single\n> small producer writes into bigger chunks, which will be delivered\n> at once (or smaller data only after a small timeout)?\n\nThe data will be write-combined.\n\nThe kernel doesn't context-switch after a write() to a pipe if there is \nspace left (and usually the pipe buffer is 64kB with current kernels, so \nthere obviously will be), unless the reader has a higher priority for some \nreason (ie the reader has been waiting a long time).\n\nSo _normally_ you'll see many many writes in one go, and only see context \nswitching when the kernel pipe buffer fills up.\n\n"},{"id":"298223","messageId":"200612112154.56166.Josef.Weidendorfer@gmx.de","threadId":"43076","inReplyTo":"Pine.LNX.4.64.0612111238560.3515@woody.osdl.org","subject":"Re: [RFC \\ WISH] Add -o option to git-rev-list","fromName":"Josef Weidendorfer","fromEmail":"josef.weidendorfer@gmx.de","sentAt":"2006-12-11T20:54:55Z","receivedAt":"2006-12-11T20:54:55Z","isPatch":false,"sender":{"key":"josef.weidendorfer@gmx.de","avatar":null},"body":"On Monday 11 December 2006 21:40, Linus Torvalds wrote:\n> On Mon, 11 Dec 2006, Josef Weidendorfer wrote:\n> > A general question: How many context switches are involved in such\n> > a producer/consumer scenario, given that the consumer writes one\n> > line at a time, and the consumer uses poll/select to wait for the\n> > data?\n> > Is there some possibility to make the kernel write-combine single\n> > small producer writes into bigger chunks, which will be delivered\n> > at once (or smaller data only after a small timeout)?\n> \n> The data will be write-combined.\n> \n> The kernel doesn't context-switch after a write() to a pipe if there is \n> space left (and usually the pipe buffer is 64kB with current kernels, so \n> there obviously will be), unless the reader has a higher priority for some \n> reason (ie the reader has been waiting a long time).\n\nAh, thanks.\nSo the implementation in Qt's QProcess is a little bit pessimistic, probably\nto make it work fine with other UNIXs out there.\n\n"},{"id":"297814","messageId":"Pine.LNX.4.64.0612111258050.3515@woody.osdl.org","threadId":"43076","inReplyTo":"200612112154.56166.Josef.Weidendorfer@gmx.de","subject":"Re: [RFC \\ WISH] Add -o option to git-rev-list","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-12-11T21:14:42Z","receivedAt":"2006-12-11T21:14:42Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 11 Dec 2006, Josef Weidendorfer wrote:\n> \n> So the implementation in Qt's QProcess is a little bit pessimistic, probably\n> to make it work fine with other UNIXs out there.\n\nI suspect it may actually have been performance-optimized for Linux-2.4, \nwhere the pipe buffer size was one page (so if you got a full 4kB on x86, \nyou'd know that the buffer was fully emptied).\n\nAnd it actually does end up depending on the load too. For example, the \n4kB can also happen to match the default buffer size in stdio (or in the \nQT IO buffering equivalent), and in that case you know that any _writer_ \nthat uses stdio buffering will usually use that as the blocksize, so you \nhave reasons beyond the kernel to think that maybe a certain particular \nblocksize is more common than others.\n\nAnd if that's the case, then if you know that _usually_ the read() would \nreturn -EAGAIN, then it's actually better to return to the main loop and \nselect(), on the assumption that quite often, the select will be the right \nthing (and if it isn't, it will just return immediately anyway, and you \ncan do the second read).\n\nIn other words, it's really a \"tuning thing\" that depends on the IO \npatterns of the apps involved, and the underlying pattern of the OS can \naffect it too.\n\nOf course, most of the time it doesn't make _that_ much of a difference, \nespecially when system calls are reasonably cheap (and they largely are, \nunder Linux). So you can certainly see the effects of this, but the \neffects will vary so much on the load and other issues that it's hard to \nsay that there is \"one correct way\" to do things.\n\nFor example, very subtle issues can be at play:\n\n - using a larger buffer and having fewer system calls can be \n advantageous in many cases. But certainly not always.\n\n HOWEVER. If you have two processes that both do very little with the \n data (generating it _or_ using it), and process switching is cheap \n thanks to address space identifiers or clever TLB's, then a small \n buffer may actually be much better, if it means that the whole working \n set fits in the L2 cache.\n\n So a large buffer with fewer system calls may actually be _bad_, if \n only because it causes things to go to L3 or memory. In contrast, if \n you can re-use a smaller buffer, you'll stay in the L2 cache better. \n\n Same goes for the L1 cache too, but that's really only noticeable for \n true micro-benchmarks. Sadly, people often do performance analysis with \n microbenches, so they can actually tune their setup for something that \n is NOT what people actually see in real life.\n\n In general, if you re-use your buffer, try to keep it small enough that \n it stays hot in the L2 can be a good thing. Using 8kB or 16kB buffers \n and 64-128 system calls can actually be a lot BETTER than using a 1MB \n buffer and a single system call.\n\n Examples of this is stdio. Making your stdio buffers bigger simply \n often isn't a good idea. The system call cost of flushing the buffer \n simply isn't likely to be the highest cost.\n\n - real-time or fairness issues. Sometimes you are better off doing a \n round-robin thing in the main select() loop over 10 smaller buffers \n than over ten big buffers (or over a single mainloop call-out that \n keeps doing read() until the buffer is entirely empty). \n\n This is not likely to be an issue with qgit-like behaviour (whether you \n process a 4kB buffer or 64kB buffer in one go is simply not going to \n make a visible difference to a user interface), but it can make a \n difference for \"server\" apps with latency issues. You do not want one \n busy client to basically freeze out all the other clients by just \n keeping its queue filled all teh time.\n\n So returning to the main loop may actually be the right thing after \n each read(), not for performance reasons, but because it gives a nicer \n load pattern. It may perform _worse_, but it may well give a smoother \n feel to the system!\n\n - simplicity. The main select() loop does the looping anyway, so avoiding \n looping in the callouts simplifies the system. And since it's not \n necessarily the right thing to do to loop in the individual routines \n _anyway_ (see above), why not?\n\nSo I don't think there is a right answer here. I suspect QProcess does ok.\n\n"},{"id":"298155","messageId":"e5bfff550612151045q5782e1f2j8686ccab24dbf566@mail.gmail.com","threadId":"43076","inReplyTo":"Pine.LNX.4.64.0612111258050.3515@woody.osdl.org","subject":"Re: [RFC \\ WISH] Add -o option to git-rev-list","fromName":"Marco Costalba","fromEmail":"mcostalba@gmail.com","sentAt":"2006-12-15T18:45:32Z","receivedAt":"2006-12-15T18:45:32Z","isPatch":false,"sender":{"key":"mcostalba@gmail.com","avatar":null},"body":"On 12/11/06, Linus Torvalds wrote:\n>\n>\n> So I don't think there is a right answer here. I suspect QProcess does ok.\n>\n\nI have just pushed a patch (wiil be available in few hours I suspect)\nto easily choose the data exchange facility with git-rev-list.\n\nDefault its temporary file based, but after uncommenting the\nUSE_QPROCESS define in dataloader.cpp and compile again, qgit will use\na QProcess based approach to connect with git-rev-list.\n\nOnly the low level connection is different in the two cases, _all_ the\nrest, expecially data storing and parsing, is the same. If you take a\nlook at src/dataloader.cpp you will see that the difference between\nQProcess and temp file code is minumum.\n\nI've also tried to clearly report in comments the different approaches\nand the data copy involved in both cases. I've searched a little bit\nin QProcess sources too to get some additional info.\n\n\nAnd finally these are mine warm and cold cache tests.\n\nLoad time test on Linux tree (44557 revs, 32167KB)\nCPU Mobile Pentium 4 set at 1.2GHZ\n\nTo set data read interval in ms use (src/dataloader.cpp):\n#define GUI_UPDATE_INTERVAL 500\n\nWarmed-up cache\nQProcess 7632ms (500ms data read interval)\nQProcess 7972ms (100ms data read interval)\n\nTemporary file 4408ms (500ms data read interval)\nTemporary file 4591ms (100ms data read interval)\n\n\nCold cache\nQProcess 25611ms (500ms data read interval)\n\nFile 22399ms (500ms data read interval)\n\n\n"},{"id":"297452","messageId":"Pine.LNX.4.64.0612151118270.3849@woody.osdl.org","threadId":"43076","inReplyTo":"e5bfff550612151045q5782e1f2j8686ccab24dbf566@mail.gmail.com","subject":"Re: [RFC \\ WISH] Add -o option to git-rev-list","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-12-15T19:20:47Z","receivedAt":"2006-12-15T19:20:47Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Fri, 15 Dec 2006, Marco Costalba wrote:\n> \n> Warmed-up cache\n> QProcess 7632ms (500ms data read interval)\n> QProcess 7972ms (100ms data read interval)\n\nWhy do you even bother posting numbers, when multiple people have told you \nthat the numbers you post are meaningless?\n\nAs long as you throttle the writer by not reading data in a timely fashion \n(by using poll() or select() in the main loop and reading when it's \navailable), and you continue to talk about \"data read intervals\", all your \nnumbers are CRAP.\n\nOF COURSE a temp-file will work better, because then you basically have an \ninfinite buffer, and data read intervals don't _matter_. But we've tried \nto tell you that this is not because temp-files are better, but because \nyour reader artificially throttles the writer for a pipe.\n\n"},{"id":"296581","messageId":"e5bfff550612151241pd11f49eqd1666ce0b33855b6@mail.gmail.com","threadId":"43076","inReplyTo":"Pine.LNX.4.64.0612151118270.3849@woody.osdl.org","subject":"Re: [RFC \\ WISH] Add -o option to git-rev-list","fromName":"Marco Costalba","fromEmail":"mcostalba@gmail.com","sentAt":"2006-12-15T20:41:28Z","receivedAt":"2006-12-15T20:41:28Z","isPatch":false,"sender":{"key":"mcostalba@gmail.com","avatar":null},"body":"On 12/15/06, Linus Torvalds wrote:\n>\n>\n> On Fri, 15 Dec 2006, Marco Costalba wrote:\n> >\n> > Warmed-up cache\n> > QProcess 7632ms (500ms data read interval)\n> > QProcess 7972ms (100ms data read interval)\n>\n> Why do you even bother posting numbers, when multiple people have told you\n> that the numbers you post are meaningless?\n>\n> As long as you throttle the writer by not reading data in a timely fashion\n> (by using poll() or select() in the main loop and reading when it's\n> available), and you continue to talk about \"data read intervals\", all your\n> numbers are CRAP.\n>\n\nQProcess implementation is like this FWIK (from\nqt-x11-free-3.3.4/src/kernel/qprocess_unix.cpp):\n\nThe SIGCHLD handler writes to a socket to tell the manager that\nsomething happened. Then in QProcessManager::sigchldHnd() data is read by\n\nprocess->socketRead( proc->socketStdout );\n\n\nIn QProcess::socketRead() a call to C function read() is done to get\n4KB of data:\n\nconst int basize = 4096;\nQByteArray *ba = new QByteArray( basize );\nn = ::read( fd, ba->data(), basize );\n\n\nThe pointer ba is then appended to a pointer list.\n\nThis happens _ALWAYS_ indipendently of _how_ the application calls the\nQt library for reading.\n\nWhen application read recieved data with QProcess::readStdout(), then\ndata stored in buffers pointed by the pointer list is memcpy() to a\ntemporary array that is the return value of QProcess::readStdout().\nSee in qt-x11-free-3.3.4/src/kernel/qprocess.cpp:\n\nQByteArray QProcess::readStdout()\n{\n if ( readStdoutCalled ) {\n\treturn QByteArray();\n }\n readStdoutCalled = TRUE;\n QMembuf *buf = membufStdout();\n readStdoutCalled = FALSE;\n\n return buf->readAll(); // here a memcpy() is involved\n}\n\n\nSo it' true that we use a timeout, but only to trigger a memcpy() from\nQt internal library buffers that, instead are feeded as soon as\npossible by Qt implementation.\n\nIOW the timely select() is already done by Qt library. We just read\nwhat has been already received and stored.\n\n Marco\n\n"},{"id":"293858","messageId":"e5bfff550612151304v647106c9v3f042b424bf6cbfd@mail.gmail.com","threadId":"43076","inReplyTo":"e5bfff550612151241pd11f49eqd1666ce0b33855b6@mail.gmail.com","subject":"Re: [RFC \\ WISH] Add -o option to git-rev-list","fromName":"Marco Costalba","fromEmail":"mcostalba@gmail.com","sentAt":"2006-12-15T21:04:17Z","receivedAt":"2006-12-15T21:04:17Z","isPatch":false,"sender":{"key":"mcostalba@gmail.com","avatar":null},"body":"On 12/15/06, Marco Costalba wrote:\n>\n> IOW the timely select() is already done by Qt library. We just read\n> what has been already received and stored.\n>\n\nTry to set the timer to a very high value so that never triggers and\nthere is only one read call at the end, after git-rev-list exited,\nwhen there is a forced call to the data read function.\n\nTimes seems to get even better! This is because timely read of the\npipe is _already_ done inside Qt library, not by qgit application.\n\n"}]}