threads / discuss / 28595

66 patches and counting

Subject: 66 patches and counting

## tl;dr

8 messages between Oct 5, 2011 and Oct 7, 2011.

replies: 7people: 4as markdown or json

Michael Haggerty· Oct 5, 2011, 21:29 UTC · lore

My renovation of refs.c [1] is currently at 66 patches and counting. What can I say?: (1) I like to make changes in the smallest irreducible steps and (2) there is a lot that needed to be done in refs.c.

When I'm done, is it OK to dump a patch series like that on the git mailing list? Is it pointless because nobody will review them anyway? Is a big pile of changes like this welcome in any form? Would it be better to convey the changes via git itself (e.g., github) rather than via emails?

Michael
[1] hierarchical-refs at git://github.com/mhagger/git.git
-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
Junio C Hamano· Oct 5, 2011, 21:36 UTC · re: Michael Haggerty · lore

Re: 66 patches and counting

Michael Haggerty <mhagger@alum.mit.edu> writes:
> When I'm done, is it OK to dump a patch series like that on the git
> mailing list?
Bits and pieces that are of digestable sizes. 

I would wait sending anything large-ish (like more than 10 patches) before 'next' is rewound, which is the sign the cycle is in full swing.

> Is it pointless because nobody will review them anyway?
Bits and pieces that are of digestable sizes. Especially given:
> I like to make changes in the smallest irreducible steps

