[RFD/PATCH] Implement pack.compression and pack-objects --compression=N

[RFD/PATCH] Implement pack.compression and pack-objects --compression=N

[Dana How]

Git's object store and packing are optimized for
* Expensive repo to repo bandwidth; and
* Small-ish files
This justifies extensive use of compression.

In a multi-developer *office* with inter-repository
transfers occurring over a 100Mb+ LAN,  there is less
reason to compress files and slow down response times.
Response times suffer even more when large files are involved.
However,  *off-line* pack compression may still be
desirable to reduce storage space.

Consequently,  for such a usage pattern it is useful
to specify different compression levels for loose
objects and packs.  This patch implements a config
variable pack.compression in addition to the existing
core.compression,  meant to be used for repacking.
It also adds --compression=N to pack-objects,
meant for push/pull/fetch,  if different,  or if different
on a per-repository basis.

** THIS PATCH IS UNTESTED AND MEANT FOR DISCUSSION. **
git-repack.sh might also need to be modified,
and how to pass --compression=N during push/pull/fetch
has not been investigated.

This applies on top of the git-repack --max-pack-size patchset.

Signed-off-by: Dana L. How <danahow@gmail.com>
---
 builtin-pack-objects.c |   56 ++++++++++++++++++++++++++++++++++++------------
 1 files changed, 42 insertions(+), 14 deletions(-)

diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 69fec34..b663c15 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -70,6 +70,7 @@ static uint32_t pack_size_limit;
 static int pack_to_stdout;
 static int num_preferred_base;
 static struct progress progress_state;
+static int pack_compression_level, pack_compression_seen;
 
 /*
  * The object names in objects array are hashed with this hashtable,
@@ -414,6 +415,16 @@ static unsigned long write_object(struct sha1file *f,
 	/* write limit if limited packsize and not first object */
 	unsigned long limit = pack_size_limit && nr_written ?
 				pack_size_limit - write_offset : 0;
+				/* no if no delta */
+	int usable_delta =	!entry->delta ? 0 :
+				/* yes if unlimited packfile */
+				!pack_size_limit ? 1 :
+				/* no if base written to previous pack */
+				entry->delta->offset == (off_t)-1 ? 0 :
+				/* otherwise double-check written to this
+				 * pack,  like we do below
+				 */
+				entry->delta->offset ? 1 : 0;
 
 	if (!pack_to_stdout)
 		crc32_begin(f);
