Re: [PATCH 00/16] Speed up Counting Objects with bitmap data

From: Thomas Rast <trast@inf.ethz.ch>
Date: Jun 25, 2013, 16:05 UTC
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:

Show 26 quoted lines

>  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