threads / patch / 43298

patchchange the unpack limit treshold to a saner value

Subject: [PATCH take 2] change the unpack limit treshold to a saner value

## tl;dr

8 messages between Dec 6, 2006 and Dec 7, 2006. Diffs are folded; open one to read it.

replies: 7people: 4as markdown or json

Nicolas Pitre· Dec 6, 2006, 21:08 UTC · lore

[PATCH] change the unpack limit treshold to a saner value

Let's assume the average object size is x. Given n objects, the needed storage size is n*(x + b), where b is the average wasted block size on disk.

If those objects are packed, the needed storage becomes n*x + b so we save on the block overhead. But there is the pack index which is 1024 + n*24 + b.

Trying to find the value of n where packed objects become an advantage, we have:

	n*x + n*b > n*x + n*24 + 2*b + 1024
	n*b - 2*b > n*24 + 1024
	(n - 2)*b > n*24 + 1024

So given this we need at least 3 objects for the whole to use more space than a pack, and only if b is greater than 1096. Since a common block size is 4096 then the value of b is likely to converge towards 2048. 3 objects is also where more directory entries are used over an constant of 2 for a pack (assuming that both objects would end up with the same first 2 bytes of hash which is overly optimistic). So 3 should be the optimal number of objects for not exploding a pack. And of course larger packs are likely to take even less space due to delta compression kicking in.

This is why I think the current default treshold should be 3 instead of the insane value of 5000. But since it feels a bit odd to go from 5000 to 3 I setled on 10.

Signed-off-by: Nicolas Pitre <nico@cam.org>
---
Sidenote: I think it is OK for pushes to _not_ use thin packs.  When 
not exploding thin packs, they must be completed by adding objects 
creating duplicates and using more disk space.  The penalty for not 
using thin packs is a slight increase in bandwidth for push operations, 
but since pushes are normally much less frequent than fetches it seems 
OK to penalize the push a bit for a better disk usage on servers.
Show changes to receive-pack.c +1 −1
diff --git a/receive-pack.c b/receive-pack.c
index d62ed5b..9140312 100644
--- a/receive-pack.c
+++ b/receive-pack.c
@@ -11,7 +11,7 @@
 static const char receive_pack_usage[] = "git-receive-pack <git-dir>";
 
 static int deny_non_fast_forwards = 0;
-static int unpack_limit = 5000;
+static int unpack_limit = 10;
 static int report_status;
Junio C Hamano· Dec 6, 2006, 22:24 UTC · re: Nicolas Pitre · lore

Re: [PATCH] change the unpack limit threshold to a saner value

Nicolas Pitre <nico@cam.org> writes:
Show 7 quoted lines
> Let's assume the average object size is x. Given n objects, the needed 
> storage size is n*(x + b), where b is the average wasted block size on 
> disk.
> ...
> This is why I think the current default treshold should be 3 instead of 
> the insane value of 5000.  But since it feels a bit odd to go from 5000 
> to 3 I setled on 10.

I see you are optimizing for disk footprint, and this will result in tons of tiny packs left between "repack -a".

I have not benched it yet, but the runtime pack handling code was written assuming we have only a handful of big packs; I suspect this change would affect the performance at runtime in quite a bad way.

Nicolas Pitre· Dec 7, 2006, 00:19 UTC · re: Junio C Hamano · lore

Re: [PATCH] change the unpack limit threshold to a saner value

On Wed, 6 Dec 2006, Junio C Hamano wrote:
Show 12 quoted lines
> Nicolas Pitre <nico@cam.org> writes:
> 
> > Let's assume the average object size is x. Given n objects, the needed 
> > storage size is n*(x + b), where b is the average wasted block size on 
> > disk.
> > ...
> > This is why I think the current default treshold should be 3 instead of 
> > the insane value of 5000.  But since it feels a bit odd to go from 5000 
> > to 3 I setled on 10.
> 
> I see you are optimizing for disk footprint, and this will
> result in tons of tiny packs left between "repack -a".
Depends how often i.e. how big pushes are, and how often you repack.
> I have not benched it yet, but the runtime pack handling code
> was written assuming we have only a handful of big packs; I
> suspect this change would affect the performance at runtime in
> quite a bad way.
Possibly.
Still a treshold of 5000 is way too large IMHO.
Linus Torvalds· Dec 7, 2006, 01:08 UTC · re: Nicolas Pitre · lore

Re: [PATCH] change the unpack limit treshold to a saner value

On Wed, 6 Dec 2006, Nicolas Pitre wrote:
> 
> This is why I think the current default treshold should be 3 instead of 
> the insane value of 5000.  But since it feels a bit odd to go from 5000 
> to 3 I setled on 10.
Definitely not.

We have a much easier time handling many loose packed objects than many pack-files. For many reasons, but two really obvious ones:

 - pack-file indexes get read in on startup, and we maintain an explicit 
   list of them. Having lots of pack-files adds overhead that doesn't 
   exist for lots of loose objects.
 - loose files are spread out over 256 subdirectories to make lookup 
   easier, packfiles are not (and always create an index file too).

So in general, as a trivial heuristic, you probably want about 512 times as many loose objects as you want pack-files, i fonly because of the latter issue, because you can much more easily handle lots of loose objects than lots of pack-files. So it's _not_ a factor of 3. Or even 10.