@@ -423,8 +434,7 @@ static unsigned long write_object(struct sha1file *f,
 		to_reuse = 0;	/* can't reuse what we don't have */
 	else if (obj_type == OBJ_REF_DELTA || obj_type == OBJ_OFS_DELTA)
 				/* check_object() decided it for us ... */
-		to_reuse = !pack_size_limit ||
-			(entry->delta->offset && entry->delta->offset != (off_t)-1);
+		to_reuse = usable_delta;
 				/* ... but pack split may override that */
 	else if (obj_type != entry->in_pack_type)
 		to_reuse = 0;	/* pack has delta which is unusable */
@@ -435,6 +445,10 @@ static unsigned long write_object(struct sha1file *f,
 				 * and we do not need to deltify it.
 				 */
 
+	/* differing core & pack compression when loose object -> must recompress */
+	if (!entry->in_pack && pack_compression_level != zlib_compression_level)
+		to_reuse = 0;
+	else
 	if (!entry->in_pack && !entry->delta) {
 		unsigned char *map;
 		unsigned long mapsize;
@@ -462,16 +476,6 @@ static unsigned long write_object(struct sha1file *f,
 		z_stream stream;
 		unsigned long maxsize;
 		void *out;
-					/* no if no delta */
-		int usable_delta =	!entry->delta ? 0 :
-					/* yes if unlimited packfile */
-					!pack_size_limit ? 1 :
-					/* no if base written to previous pack */
-					entry->delta->offset == (off_t)-1 ? 0 :
-					/* otherwise double-check written to this
-					 * pack,  like we do below
-					 */
-					entry->delta->offset ? 1 : 0;
 		buf = read_sha1_file(entry->sha1, &type, &size);
 		if (!buf)
 			die("unable to read %s", sha1_to_hex(entry->sha1));
@@ -493,7 +497,7 @@ static unsigned long write_object(struct sha1file *f,
 		}
 		/* compress the data to store and put compressed length in datalen */
 		memset(&stream, 0, sizeof(stream));
-		deflateInit(&stream, zlib_compression_level);
+		deflateInit(&stream, pack_compression_level);
 		maxsize = deflateBound(&stream, size);
 		out = xmalloc(maxsize);
 		/* Compress it */
@@ -606,7 +610,7 @@ static unsigned long write_object(struct sha1file *f,
 		unuse_pack(&w_curs);
 		reused++;
 	}
-	if (entry->delta)
+	if (usable_delta)
 		written_delta++;
 	written++;
 	if (!pack_to_stdout)
@@ -1622,6 +1626,16 @@ static int git_pack_config(const char *k, const char *v)
 		window = git_config_int(k, v);
 		return 0;
 	}
+	if (!strcmp(k, "pack.compression")) {
+		int level = git_config_int(k, v);
+		if (level == -1)
+			level = Z_DEFAULT_COMPRESSION;
+		else if (level < 0 || level > Z_BEST_COMPRESSION)
+			die("bad pack compression level %d", level);
+		pack_compression_level = level;
+		pack_compression_seen = 1;
+		return 0;
+	}
 	return git_default_config(k, v);
 }
 
@@ -1732,6 +1746,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 	rp_ac = 2;
 
 	git_config(git_pack_config);
+	if (!pack_compression_seen)
+		pack_compression_level = zlib_compression_level;
 
 	progress = isatty(2);
 	for (i = 1; i < argc; i++) {
@@ -1759,6 +1775,18 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 				usage(pack_usage);
 			continue;
 		}
+		if (!prefixcmp(arg, "--compression=")) {
+			char *end;
+			int level = strtoul(arg+14, &end, 0);
+			if (!arg[14] || *end)
+				usage(pack_usage);
+			if (level == -1)
+				level = Z_DEFAULT_COMPRESSION;
+			else if (level < 0 || level > Z_BEST_COMPRESSION)
+				die("bad pack compression level %d", level);
+			pack_compression_level = level;
+			continue;
+		}
 		if (!prefixcmp(arg, "--window=")) {
 			char *end;
 			window = strtoul(arg+9, &end, 0);
-- 
1.5.2.rc0.787.g0014

Re: [RFD/PATCH] Implement pack.compression and pack-objects --compression=N

[Junio C Hamano]

Dana How <danahow@gmail.com> writes:

> Consequently,  for such a usage pattern it is useful
> to specify different compression levels for loose
> objects and packs.  This patch implements a config
> variable pack.compression in addition to the existing
> core.compression,  meant to be used for repacking.
> It also adds --compression=N to pack-objects,
> meant for push/pull/fetch,  if different,  or if different
> on a per-repository basis.
>
> ** THIS PATCH IS UNTESTED AND MEANT FOR DISCUSSION. **

I think we tweaked this area in the past, but I do not think
the current setting was determined to be the best tradeoff for
all workloads.  To be able to discuss the patch, I think it
needs to come with benchmark numbers using publicly available
repositories as guinea pigs and set of typical git operations,
so people can reproduce and compare notes.

Re: [RFD/PATCH] Implement pack.compression and pack-objects --compression=N

[Dana How]

On 5/2/07, Junio C Hamano <junkio@cox.net> wrote:
> Dana How <danahow@gmail.com> writes:
> > Consequently,  for such a usage pattern it is useful
> > to specify different compression levels for loose
> > objects and packs.  This patch implements a config
> > variable pack.compression in addition to the existing
> > core.compression,  meant to be used for repacking.
> > It also adds --compression=N to pack-objects,
> > meant for push/pull/fetch,  if different,  or if different
> > on a per-repository basis.
> >
> > ** THIS PATCH IS UNTESTED AND MEANT FOR DISCUSSION. **
>
> I think we tweaked this area in the past, but I do not think
> the current setting was determined to be the best tradeoff for
> all workloads.  To be able to discuss the patch, I think it
> needs to come with benchmark numbers using publicly available
> repositories as guinea pigs and set of typical git operations,
> so people can reproduce and compare notes.

OK, but this patch doesn't mandate any particular setting.

Its motivation in my work environment is for pack.compression
to be what core.compression currently is,  and to set
core.compression to 0 to speed up large commits
(the resulting space-inefficient loose objects will be scrubbed away
 by a later off-line repack).
Thus,  my config settings (almost) change the gzip's behind a git-add to cp's.
Do you want me to submit timings for a git-add/git-commit -a
on a typical 50-file commit I would be interested in,
with the (new) settings that I would use?

Thanks,
-- 
Dana L. How  danahow@gmail.com  +1 650 804 5991 cell

Re: [RFD/PATCH] Implement pack.compression and pack-objects --compression=N

[Dana How]

On 5/2/07, Dana How <danahow@gmail.com> wrote:
> On 5/2/07, Junio C Hamano <junkio@cox.net> wrote:
> > Dana How <danahow@gmail.com> writes:
> > > Consequently,  for such a usage pattern it is useful
> > > to specify different compression levels for loose
> > > objects and packs.  This patch implements a config
> > > variable pack.compression in addition to the existing
> > > core.compression,  meant to be used for repacking.
> > > It also adds --compression=N to pack-objects,
> > > meant for push/pull/fetch,  if different,  or if different
> > > on a per-repository basis.
> > >
> > > ** THIS PATCH IS UNTESTED AND MEANT FOR DISCUSSION. **
> >
> > I think we tweaked this area in the past, but I do not think
> > the current setting was determined to be the best tradeoff for
> > all workloads.  To be able to discuss the patch, I think it
> > needs to come with benchmark numbers using publicly available
> > repositories as guinea pigs and set of typical git operations,
> > so people can reproduce and compare notes.
>
> OK, but this patch doesn't mandate any particular setting.
>
> Its motivation in my work environment is for pack.compression
> to be what core.compression currently is,  and to set
> core.compression to 0 to speed up large commits
> (the resulting space-inefficient loose objects will be scrubbed away
>  by a later off-line repack).
> Thus,  my config settings (almost) change the gzip's behind a git-add to cp's.
> Do you want me to submit timings for a git-add/git-commit -a
> on a typical 50-file commit I would be interested in,
> with the (new) settings that I would use?

Note the linux-2.6 git tree from a week ago has 22K checked-out files
of average size 11KB; the largest is fs/nls/nls-cp949.c at 874KB.
(The largest file in git is gitk at 176K.)

The tree I'm interested in maintaining with git is almost 70GB
checked-out in 13K files of average size >5.2MB.  This is over
2 orders of magnitude larger average file size than current git users.
(Some of these numbers may decrease after a little retraining ;-).)
I would like git to perform as responsively as possible on files
up to ~500MB.  Within this tree,  the largest file is 1234MB
[I think checking this in was a mistake!]
and I did the following experiments on it:

$ rm -rf .git
$ git-init
Initialized empty Git repository in .git/
$ git-config core.compression -1
$ wc large.spef
  12762072   37832482 1234082774 large.spef
$ /usr/bin/time git-add large.spef
41.54user 0.70system 0:49.76elapsed 84%CPU (0avgtext+0avgdata 0maxresident)k
$ ls -lR .git/objects/??
.git/objects/d5:
total 83836
-r--r--r--  1 how group 85670068 May  2 15:11
d6cde2af063cdfa835038385f29a897bf9533b

$ rm -rf .git
$ git-init
Initialized empty Git repository in .git/
$ git-config core.compression 1
$ wc large.spef
  12762072   37832482 1234082774 large.spef
$ /usr/bin/time git-add large.spef
23.66user 0.74system 0:34.07elapsed 71%CPU (0avgtext+0avgdata 0maxresident)k
$ ls -lR .git/objects/??
.git/objects/d5:
total 105116
-r--r--r--  1 how group 107419557 May  2 15:13
d6cde2af063cdfa835038385f29a897bf9533b

So for a 25% increase in blob size I get 33% less elapsed time
in git-add, all by changing core.compression from -1 to 1.
I'll definitely take that improvement.  [For the compressible files
we typically have, using 0 is a bad idea:  the CPU "advantage"
is swamped out by the time to write a much larger file.]

Since I don't care [to the same degree] about the responsiveness of
packing,  I'd rather pack with -1 or better to keep packs small.
(And inflation time seems independent of compression setting.)
Since someone might be working while the packing is happening,
I'd rather not change the config setting to achieve this.
Hence the patch.

Concerning various public repositories, clearly the patch has no
impact if you don't specify different core.compression and pack.compression
values.  If you do specify different values,  I doubt there would be much
noticeable speed-up for e.g. the linux-2.6 repo stats I included above.
There would be some,  but that wasn't the motivation for the patch.

Thanks,
-- 
Dana L. How  danahow@gmail.com  +1 650 804 5991 cell

Re: [RFD/PATCH] Implement pack.compression and pack-objects --compression=N

[Junio C Hamano]

"Dana How" <danahow@gmail.com> writes:

> So for a 25% increase in blob size I get 33% less elapsed time
> in git-add, all by changing core.compression from -1 to 1.
> I'll definitely take that improvement.  [For the compressible files
> we typically have, using 0 is a bad idea:  the CPU "advantage"
> is swamped out by the time to write a much larger file.]