each step in your series should make sense by itself already (otherwise you would have rebase-i'ed them to collapse too-small pieces into one that makes sense standalone on top of the earlier steps).

> Is a big pile of changes like this welcome in any form?
Bits and pieces that are of digestable sizes.
Jonathan Nieder· Oct 5, 2011, 21:55 UTC · re: Michael Haggerty · lore

Re: 66 patches and counting

(+cc: Elijah, who has more experience in this subject than I do) Hi,

Michael Haggerty wrote:
Show 5 quoted lines
> My renovation of refs.c [1] is currently at 66 patches and counting.
> What can I say?: (1) I like to make changes in the smallest irreducible
> steps and (2) there is a lot that needed to be done in refs.c.
>
> When I'm done

We've seen series with fifty-something patches on this list before. My (generic) advice:

 1. Send in installments, early and often.  It would not be fun if the
    first ten patches have a fatal flaw that means the later ones have
    to be reworked.
 2. Make sure the cover letter makes people want to read the later
    patches.  Make sure each patch has a commit message that motivates
    it alone or explains how it fits into the larger picture.
 3. When a patch is not intended to cause any functional change, say
    so, so reviewers can check that.
 4. Include test scripts declaring what effect (or lack thereof) each
    patch is supposed to have.
 5. "Smallest irreducible step" is not necessarily the appropriate
    granularity when publishing.  "Largest piece that a person would
    want to review, apply, or revert independently" is.
Martin Fick· Oct 6, 2011, 22:16 UTC · re: Michael Haggerty · lore

Re: 66 patches and counting

On Wednesday, October 05, 2011 03:29:57 pm Michael Haggerty wrote:

Show 15 quoted lines
> My renovation of refs.c [1] is currently at 66 patches
> and counting. What can I say?: (1) I like to make
> changes in the smallest irreducible steps and (2) there
> is a lot that needed to be done in refs.c.
> 
> When I'm done, is it OK to dump a patch series like that
> on the git mailing list?  Is it pointless because nobody
> will review them anyway? Is a big pile of changes like
> this welcome in any form?  Would it be better to convey
> the changes via git itself (e.g., github) rather than
> via emails?
> 
> Michael
> 
> [1] hierarchical-refs at git://github.com/mhagger/git.git
Michael,
I downloaded your patch series and tested it on my repos.
Here are some of the timings I saw with your branch as is:
 * git clone                               2:50m  (same)
 * full fetch changes                  (> 1 hour) (bad!)
 * git branch (unpacked, ungced)            .7s   (good!)
 * git branch (packed, gced)                .18s  (~>same)
 * git checkout (unpacked, ungced)          10.5s (~>same)
 * git checkout (packed, gced)               9.5  (~>same)
 * noop fetch changes (unpacked, ungced)    14s   (~>same)
 * noop fetch changes (packed, gced)        12s   (same)

For the full fetch, I estimated, things were scrolling by slow enough that after about 15 min I interrupted it. I suspect it might be at least 6 times longer (if rate stayed the same).

Here are the best timings for all the good patches that others have submitted to fix many of the previous problems I brought up:

 * git clone                               2:50m
 * full fetch changes                      4:50m   
 * git branch (unpacked, ungced)              9s
 * git branch (packed, gced)                  .05s
 * git checkout (unpacked, ungced)            9s
 * git checkout (packed, gced)                8s
 * noop fetch changes (unpacked, ungced)     12s
 * noop fetch changes (packed, gced)         12s
(my internal patches bring full fetch down to 2:50m)

It would be nice if you could rebase your work on top of some of the other patches also so that I could see those results. I might give that a try if I have the time and it is easy (or I might rebase those patches on yours).

Thanks,
-Martin
-- 
Employee of Qualcomm Innovation Center, Inc. which is a 
member of Code Aurora Forum
Martin Fick· Oct 7, 2011, 01:59 UTC · re: Martin Fick · lore

Re: 66 patches and counting

On Thursday, October 06, 2011 04:16:39 pm Martin Fick wrote:
Show 8 quoted lines
> On Wednesday, October 05, 2011 03:29:57 pm Michael
> Haggerty
> > [1] hierarchical-refs at
> > git://github.com/mhagger/git.git
> 
> I downloaded your patch series and tested it on my repos.
> 
>  * full fetch changes                  (> 1 hour) (bad!)
I bisected this problem, it was introduced in this commit:
  commit e12ce45b4f1bd8ed6652a742b7e6cf6f101b3604
  Author: Michael Haggerty <mhagger@alum.mit.edu>
  Date:   Wed Oct 5 11:30:06 2011 +0200
    Store references hierarchically
    
    This slightly changes the order of iteration over 
references; now
    references are strictly sorted componentwise rather than 
as
    "/"-containing strings as before.  For example, 
"subspace/one" now
    sorts before "subspace-x", whereas before the order was 
reversed.
    
    Tweak a test case to accept the new ordering.
Up until that point, the fetch looks pretty good,
-Martin
-- 
Employee of Qualcomm Innovation Center, Inc. which is a 
member of Code Aurora Forum
Michael Haggerty· Oct 7, 2011, 03:14 UTC · re: Martin Fick · lore

Re: 66 patches and counting

On 10/07/2011 12:16 AM, Martin Fick wrote:
> I downloaded your patch series and tested it on my repos.

Very cool (though a bit premature, as you discovered). The patch series still has a known performance regression in the area of do_for_each_ref(), which I hope to figure out soon. I will definitely tell you when I think that the patch series is ready for more serious testing (hopefully today) in the hopes that you can benchmark it against your repo.

In the future, please tell me the SHA1 of any versions that you test, as I am still frequently non-ff updating the hierarchical-refs branch.

> Here are the best timings for all the good patches that 
> others have submitted to fix many of the previous problems I 
> brought up:

What are, in your measurements, the "good patches" that you consider contenders performance-wise? (I've lost track because there have been so many suggestions in this area.)

It would be great if you would serve as a kind of benchmarking referee/clearinghouse for the various suggested patches. I have been benchmarking with some rough scripts that I wrote [1]; the current status is appended below (the numbers are clock times in seconds). FWIW the attached output was generated using roughly the following commands:

t/make-refperf-repo --refs=10000 --commits=20000 cp t/refperf-many . # Adjust REFPERF_BRANCH in ./refperf-many: $EDITOR ./refperf-many revs="v1.7.6 v1.7.7 origin/master origin/hierarchical-refs^^ origin/hierarchical-refs^ origin/hierarchical-refs origin/master" ./refperf-many $revs t/refperf-summary $revs >refperf-summary.out

See the comments at the top of the scripts for more details. Please suggest more tests to be added to t/refperf!

> It would be nice if you could rebase your work on top of 
> some of the other patches also so that I could see those 
> results. I might give that a try if I have the time and it 
> is easy (or I might rebase those patches on yours).

I believe that at least some of the other patches will be superseded by mine. When I get my patch series done I will look into it in more detail...

Michael
[1] Branch "refperf" at git://github.com/mhagger/git.git
-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/


===================================  =======  =======  =======  =======  =======  =======
Test name                                [0]      [1]      [2]      [3]      [4]      [5]
===================================  =======  =======  =======  =======  =======  =======
branch-loose-cold                       5.84     5.38     6.05     0.53     0.47     0.56
branch-loose-warm                       0.12     0.11     0.13     0.00     0.02     0.03
for-each-ref-loose-cold                 6.83     6.45     6.13     8.21     8.69     8.28
for-each-ref-loose-warm                 0.25     0.25     0.25     0.84     0.84     0.84
checkout-loose-cold                     4.55     5.08     5.76     0.73     0.60     0.68
checkout-loose-warm                     0.11     0.11     0.12     0.03     0.02     0.04
checkout-orphan-loose                   0.10     0.10     0.11     0.01     0.02     0.01
checkout-from-detached-loose            0.97     1.05     0.95     7.80     8.27     8.37
branch-contains-loose-cold             14.98    15.44    18.54    16.46    16.91    17.54
branch-contains-loose-warm              9.44     8.97    10.15     9.60     9.58     9.71
pack-refs-loose                         0.97     1.00     1.37     1.69     1.62     1.65
branch-packed-cold                      0.49     0.63     0.81     1.38     1.37     1.45
branch-packed-warm                      0.02     0.02     0.05     0.77     0.74     0.80
for-each-ref-packed-cold                1.18     0.97     1.56     1.33     2.03     1.51
for-each-ref-packed-warm                0.15     0.15     0.16     0.52     0.52     0.50
checkout-packed-cold                    0.84     0.60     0.70     0.96     0.94     1.28
checkout-packed-warm                    0.02     0.09     0.04     0.41     0.41     0.44
checkout-orphan-packed                  0.01     0.02     0.10     0.38     0.39     0.42
checkout-from-detached-packed           6.94     7.35     9.18     1.31     1.35     1.45
branch-contains-packed-cold            11.19    10.81    12.35    10.91    10.09    10.16
branch-contains-packed-warm            10.24     9.90    11.19     9.13     9.07     9.13
clone-loose-cold                       93.77    90.65    98.07    99.03   103.72   101.85
clone-loose-warm                        3.12     3.19     3.24     3.56     3.60     3.50
fetch-nothing-loose                     0.85     0.84     0.87     1.20     1.18     1.21
pack-refs                               0.12     0.13     0.14     0.51     0.54     0.51
fetch-nothing-packed                    0.85     0.84     0.86     1.20     1.19     1.20
clone-packed-cold                       2.17     2.38     2.50     2.28     2.46     2.34
clone-packed-warm                       0.32     0.34     0.39     0.40     0.38     0.30
fetch-everything-cold                  92.37    95.96   102.20   100.80   104.72   106.85
fetch-everything-warm                   5.55     5.65     5.53     6.26     6.27     6.35
===================================  =======  =======  =======  =======  =======  =======


[0] v1.7.6 (refperf.times.d78c84e8698e750139667bc724b08eb34e795b65)
[1] v1.7.7 (refperf.times.a258e475eb74e183e9e68ca30e32c5253081356d)
[2] origin/master (refperf.times.27897d25f1b36d400b82b655701b87fd205dbc2f)
[3] origin/hierarchical-refs^^ (refperf.times.eabcf1ed884d95878f12ca5ea32a3e20c70194f2)
[4] origin/hierarchical-refs^ (refperf.times.be9118234275c4fa6002ef97d989b0d28c94c0bd)
[5] origin/hierarchical-refs (refperf.times.74d1ae38f70f4f97745caf235ef3f34b3e326ad4)
Michael Haggerty· Oct 7, 2011, 15:51 UTC · re: Michael Haggerty · lore

Scalable reference handling

On 10/07/2011 05:14 AM, Michael Haggerty wrote:
Show 9 quoted lines
> On 10/07/2011 12:16 AM, Martin Fick wrote:
>> I downloaded your patch series and tested it on my repos.
> 
> Very cool (though a bit premature, as you discovered).  The patch series
> still has a known performance regression in the area of
> do_for_each_ref(), which I hope to figure out soon.  I will definitely
> tell you when I think that the patch series is ready for more serious
> testing (hopefully today) in the hopes that you can benchmark it against
> your repo.

I just pushed versions to github that I think are ready for some preliminary testing. There were some silly inefficiencies in the version that you tested earlier, so this version is considerably faster in a few key tests.

I don't have complete benchmarking results, but I have attached what I have. I wouldn't put much weight on small differences in the numbers because the computer was not 100% quiescent while I ran the tests. But I think the results are impressive: the new code (columns 5-8) is a bit slower in only a few cases but faster (sometimes by a large factor) in many other cases.

I can't write more now, but Martin, if you have time to benchmark 9944c7faf903a95d4ed9de284ace32debe21cdc1 against your repository, I would be very interested to learn the results.

BTW I am not asking anybody to review the patch series yet; I would like to do some more tests and cleanup first. But of course I wouldn't object to feedback. A good starting point would be the comments at the top of refs.c, where the basic data structures are explained.

Michael
-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/


===================================  =======  =======  =======  =======  =======  =======  =======  =======  =======
Test name                                [0]      [1]      [2]      [3]      [4]      [5]      [6]      [7]      [8]
===================================  =======  =======  =======  =======  =======  =======  =======  =======  =======
branch-loose-cold                       5.14     5.80     4.98     4.55     5.03     0.41     0.45     0.45     0.51
branch-loose-warm                       0.10     0.12     0.12     0.19     0.11     0.02     0.02     0.03     0.02
for-each-ref-loose-cold                 6.02     6.36     7.48     5.62     6.27     5.32     5.02     5.24     5.01
for-each-ref-loose-warm                 0.25     0.25     0.27     0.25     0.25     0.26     0.26     0.25     0.26
checkout-loose-cold                     5.47     5.03     5.66     5.39     5.72     0.65     0.77     0.95     0.59
checkout-loose-warm                     0.11     0.11     0.11     0.11     0.11     0.02     0.03     0.03     0.03
checkout-orphan-loose                   0.09     0.09     0.10     0.10     0.10     0.03     0.01     0.01     0.02
checkout-from-detached-loose-cold        N/A      N/A      N/A      N/A      N/A      N/A      N/A      N/A      N/A
checkout-from-detached-loose-warm        N/A      N/A      N/A      N/A      N/A      N/A      N/A      N/A      N/A
branch-contains-loose-cold             14.69    13.95    13.76    13.84    14.07    14.08    14.12    14.01    14.77
branch-contains-loose-warm              8.99     8.89     8.74     8.82     8.80     8.81     8.84     8.76     8.91
pack-refs-loose                         1.23     1.02     1.02     1.02     1.01     1.02     1.00     0.99     1.03
branch-packed-cold                      0.66     0.65     0.84     0.67     0.52     0.76     0.58     0.55     0.59
branch-packed-warm                      0.01     0.01     0.03     0.03     0.02     0.02     0.05     0.02     0.05
for-each-ref-packed-cold                1.38     1.29     1.07     1.13     1.05     1.08     1.26     1.05     1.27
for-each-ref-packed-warm                0.17     0.16     0.17     0.17     0.17     0.16     0.17     0.16     0.16
checkout-packed-cold                    8.74     7.75     7.75     1.59     1.61     1.79     1.54     1.55     1.53
checkout-packed-warm                    0.03     0.05     0.04     0.04     0.05     0.05     0.03     0.05     0.06
checkout-orphan-packed                  0.04     0.01     0.01     0.03     0.02     0.02     0.05     0.05     0.02
checkout-from-detached-packed-cold      8.88     8.22     8.12     1.98     1.81     1.94     1.99     1.88     1.97
checkout-from-detached-packed-warm      6.55     6.43     6.44     0.44     0.46     0.45     0.46     0.46     0.49
branch-contains-packed-cold            11.59    10.73    10.37     9.42     9.24     9.40     9.37     9.56     9.34
branch-contains-packed-warm            10.20     9.79     9.83     8.68     8.61     8.68     8.61     8.82     9.33
clone-loose-cold                      105.58    86.98    89.42    88.96    88.19    87.01    87.20    87.11    88.20
clone-loose-warm                        3.26     3.11     3.18     3.10     3.17     3.14     3.14     3.21     3.16
fetch-nothing-loose                     0.85     0.84     0.86     0.85     0.84     0.88     0.84     0.84     0.84
pack-refs                               0.12     0.14     0.16     0.13     0.15     0.14     0.14     0.15     0.14
fetch-nothing-packed                    0.84     0.84     0.86     0.85     0.89     0.85     0.84     0.84     0.83
clone-packed-cold                       2.72     2.33     2.11     2.19     2.23     2.14     2.24     2.01     2.16
clone-packed-warm                       0.40     0.40     0.33     0.40     0.28     0.29     0.33     0.31     0.31
fetch-everything-cold                 106.28    92.92    94.12    88.76    91.62    88.75    89.49    89.78    91.43
fetch-everything-warm                   5.56     5.63     5.70     5.62     5.63     5.66     5.69     5.71     5.78
===================================  =======  =======  =======  =======  =======  =======  =======  =======  =======


[0] v1.7.6 (refperf.times.d78c84e8698e750139667bc724b08eb34e795b65)
[1] v1.7.7 (refperf.times.a258e475eb74e183e9e68ca30e32c5253081356d)
[2] origin/master (refperf.times.27897d25f1b36d400b82b655701b87fd205dbc2f)
[3] 16583974c20b856bb60b5a733020425c16a19670^ (refperf.times.558b49c8489c95cf966b959c3444c95d177dc4dc)
[4] 16583974c20b856bb60b5a733020425c16a19670 (refperf.times.16583974c20b856bb60b5a733020425c16a19670)
[5] origin/hierarchical-refs^^^^ (refperf.times.5f5a126553eef88455f7deb2745c5f93073bfe69)
[6] origin/hierarchical-refs^^^ (refperf.times.a306af145856f8296bf2ff4a3ace5e86ac3b1fc8)
[7] origin/hierarchical-refs^ (refperf.times.fd53cf7a7fcc360f30ea0ec5b964cefeec70c11b)
[8] origin/hierarchical-refs (refperf.times.9944c7faf903a95d4ed9de284ace32debe21cdc1)
Martin Fick· Oct 7, 2011, 18:51 UTC · re: Michael Haggerty · lore

Re: Scalable reference handling

On Friday, October 07, 2011 09:51:46 am Michael Haggerty wrote:

> I can't write more now, but Martin, if you have time to
> benchmark 9944c7faf903a95d4ed9de284ace32debe21cdc1
> against your repository, I would be very interested to
> learn the results.

The fetch no longer seems to suffer from the large regression, it is now faster (~7m) than 1.7.7 (which was +15m).

As a quick note, if I comment out the invalidate_cached_refs() call in write_ref_sha1() on line 2065 (on top of 9944c7), it is still much faster, only ~2m. Perhaps growing the array on the fly with many refs is still be too inefficient?

-Martin
-- 
Employee of Qualcomm Innovation Center, Inc. which is a 
member of Code Aurora Forum

← back to recent threads