{"thread":{"id":"28595","subject":"66 patches and counting","startedAt":"2011-10-05T21:29:57Z","lastAt":"2011-10-07T18:51:43Z","messageCount":8,"participants":["Michael Haggerty","Junio C Hamano","Jonathan Nieder","Martin Fick"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"176962","messageId":"4E8CCC55.9070408@alum.mit.edu","threadId":"28595","inReplyTo":null,"subject":"66 patches and counting","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-10-05T21:29:57Z","receivedAt":"2011-10-05T21:29:57Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"My renovation of refs.c [1] is currently at 66 patches and counting.\nWhat can I say?: (1) I like to make changes in the smallest irreducible\nsteps and (2) there is a lot that needed to be done in refs.c.\n\nWhen I'm done, is it OK to dump a patch series like that on the git\nmailing list?  Is it pointless because nobody will review them anyway?\nIs a big pile of changes like this welcome in any form?  Would it be\nbetter to convey the changes via git itself (e.g., github) rather than\nvia emails?\n\nMichael\n\n[1] hierarchical-refs at git://github.com/mhagger/git.git\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"176964","messageId":"7vty7n5dfi.fsf@alter.siamese.dyndns.org","threadId":"28595","inReplyTo":"4E8CCC55.9070408@alum.mit.edu","subject":"Re: 66 patches and counting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-05T21:36:01Z","receivedAt":"2011-10-05T21:36:01Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> When I'm done, is it OK to dump a patch series like that on the git\n> mailing list?\n\nBits and pieces that are of digestable sizes. \n\nI would wait sending anything large-ish (like more than 10 patches) before\n'next' is rewound, which is the sign the cycle is in full swing.\n\n> Is it pointless because nobody will review them anyway?\n\nBits and pieces that are of digestable sizes. Especially given:\n\n> I like to make changes in the smallest irreducible steps\n\neach step in your series should make sense by itself already (otherwise\nyou would have rebase-i'ed them to collapse too-small pieces into one that\nmakes sense standalone on top of the earlier steps).\n\n> Is a big pile of changes like this welcome in any form?\n\nBits and pieces that are of digestable sizes.\n"},{"id":"176967","messageId":"20111005215505.GA12747@elie","threadId":"28595","inReplyTo":"4E8CCC55.9070408@alum.mit.edu","subject":"Re: 66 patches and counting","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-10-05T21:55:05Z","receivedAt":"2011-10-05T21:55:05Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"(+cc: Elijah, who has more experience in this subject than I do)\nHi,\n\nMichael Haggerty wrote:\n\n> My renovation of refs.c [1] is currently at 66 patches and counting.\n> What can I say?: (1) I like to make changes in the smallest irreducible\n> steps and (2) there is a lot that needed to be done in refs.c.\n>\n> When I'm done\n\nWe've seen series with fifty-something patches on this list before.\nMy (generic) advice:\n\n 1. Send in installments, early and often.  It would not be fun if the\n    first ten patches have a fatal flaw that means the later ones have\n    to be reworked.\n\n 2. Make sure the cover letter makes people want to read the later\n    patches.  Make sure each patch has a commit message that motivates\n    it alone or explains how it fits into the larger picture.\n\n 3. When a patch is not intended to cause any functional change, say\n    so, so reviewers can check that.\n\n 4. Include test scripts declaring what effect (or lack thereof) each\n    patch is supposed to have.\n\n 5. \"Smallest irreducible step\" is not necessarily the appropriate\n    granularity when publishing.  \"Largest piece that a person would\n    want to review, apply, or revert independently\" is.\n"},{"id":"177121","messageId":"201110061616.39381.mfick@codeaurora.org","threadId":"28595","inReplyTo":"4E8CCC55.9070408@alum.mit.edu","subject":"Re: 66 patches and counting","fromName":"Martin Fick","fromEmail":"mfick@codeaurora.org","sentAt":"2011-10-06T22:16:39Z","receivedAt":"2011-10-06T22:16:39Z","isPatch":false,"sender":{"key":"mfick@codeaurora.org","avatar":null},"body":"On Wednesday, October 05, 2011 03:29:57 pm Michael Haggerty \nwrote:\n> My renovation of refs.c [1] is currently at 66 patches\n> and counting. What can I say?: (1) I like to make\n> changes in the smallest irreducible steps and (2) there\n> is a lot that needed to be done in refs.c.\n> \n> When I'm done, is it OK to dump a patch series like that\n> on the git mailing list?  Is it pointless because nobody\n> will review them anyway? Is a big pile of changes like\n> this welcome in any form?  Would it be better to convey\n> the changes via git itself (e.g., github) rather than\n> via emails?\n> \n> Michael\n> \n> [1] hierarchical-refs at git://github.com/mhagger/git.git\n\nMichael,\n\nI downloaded your patch series and tested it on my repos.\n\nHere are some of the timings I saw with your branch as is:\n\n * git clone                               2:50m  (same)\n * full fetch changes                  (> 1 hour) (bad!)\n * git branch (unpacked, ungced)            .7s   (good!)\n * git branch (packed, gced)                .18s  (~>same)\n * git checkout (unpacked, ungced)          10.5s (~>same)\n * git checkout (packed, gced)               9.5  (~>same)\n * noop fetch changes (unpacked, ungced)    14s   (~>same)\n * noop fetch changes (packed, gced)        12s   (same)\n\nFor the full fetch, I estimated, things were scrolling by \nslow enough that after about 15 min I interrupted it. I \nsuspect it might be at least 6 times longer (if rate stayed \nthe same).\n\n\nHere are the best timings for all the good patches that \nothers have submitted to fix many of the previous problems I \nbrought up:\n\n * git clone                               2:50m\n * full fetch changes                      4:50m   \n * git branch (unpacked, ungced)              9s\n * git branch (packed, gced)                  .05s\n * git checkout (unpacked, ungced)            9s\n * git checkout (packed, gced)                8s\n * noop fetch changes (unpacked, ungced)     12s\n * noop fetch changes (packed, gced)         12s\n\n(my internal patches bring full fetch down to 2:50m)\n\nIt would be nice if you could rebase your work on top of \nsome of the other patches also so that I could see those \nresults. I might give that a try if I have the time and it \nis easy (or I might rebase those patches on yours).\n\nThanks,\n\n-Martin\n\n-- \nEmployee of Qualcomm Innovation Center, Inc. which is a \nmember of Code Aurora Forum\n"},{"id":"177130","messageId":"201110061959.34018.mfick@codeaurora.org","threadId":"28595","inReplyTo":"201110061616.39381.mfick@codeaurora.org","subject":"Re: 66 patches and counting","fromName":"Martin Fick","fromEmail":"mfick@codeaurora.org","sentAt":"2011-10-07T01:59:33Z","receivedAt":"2011-10-07T01:59:33Z","isPatch":false,"sender":{"key":"mfick@codeaurora.org","avatar":null},"body":"On Thursday, October 06, 2011 04:16:39 pm Martin Fick wrote:\n> On Wednesday, October 05, 2011 03:29:57 pm Michael\n> Haggerty\n> > [1] hierarchical-refs at\n> > git://github.com/mhagger/git.git\n> \n> I downloaded your patch series and tested it on my repos.\n> \n>  * full fetch changes                  (> 1 hour) (bad!)\n\nI bisected this problem, it was introduced in this commit:\n\n\n  commit e12ce45b4f1bd8ed6652a742b7e6cf6f101b3604\n  Author: Michael Haggerty <mhagger@alum.mit.edu>\n  Date:   Wed Oct 5 11:30:06 2011 +0200\n\n    Store references hierarchically\n    \n    This slightly changes the order of iteration over \nreferences; now\n    references are strictly sorted componentwise rather than \nas\n    \"/\"-containing strings as before.  For example, \n\"subspace/one\" now\n    sorts before \"subspace-x\", whereas before the order was \nreversed.\n    \n    Tweak a test case to accept the new ordering.\n\n\n\nUp until that point, the fetch looks pretty good,\n\n-Martin\n\n-- \nEmployee of Qualcomm Innovation Center, Inc. which is a \nmember of Code Aurora Forum\n"},{"id":"177133","messageId":"4E8E6E8E.5070909@alum.mit.edu","threadId":"28595","inReplyTo":"201110061616.39381.mfick@codeaurora.org","subject":"Re: 66 patches and counting","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-10-07T03:14:22Z","receivedAt":"2011-10-07T03:14:22Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 10/07/2011 12:16 AM, Martin Fick wrote:\n> I downloaded your patch series and tested it on my repos.\n\nVery cool (though a bit premature, as you discovered).  The patch series\nstill has a known performance regression in the area of\ndo_for_each_ref(), which I hope to figure out soon.  I will definitely\ntell you when I think that the patch series is ready for more serious\ntesting (hopefully today) in the hopes that you can benchmark it against\nyour repo.\n\nIn the future, please tell me the SHA1 of any versions that you test, as\nI am still frequently non-ff updating the hierarchical-refs branch.\n\n> Here are the best timings for all the good patches that \n> others have submitted to fix many of the previous problems I \n> brought up:\n\nWhat are, in your measurements, the \"good patches\" that you consider\ncontenders performance-wise?  (I've lost track because there have been\nso many suggestions in this area.)\n\nIt would be great if you would serve as a kind of benchmarking\nreferee/clearinghouse for the various suggested patches.  I have been\nbenchmarking with some rough scripts that I wrote [1]; the current\nstatus is appended below (the numbers are clock times in seconds).  FWIW\nthe attached output was generated using roughly the following commands:\n\nt/make-refperf-repo --refs=10000 --commits=20000\ncp t/refperf-many .\n# Adjust REFPERF_BRANCH in ./refperf-many:\n$EDITOR ./refperf-many\nrevs=\"v1.7.6 v1.7.7 origin/master origin/hierarchical-refs^^\norigin/hierarchical-refs^ origin/hierarchical-refs origin/master\"\n./refperf-many $revs\nt/refperf-summary $revs >refperf-summary.out\n\nSee the comments at the top of the scripts for more details.  Please\nsuggest more tests to be added to t/refperf!\n\n> It would be nice if you could rebase your work on top of \n> some of the other patches also so that I could see those \n> results. I might give that a try if I have the time and it \n> is easy (or I might rebase those patches on yours).\n\nI believe that at least some of the other patches will be superseded by\nmine.  When I get my patch series done I will look into it in more detail...\n\nMichael\n\n[1] Branch \"refperf\" at git://github.com/mhagger/git.git\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n\n\n===================================  =======  =======  =======  =======  =======  =======\nTest name                                [0]      [1]      [2]      [3]      [4]      [5]\n===================================  =======  =======  =======  =======  =======  =======\nbranch-loose-cold                       5.84     5.38     6.05     0.53     0.47     0.56\nbranch-loose-warm                       0.12     0.11     0.13     0.00     0.02     0.03\nfor-each-ref-loose-cold                 6.83     6.45     6.13     8.21     8.69     8.28\nfor-each-ref-loose-warm                 0.25     0.25     0.25     0.84     0.84     0.84\ncheckout-loose-cold                     4.55     5.08     5.76     0.73     0.60     0.68\ncheckout-loose-warm                     0.11     0.11     0.12     0.03     0.02     0.04\ncheckout-orphan-loose                   0.10     0.10     0.11     0.01     0.02     0.01\ncheckout-from-detached-loose            0.97     1.05     0.95     7.80     8.27     8.37\nbranch-contains-loose-cold             14.98    15.44    18.54    16.46    16.91    17.54\nbranch-contains-loose-warm              9.44     8.97    10.15     9.60     9.58     9.71\npack-refs-loose                         0.97     1.00     1.37     1.69     1.62     1.65\nbranch-packed-cold                      0.49     0.63     0.81     1.38     1.37     1.45\nbranch-packed-warm                      0.02     0.02     0.05     0.77     0.74     0.80\nfor-each-ref-packed-cold                1.18     0.97     1.56     1.33     2.03     1.51\nfor-each-ref-packed-warm                0.15     0.15     0.16     0.52     0.52     0.50\ncheckout-packed-cold                    0.84     0.60     0.70     0.96     0.94     1.28\ncheckout-packed-warm                    0.02     0.09     0.04     0.41     0.41     0.44\ncheckout-orphan-packed                  0.01     0.02     0.10     0.38     0.39     0.42\ncheckout-from-detached-packed           6.94     7.35     9.18     1.31     1.35     1.45\nbranch-contains-packed-cold            11.19    10.81    12.35    10.91    10.09    10.16\nbranch-contains-packed-warm            10.24     9.90    11.19     9.13     9.07     9.13\nclone-loose-cold                       93.77    90.65    98.07    99.03   103.72   101.85\nclone-loose-warm                        3.12     3.19     3.24     3.56     3.60     3.50\nfetch-nothing-loose                     0.85     0.84     0.87     1.20     1.18     1.21\npack-refs                               0.12     0.13     0.14     0.51     0.54     0.51\nfetch-nothing-packed                    0.85     0.84     0.86     1.20     1.19     1.20\nclone-packed-cold                       2.17     2.38     2.50     2.28     2.46     2.34\nclone-packed-warm                       0.32     0.34     0.39     0.40     0.38     0.30\nfetch-everything-cold                  92.37    95.96   102.20   100.80   104.72   106.85\nfetch-everything-warm                   5.55     5.65     5.53     6.26     6.27     6.35\n===================================  =======  =======  =======  =======  =======  =======\n\n\n[0] v1.7.6 (refperf.times.d78c84e8698e750139667bc724b08eb34e795b65)\n[1] v1.7.7 (refperf.times.a258e475eb74e183e9e68ca30e32c5253081356d)\n[2] origin/master (refperf.times.27897d25f1b36d400b82b655701b87fd205dbc2f)\n[3] origin/hierarchical-refs^^ (refperf.times.eabcf1ed884d95878f12ca5ea32a3e20c70194f2)\n[4] origin/hierarchical-refs^ (refperf.times.be9118234275c4fa6002ef97d989b0d28c94c0bd)\n[5] origin/hierarchical-refs (refperf.times.74d1ae38f70f4f97745caf235ef3f34b3e326ad4)\n\n"},{"id":"177158","messageId":"4E8F2012.90108@alum.mit.edu","threadId":"28595","inReplyTo":"4E8E6E8E.5070909@alum.mit.edu","subject":"Scalable reference handling","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-10-07T15:51:46Z","receivedAt":"2011-10-07T15:51:46Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 10/07/2011 05:14 AM, Michael Haggerty wrote:\n> On 10/07/2011 12:16 AM, Martin Fick wrote:\n>> I downloaded your patch series and tested it on my repos.\n> \n> Very cool (though a bit premature, as you discovered).  The patch series\n> still has a known performance regression in the area of\n> do_for_each_ref(), which I hope to figure out soon.  I will definitely\n> tell you when I think that the patch series is ready for more serious\n> testing (hopefully today) in the hopes that you can benchmark it against\n> your repo.\n\nI just pushed versions to github that I think are ready for some\npreliminary testing.  There were some silly inefficiencies in the\nversion that you tested earlier, so this version is considerably faster\nin a few key tests.\n\nI don't have complete benchmarking results, but I have attached what I\nhave.  I wouldn't put much weight on small differences in the numbers\nbecause the computer was not 100% quiescent while I ran the tests.  But\nI think the results are impressive: the new code (columns 5-8) is a bit\nslower in only a few cases but faster (sometimes by a large factor) in\nmany other cases.\n\nI can't write more now, but Martin, if you have time to benchmark\n9944c7faf903a95d4ed9de284ace32debe21cdc1 against your repository, I\nwould be very interested to learn the results.\n\nBTW I am not asking anybody to review the patch series yet; I would like\nto do some more tests and cleanup first.  But of course I wouldn't\nobject to feedback.  A good starting point would be the comments at the\ntop of refs.c, where the basic data structures are explained.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n\n\n===================================  =======  =======  =======  =======  =======  =======  =======  =======  =======\nTest name                                [0]      [1]      [2]      [3]      [4]      [5]      [6]      [7]      [8]\n===================================  =======  =======  =======  =======  =======  =======  =======  =======  =======\nbranch-loose-cold                       5.14     5.80     4.98     4.55     5.03     0.41     0.45     0.45     0.51\nbranch-loose-warm                       0.10     0.12     0.12     0.19     0.11     0.02     0.02     0.03     0.02\nfor-each-ref-loose-cold                 6.02     6.36     7.48     5.62     6.27     5.32     5.02     5.24     5.01\nfor-each-ref-loose-warm                 0.25     0.25     0.27     0.25     0.25     0.26     0.26     0.25     0.26\ncheckout-loose-cold                     5.47     5.03     5.66     5.39     5.72     0.65     0.77     0.95     0.59\ncheckout-loose-warm                     0.11     0.11     0.11     0.11     0.11     0.02     0.03     0.03     0.03\ncheckout-orphan-loose                   0.09     0.09     0.10     0.10     0.10     0.03     0.01     0.01     0.02\ncheckout-from-detached-loose-cold        N/A      N/A      N/A      N/A      N/A      N/A      N/A      N/A      N/A\ncheckout-from-detached-loose-warm        N/A      N/A      N/A      N/A      N/A      N/A      N/A      N/A      N/A\nbranch-contains-loose-cold             14.69    13.95    13.76    13.84    14.07    14.08    14.12    14.01    14.77\nbranch-contains-loose-warm              8.99     8.89     8.74     8.82     8.80     8.81     8.84     8.76     8.91\npack-refs-loose                         1.23     1.02     1.02     1.02     1.01     1.02     1.00     0.99     1.03\nbranch-packed-cold                      0.66     0.65     0.84     0.67     0.52     0.76     0.58     0.55     0.59\nbranch-packed-warm                      0.01     0.01     0.03     0.03     0.02     0.02     0.05     0.02     0.05\nfor-each-ref-packed-cold                1.38     1.29     1.07     1.13     1.05     1.08     1.26     1.05     1.27\nfor-each-ref-packed-warm                0.17     0.16     0.17     0.17     0.17     0.16     0.17     0.16     0.16\ncheckout-packed-cold                    8.74     7.75     7.75     1.59     1.61     1.79     1.54     1.55     1.53\ncheckout-packed-warm                    0.03     0.05     0.04     0.04     0.05     0.05     0.03     0.05     0.06\ncheckout-orphan-packed                  0.04     0.01     0.01     0.03     0.02     0.02     0.05     0.05     0.02\ncheckout-from-detached-packed-cold      8.88     8.22     8.12     1.98     1.81     1.94     1.99     1.88     1.97\ncheckout-from-detached-packed-warm      6.55     6.43     6.44     0.44     0.46     0.45     0.46     0.46     0.49\nbranch-contains-packed-cold            11.59    10.73    10.37     9.42     9.24     9.40     9.37     9.56     9.34\nbranch-contains-packed-warm            10.20     9.79     9.83     8.68     8.61     8.68     8.61     8.82     9.33\nclone-loose-cold                      105.58    86.98    89.42    88.96    88.19    87.01    87.20    87.11    88.20\nclone-loose-warm                        3.26     3.11     3.18     3.10     3.17     3.14     3.14     3.21     3.16\nfetch-nothing-loose                     0.85     0.84     0.86     0.85     0.84     0.88     0.84     0.84     0.84\npack-refs                               0.12     0.14     0.16     0.13     0.15     0.14     0.14     0.15     0.14\nfetch-nothing-packed                    0.84     0.84     0.86     0.85     0.89     0.85     0.84     0.84     0.83\nclone-packed-cold                       2.72     2.33     2.11     2.19     2.23     2.14     2.24     2.01     2.16\nclone-packed-warm                       0.40     0.40     0.33     0.40     0.28     0.29     0.33     0.31     0.31\nfetch-everything-cold                 106.28    92.92    94.12    88.76    91.62    88.75    89.49    89.78    91.43\nfetch-everything-warm                   5.56     5.63     5.70     5.62     5.63     5.66     5.69     5.71     5.78\n===================================  =======  =======  =======  =======  =======  =======  =======  =======  =======\n\n\n[0] v1.7.6 (refperf.times.d78c84e8698e750139667bc724b08eb34e795b65)\n[1] v1.7.7 (refperf.times.a258e475eb74e183e9e68ca30e32c5253081356d)\n[2] origin/master (refperf.times.27897d25f1b36d400b82b655701b87fd205dbc2f)\n[3] 16583974c20b856bb60b5a733020425c16a19670^ (refperf.times.558b49c8489c95cf966b959c3444c95d177dc4dc)\n[4] 16583974c20b856bb60b5a733020425c16a19670 (refperf.times.16583974c20b856bb60b5a733020425c16a19670)\n[5] origin/hierarchical-refs^^^^ (refperf.times.5f5a126553eef88455f7deb2745c5f93073bfe69)\n[6] origin/hierarchical-refs^^^ (refperf.times.a306af145856f8296bf2ff4a3ace5e86ac3b1fc8)\n[7] origin/hierarchical-refs^ (refperf.times.fd53cf7a7fcc360f30ea0ec5b964cefeec70c11b)\n[8] origin/hierarchical-refs (refperf.times.9944c7faf903a95d4ed9de284ace32debe21cdc1)\n\n"},{"id":"177172","messageId":"201110071251.44052.mfick@codeaurora.org","threadId":"28595","inReplyTo":"4E8F2012.90108@alum.mit.edu","subject":"Re: Scalable reference handling","fromName":"Martin Fick","fromEmail":"mfick@codeaurora.org","sentAt":"2011-10-07T18:51:43Z","receivedAt":"2011-10-07T18:51:43Z","isPatch":false,"sender":{"key":"mfick@codeaurora.org","avatar":null},"body":"On Friday, October 07, 2011 09:51:46 am Michael Haggerty \nwrote:\n> I can't write more now, but Martin, if you have time to\n> benchmark 9944c7faf903a95d4ed9de284ace32debe21cdc1\n> against your repository, I would be very interested to\n> learn the results.\n\nThe fetch no longer seems to suffer from the large \nregression, it is now faster (~7m) than 1.7.7 (which was \n+15m).\n\n\nAs a quick note, if I comment out the \ninvalidate_cached_refs() call in write_ref_sha1() on line  \n2065 (on top of 9944c7), it is still much faster, only ~2m.  \nPerhaps growing the array on the fly with many refs is still \nbe too inefficient?\n\n\n-Martin\n\n-- \nEmployee of Qualcomm Innovation Center, Inc. which is a \nmember of Code Aurora Forum\n"}]}