The above number is about loose objects, right?

> Since I don't care [to the same degree] about the responsiveness of
> packing,  I'd rather pack with -1 or better to keep packs small.

I see.  You are saying that the fact that core.compression is
used also for packing makes the variable less useful.

I agree that it would make sense to have at least the pack and
core compression independent.  I am not sure if we would also
want to make the pack compression tweakable depending on the
purpose of the packing (network transfer vs .git/objects/pack/).

> (And inflation time seems independent of compression setting.)

True.

Re: [RFD/PATCH] Implement pack.compression and pack-objects --compression=N

[Dana How]

On 5/3/07, Junio C Hamano <junkio@cox.net> wrote:
> "Dana How" <danahow@gmail.com> writes:
> > So for a 25% increase in blob size I get 33% less elapsed time
> > in git-add, all by changing core.compression from -1 to 1.
> > I'll definitely take that improvement.  [For the compressible files
> > we typically have, using 0 is a bad idea:  the CPU "advantage"
> > is swamped out by the time to write a much larger file.]
> The above number is about loose objects, right?
Correct.

> > Since I don't care [to the same degree] about the responsiveness of
> > packing,  I'd rather pack with -1 or better to keep packs small.
> I see.  You are saying that the fact that core.compression is
> used also for packing makes the variable less useful.
Exactly.