But since there _is_ reason to do pack-files too, and since using too big a value means that you never end up keeping a pack-file _at_all_ if you pull often, I'd suggest that rather than use a limit of 512 you go for something like 100-200 objects as the threshold (of course, the proper one would depend on the distribution of the size of your pack-files, but I'll just hand-wave and say that together with occasional re-packing, something in that range is _generally_ going to be a good idea).

Nicolas Pitre· Dec 7, 2006, 03:24 UTC · re: Linus Torvalds · lore

Re: [PATCH] change the unpack limit treshold to a saner value

On Wed, 6 Dec 2006, Linus Torvalds wrote:
Show 22 quoted lines
> We have a much easier time handling many loose packed objects than many 
> pack-files. For many reasons, but two really obvious ones:
> 
>  - pack-file indexes get read in on startup, and we maintain an explicit 
>    list of them. Having lots of pack-files adds overhead that doesn't 
>    exist for lots of loose objects.
> 
>  - loose files are spread out over 256 subdirectories to make lookup 
>    easier, packfiles are not (and always create an index file too).
> 
> So in general, as a trivial heuristic, you probably want about 512 times 
> as many loose objects as you want pack-files, i fonly because of the 
> latter issue, because you can much more easily handle lots of loose 
> objects than lots of pack-files. So it's _not_ a factor of 3. Or even 10.
> 
> But since there _is_ reason to do pack-files too, and since using too big 
> a value means that you never end up keeping a pack-file _at_all_ if you 
> pull often, I'd suggest that rather than use a limit of 512 you go for 
> something like 100-200 objects as the threshold (of course, the proper one 
> would depend on the distribution of the size of your pack-files, but I'll 
> just hand-wave and say that together with occasional re-packing, something 
> in that range is _generally_ going to be a good idea).

Note that this setting is currently observed for pushes not pulls. On the pull side you currentli need to provide -k for not exploding packs.

So the question is what number of objects on average do pushes have? If most pushes are below the treshold this is not going to be really useful.

And I think 5000 is definitely way too high. 10 might be too small indeed. 100 is maybe a good default to try out.

Linus Torvalds· Dec 7, 2006, 03:39 UTC · re: Nicolas Pitre · lore

Re: [PATCH] change the unpack limit treshold to a saner value

On Wed, 6 Dec 2006, Nicolas Pitre wrote:
> 
> Note that this setting is currently observed for pushes not pulls.
> On the pull side you currentli need to provide -k for not exploding 
> packs.
So noted.
> So the question is what number of objects on average do pushes have?  If 
> most pushes are below the treshold this is not going to be really 
> useful.
It will depend a lot on the project, and "where" in the project you are.

For example, for most end developers, the "push" is likely going to be a few commits (say, a days work). Probably on the order of a few tens to maybe a few hundred objects. It's actually hard to create a pack with less than ten objects if you have a few directory levels (a single small commit in the kernel is usually 5-7 objects: commit + 2-3 levels of directory + a couple of blobs).

For me, as I pull a big merge and push it out, a push can easily be in the thousands of objects, just because I merged other peoples combined work over several weeks.

And for a "mirror" server, it will depend on the granularity of the mirroring.

> And I think 5000 is definitely way too high.  10 might be too small 
> indeed.  100 is maybe a good default to try out.
I think 100 is a nice round number for humans. Worth trying.
Nicolas Pitre· Dec 7, 2006, 04:01 UTC · re: Nicolas Pitre · lore

Currently the treshold is 5000. The likelihood of this value to ever be crossed for a single push is really small making it not really useful.

The optimal treshold for a pure space saving on a filesystem with 4kb blocks is 3. However this is likely to create many small packs concentrating a large number of files in a single directory compared to the same objects which are spread over 256 directories when loose. This means we would need 512 objects per pack on average to approximagte the same directory cost (a pack has 2 files because of the index).

But 512 is a really high value just like 5000 since most pushes are unlikely to have that many objects. So let's try with a value of 100 which should have a good balance between small pushes going to be exploded into loose objects and large pushes kept as whole packs.

This is not a replacement for periodic repacks of course.
Signed-off-by: Nicolas Pitre <nico@cam.org>
---
Show changes to receive-pack.c +1 −1
diff --git a/receive-pack.c b/receive-pack.c
index d62ed5b..9140312 100644
--- a/receive-pack.c
+++ b/receive-pack.c
@@ -11,7 +11,7 @@
 static const char receive_pack_usage[] = "git-receive-pack <git-dir>";
 
 static int deny_non_fast_forwards = 0;
-static int unpack_limit = 5000;
+static int unpack_limit = 100;
 static int report_status;
Shawn Pearce· Dec 7, 2006, 07:59 UTC · re: Nicolas Pitre · lore

Re: [PATCH take 2] change the unpack limit treshold to a saner value

Nicolas Pitre <nico@cam.org> wrote:
> Currently the treshold is 5000.  The likelihood of this value to ever be 
> crossed for a single push is really small making it not really useful.

I started that out at 5000 because it was a reasonably safe threshold. Users who didn't explicitly lower this value didn't enable the feature. But it was still useful on initial pushes into a brand new repository when the project was very large. For example pushing git.git into a bare repository *sucked* before this change.

Yea, 5000 is probably too high.  Nice to see it drop.

← back to recent threads