66 patches and counting

66 patches and counting

[Michael Haggerty]

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/

Re: 66 patches and counting

[Junio C Hamano]

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.

Re: 66 patches and counting

[Jonathan Nieder]

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

Michael Haggerty wrote:

> 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.

Re: 66 patches and counting

[Martin Fick]

On Wednesday, October 05, 2011 03:29:57 pm Michael Haggerty 
wrote:
> 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

Re: 66 patches and counting

[Martin Fick]

On Thursday, October 06, 2011 04:16:39 pm Martin Fick wrote:
> 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

Re: 66 patches and counting

[Michael Haggerty]

[-- Attachment #1: Type: text/plain, Size: 2236 bytes --]

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/

[-- Attachment #2: refperf-summary.out --]
[-- Type: text/plain, Size: 3535 bytes --]

===================================  =======  =======  =======  =======  =======  =======
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)

Scalable reference handling

[Michael Haggerty]

[-- Attachment #1: Type: text/plain, Size: 1666 bytes --]

On 10/07/2011 05:14 AM, Michael Haggerty wrote:
> 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/

[-- Attachment #2: refperf-summary-3.out --]
[-- Type: text/plain, Size: 4983 bytes --]

===================================  =======  =======  =======  =======  =======  =======  =======  =======  =======
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)

Re: Scalable reference handling

[Martin Fick]

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