> I agree that it would make sense to have at least the pack and
> core compression independent.  I am not sure if we would also
> want to make the pack compression tweakable depending on the
> purpose of the packing (network transfer vs .git/objects/pack/).
The final 12-line hunk in the patch implements --pack-compression=N.
To be blunt,  I'm not sure *I* need it.  Perhaps a high-use web-accessible
public repository would be interested in repacking off-line with 9,  and
creating packs for each puller using -1 to reduce CPU load.  In the latter
case,  due to builtin-pack-objects.c:write_object():to_reuse==1,
much of the transferred data would still be at the better compression
but without again paying the CPU cost.  Shall I remove it?

BTW, I'm now in the habit of browsing Git's Gitweb at git.or.cz and notice
"pu" has --max-pack-size.  You may want to upgrade to the last 5-patch
version in which --max-pack-size no longer forces --no-reuse-delta at
Shawn's request,  among other clean-ups.

Thanks,
-- 
Dana L. How  danahow@gmail.com  +1 650 804 5991 cell

Re: [RFD/PATCH] Implement pack.compression and pack-objects --compression=N

[Nicolas Pitre]

On Thu, 3 May 2007, Junio C Hamano wrote:

> "Dana How" <danahow@gmail.com> writes:
> 
> > So for a 25% increase in blob size I get 33% less elapsed time
> > in git-add, all by changing core.compression from -1 to 1.
> > I'll definitely take that improvement.  [For the compressible files
> > we typically have, using 0 is a bad idea:  the CPU "advantage"
> > is swamped out by the time to write a much larger file.]
> 
> The above number is about loose objects, right?
> 
> > Since I don't care [to the same degree] about the responsiveness of
> > packing,  I'd rather pack with -1 or better to keep packs small.
> 
> I see.  You are saying that the fact that core.compression is
> used also for packing makes the variable less useful.

