From: Thomas Rast <trast@inf.ethz.ch>
Date: Tue, 25 Jun 2013 16:05:05 GMT
Subject: Re: [PATCH 00/16] Speed up Counting Objects with bitmap data
Message-ID: <87hagllvsk.fsf@linux-k42r.v.cablecom.net>
In-Reply-To: <1372116193-32762-1-git-send-email-tanoku@gmail.com>

Vicent Marti <tanoku@gmail.com> writes:

> Like with every other patch that offers performance improvements,
> sample benchmarks are provided (spoiler: they are pretty fucking
> cool).

Great stuff.

I read the first half, and skimmed the second half.  See the individual
replies for comments.

However:

>  Documentation/technical/bitmap-format.txt |  235 ++++++++
>  Makefile                                  |   11 +
>  builtin.h                                 |    1 +
>  builtin/pack-objects.c                    |  362 +++++++-----
>  builtin/pack-objects.h                    |   33 ++
>  builtin/rev-list.c                        |   35 +-
>  builtin/write-bitmap.c                    |  256 +++++++++
>  cache.h                                   |    5 +
>  ewah/bitmap.c                             |  229 ++++++++
>  ewah/ewah_bitmap.c                        |  703 ++++++++++++++++++++++++
>  ewah/ewah_io.c                            |  199 +++++++
>  ewah/ewah_rlw.c                           |  124 +++++
>  ewah/ewok.h                               |  194 +++++++
>  ewah/ewok_rlw.h                           |  114 ++++
>  git-compat-util.h                         |   28 +
>  git-repack.sh                             |   10 +-
>  git.c                                     |    1 +
>  khash.h                                   |  329 +++++++++++
>  list-objects.c                            |    1 +
>  pack-bitmap-write.c                       |  520 ++++++++++++++++++
>  pack-bitmap.c                             |  855 +++++++++++++++++++++++++++++
>  pack-bitmap.h                             |   64 +++
>  pack-write.c                              |    2 +
>  revision.c                                |    5 +
>  revision.h                                |    2 +
>  sha1_file.c                               |   57 +-

It's pretty hard to miss that there isn't a single test in the entire
series.  It seems that the features you add depend on pack.usebitmaps,
and since the tests run with empty config (unless of course they set
their own) your feature is completely untested -- unless I'm missing
something.

I imagine the tests would be of the format

test_expect_success 'do <stuff> without bitmaps' '
	git ... >expect
'

test_expect_success 'do <stuff> with bitmaps' '
	test_config pack.usebitmaps true &&
	# do something to ensure that we have bitmaps
	git ... >actual &&
	test_cmp expect actual
'

or some such.

For bonus points, you could also add some light performance tests in
t/perf/, just to show off ;-)

-- 
Thomas Rast
trast@{inf,student}.ethz.ch