I think that would make sense to have separate configs for pack and 
loose object compression.  When not specified they should simply default 
to core.compression if it exists.  Otherwise I'd suggest that pack 
compression default level be Z_DEFAULT_COMPRESSION and loose object 
compression default level be Z_BEST_SPEED.  This would make interactive 
operations like git-add and git-commit even faster by default.


Nicolas

Re: [RFD/PATCH] Implement pack.compression and pack-objects --compression=N

[Dana How]

On 5/4/07, Nicolas Pitre <nico@cam.org> wrote:
> On Thu, 3 May 2007, Junio C Hamano wrote:
>
> > "Dana How" <danahow@gmail.com> writes:
> >
> > > So for a 25% increase in blob size I get 33% less elapsed time
> > > in git-add, all by changing core.compression from -1 to 1.
> > > I'll definitely take that improvement.  [For the compressible files
> > > we typically have, using 0 is a bad idea:  the CPU "advantage"
> > > is swamped out by the time to write a much larger file.]
> >
> > The above number is about loose objects, right?
> >
> > > Since I don't care [to the same degree] about the responsiveness of
> > > packing,  I'd rather pack with -1 or better to keep packs small.
> >
> > I see.  You are saying that the fact that core.compression is
> > used also for packing makes the variable less useful.
>
> I think that would make sense to have separate configs for pack and
> loose object compression.  When not specified they should simply default
> to core.compression if it exists.  Otherwise I'd suggest that pack
> compression default level be Z_DEFAULT_COMPRESSION and loose object
> compression default level be Z_BEST_SPEED.  This would make interactive
> operations like git-add and git-commit even faster by default.

I agree with your Z_BEST_SPEED idea.  I did not include it in
the patch b/c I didn't want to change any behavior in the absence
of new config settings.

Are you actually arguing for *3* different compression-related config
variables?  How about:
(a) core.compression controls loose objects. defaults to Z_BEST_SPEED.
(b) pack.compression controls packing. defaults to Z_DEFAULT_COMPRESSION
if neither variable exists. defaults to core.compression if only that exists

The last sentence mimics current behavior and to me seems least
surprising.  Or pack.compression could be simpler: default to
Z_DEFAULT_COMPRESSION if pack.compression doesn't exist
(no interaction with core.compression).

Thanks,
-- 
Dana L. How  danahow@gmail.com  +1 650 804 5991 cell

Re: [RFD/PATCH] Implement pack.compression and pack-objects --compression=N

[Nicolas Pitre]

On Fri, 4 May 2007, Dana How wrote:

> On 5/4/07, Nicolas Pitre <nico@cam.org> wrote:
> > On Thu, 3 May 2007, Junio C Hamano wrote:
> > 
> > > "Dana How" <danahow@gmail.com> writes:
> > >
> > > > So for a 25% increase in blob size I get 33% less elapsed time
> > > > in git-add, all by changing core.compression from -1 to 1.
> > > > I'll definitely take that improvement.  [For the compressible files
> > > > we typically have, using 0 is a bad idea:  the CPU "advantage"
> > > > is swamped out by the time to write a much larger file.]
> > >
> > > The above number is about loose objects, right?
> > >
> > > > Since I don't care [to the same degree] about the responsiveness of
> > > > packing,  I'd rather pack with -1 or better to keep packs small.
> > >
> > > I see.  You are saying that the fact that core.compression is
> > > used also for packing makes the variable less useful.
> > 
> > I think that would make sense to have separate configs for pack and
> > loose object compression.  When not specified they should simply default
> > to core.compression if it exists.  Otherwise I'd suggest that pack
> > compression default level be Z_DEFAULT_COMPRESSION and loose object
> > compression default level be Z_BEST_SPEED.  This would make interactive
> > operations like git-add and git-commit even faster by default.
> 
> I agree with your Z_BEST_SPEED idea.  I did not include it in
> the patch b/c I didn't want to change any behavior in the absence
> of new config settings.
> 
> Are you actually arguing for *3* different compression-related config
> variables?

Yes.

> How about:
> (a) core.compression controls loose objects. defaults to Z_BEST_SPEED.
> (b) pack.compression controls packing. defaults to Z_DEFAULT_COMPRESSION
> if neither variable exists. defaults to core.compression if only that exists

Yes, although I wouldn't default pack.compression to core.compression  
if pack.compression doesn't exist.  The documentation about 
core.compression currently talks 
(wrongly) only about loose objects anyway, so making pack.compression 
stand on its own won't be that bad.


Nicolas

Re: [RFD/PATCH] Implement pack.compression and pack-objects --compression=N

[Dana How]

On 5/4/07, Nicolas Pitre <nico@cam.org> wrote:
> On Fri, 4 May 2007, Dana How wrote:
> > On 5/4/07, Nicolas Pitre <nico@cam.org> wrote:
> > > I think that would make sense to have separate configs for pack and
> > > loose object compression.  When not specified they should simply default
> > > to core.compression if it exists.  Otherwise I'd suggest that pack
> > > compression default level be Z_DEFAULT_COMPRESSION and loose object
> > > compression default level be Z_BEST_SPEED.  This would make interactive
> > > operations like git-add and git-commit even faster by default.
> > I agree with your Z_BEST_SPEED idea.  I did not include it in
> > the patch b/c I didn't want to change any behavior in the absence
> > of new config settings.
> > Are you actually arguing for *3* different compression-related config
> > variables?
> Yes.
>
> > How about:
> > (a) core.compression controls loose objects. defaults to Z_BEST_SPEED.
> > (b) pack.compression controls packing. defaults to Z_DEFAULT_COMPRESSION
> > if neither variable exists. defaults to core.compression if only that exists
> Yes, although I wouldn't default pack.compression to core.compression
> if pack.compression doesn't exist.  The documentation about
> core.compression currently talks
> (wrongly) only about loose objects anyway, so making pack.compression
> stand on its own won't be that bad.

Now that I'm awake your original quote at the top suggests:
(a) zlib_compression_level =
     isset(core.loosecompression) ? core.loosecompression :
     isset(core.compression) ? core.compression : Z_BEST_SPEED;
(b) pack_compression_level =
     isset(pack.compression) ? pack.compression :
     isset(core.compression) ? core.compression : Z_DEFAULT_COMPRESSION;

Your later reaction to my quoted (a)/(b) table suggests:
(a) zlib_compression_level =
     isset(core.compression) ? core.compression : Z_BEST_SPEED;
(b) pack_compresion_level =
     isset(pack.compression) ? pack.compression : Z_DEFAULT_COMPRESSION;

In either case,  the C variable
zlib_compression_level controls compression level for loose objects,
and pack_compression_level controls compression in a pack.
"isset()" means an active setting does appear in a config file
(it could be the default value).

The 2nd behavior table is sufficient for me and simpler than my patch.
Do you want the 1st or 2nd behavior?

Thanks,
-- 
Dana L. How  danahow@gmail.com  +1 650 804 5991 cell

Re: [RFD/PATCH] Implement pack.compression and pack-objects --compression=N

[Nicolas Pitre]

On Fri, 4 May 2007, Dana How wrote:

> Now that I'm awake your original quote at the top suggests:
> (a) zlib_compression_level =
>     isset(core.loosecompression) ? core.loosecompression :
>     isset(core.compression) ? core.compression : Z_BEST_SPEED;
> (b) pack_compression_level =
>     isset(pack.compression) ? pack.compression :
>     isset(core.compression) ? core.compression : Z_DEFAULT_COMPRESSION;
> 
> Your later reaction to my quoted (a)/(b) table suggests:
> (a) zlib_compression_level =
>     isset(core.compression) ? core.compression : Z_BEST_SPEED;
> (b) pack_compresion_level =
>     isset(pack.compression) ? pack.compression : Z_DEFAULT_COMPRESSION;

My final comment is that I think the former looks more coherent, while 
the later is simpler to implement.


Nicolas