[PATCH 09/13] drop objects larger than --blob-limit if specified

---
 builtin-pack-objects.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

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

I still consider this feature to make no sense.


Nicolas

On 4/5/07, Nicolas Pitre [off-list ref] wrote:

I still consider this feature to make no sense.

Well, suppose I'm packing my 55GB of data into 2GB
packfiles.  There seemed to be some agreement that
limiting packfile size was useful.  700MB is another example.

Now,  suppose there is an object whose packing would
result in a packfile larger than the limit.  What should we do?

(1) Refuse to run.  This solution means I can't pack my repository.
(2) Pack the object any way and let the packfile size exceed
     my specification.  Ignoring a clear preference from the user
     doesn't seem good.
(3) Pack the object by itself in its own pack. This is better than the
     previous since I haven't wrapped up any small object in a pack
     whose size I dont't want to deal with.  The resulting pack is too big,
     but the original object was also too big so at least I haven't made
     the problem worse.  But why bother wrapping the object so?
     I just made the list of packs to look through longer for every access,
     instead of leaving the big object in the objects/xx directories which
     are already used to handle exceptions (usually meaning more recent).
     In my 55GB example, I have 9 jumbo objects, and this solution
     would more than double the number of packs to step through.
     Having them randomly placed in 256 subdirectories seems better.
(4) Just leave the jumbo object by itself, unpacked.

What do you think?

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

On Thu, 5 Apr 2007, Dana How wrote:

On 4/5/07, Nicolas Pitre [off-list ref] wrote:

quoted

I still consider this feature to make no sense.

Well, suppose I'm packing my 55GB of data into 2GB
packfiles.  There seemed to be some agreement that
limiting packfile size was useful.  700MB is another example.

Now,  suppose there is an object whose packing would
result in a packfile larger than the limit.  What should we do?

You error out.

(1) Refuse to run.  This solution means I can't pack my repository.

Exactly.  If you want packs not to be larger than 10MB and you have a 
100MB blob then you are screwed.  Just lift your pack size limit in such 
case.

(2) Pack the object any way and let the packfile size exceed
    my specification.  Ignoring a clear preference from the user
    doesn't seem good.

It is not indeed.

(3) Pack the object by itself in its own pack. This is better than the
    previous since I haven't wrapped up any small object in a pack
    whose size I dont't want to deal with.  The resulting pack is too big,
    but the original object was also too big so at least I haven't made
    the problem worse.  But why bother wrapping the object so?
    I just made the list of packs to look through longer for every access,
    instead of leaving the big object in the objects/xx directories which
    are already used to handle exceptions (usually meaning more recent).
    In my 55GB example, I have 9 jumbo objects, and this solution
    would more than double the number of packs to step through.
    Having them randomly placed in 256 subdirectories seems better.

You forget about the case where those jumbo blobs could delta well 
against each other.  That means that one pack could possibly contain 
those 9 objects because 8 of them are tiny deltas against the first big 
one.

(4) Just leave the jumbo object by itself, unpacked.

Hmmmmm.

What do you think?

Let's say I wouldn't mind much if it was implemented differently.  The 
objects array is probably the biggest cost in terms of memory usage for 
pack-objects.  When you have 4 milions objects like in the kde repo that 
means each new field you add will cost between 4 to 16 MB of memory.  I 
think this is too big a cost for filtering out a couple big objects once 
in a while.

Instead, I think you should apply the filtering in add_object_entry() 
directly and simply skip adding the unwanted object to the list 
altogether.


Nicolas

On Thu, 5 Apr 2007, Nicolas Pitre wrote:

quoted

(2) Pack the object any way and let the packfile size exceed
    my specification.  Ignoring a clear preference from the user
    doesn't seem good.

It is not indeed.

Well, I think there is an easy solution.

Just go back and say that when the user limits the size, it limits the 
offset at which objects can *start*.

Not only is that the only thing that the index file itself cares about, it 
also means that

 - you don't ever need to undo anything at all (because you know the 
   starting offset) when you begin packing a new object.

   This should simplify your patch a lot.

 - the object size issue just goes away. Sure, the pack-file limit looks a 
   bit strange (it's no longer a hard limit on the *size* of the 
   pack-file, just on the object offsets), but let's face it, we really 
   really don't care.

And in practice, by setting the pack-file limit to 2GB (or even 1GB), you 
never ever have to worry about the 32-bit filesystem limits any more, 
unless your repository is fundamentally so screwed that you simply 
*cannot* reporesent it well on something like FATFS (ie any object that is 
2GB in size will probably have a blob that is even bigger, and FATFS 
already has problems with it).

So in practice, just limiting the index offsets is what you really want 
anyway.

		Linus

On Fri, 6 Apr 2007, Dana How wrote:

* To avoid offsets larger than 31 bits in .idx files.  Your proposal,
 and what I was doing for --pack-limit && --stdout, is sufficient to
address this.

BTW I have code for an index version 2 that allows for 64-bit offsets.  
It also adds a CRC32 for each object data as stored in the pack to make 
data reuse safer when repacking.  I only have to fix a couple loose ends 
before sending patches out.

The new index only uses 64-bit offsets when it has to.  Meaning that 
objects in front of a pack which usually are the most recent ones won't 
pay the overhead of a larger offset table at run time.


Nicolas

On 4/6/07, Linus Torvalds [off-list ref] wrote:

On Thu, 5 Apr 2007, Nicolas Pitre wrote:

quoted

quoted

(2) Pack the object any way and let the packfile size exceed
    my specification.  Ignoring a clear preference from the user
    doesn't seem good.

It is not indeed.

Well, I think there is an easy solution.

Just go back and say that when the user limits the size, it limits the
offset at which objects can *start*.

Yes,  this is what my original, unsplit patch did in order to support
the --pack-limit && --stdout combination, since lseek doesn't
always work on stdout.

Not only is that the only thing that the index file itself cares about, it
also means that

 - you don't ever need to undo anything at all (because you know the
   starting offset) when you begin packing a new object.

Yes, that was true.

   This should simplify your patch a lot.

It removes the calls to, and the additions of, sha1mark() and sha1undo()
in csum-file.[ch].  The changes to builtin-pack-objects.c are almost
the same -- an "if" moves from after the write_object() to before.

 - the object size issue just goes away. Sure, the pack-file limit looks a
   bit strange (it's no longer a hard limit on the *size* of the
   pack-file, just on the object offsets), but let's face it, we really
   really don't care.

And in practice, by setting the pack-file limit to 2GB (or even 1GB), you
never ever have to worry about the 32-bit filesystem limits any more,
unless your repository is fundamentally so screwed that you simply
*cannot* reporesent it well on something like FATFS (ie any object that is
2GB in size will probably have a blob that is even bigger, and FATFS
already has problems with it).

So in practice, just limiting the index offsets is what you really want
anyway.

I agree with your arguments, but packs have different uses
and to me there are several differing (non-)reasons for --pack-limit.
* To split packs into smaller chunks when the .pack format is used as a
  communications protocol. But the discussion has converged to
  "we're not going to split packs at the transmitter". I agree with this
  conclusion,  since it would make the total transmission larger (loss
of deltas).
  Since no .idx file is sent there is no requirement for --pack-limit here.
* To avoid offsets larger than 31 bits in .idx files.  Your proposal,
  and what I was doing for --pack-limit && --stdout, is sufficient to
address this.
* Avoiding (e.g.) 2GB+ files when none already exist in the repository --
  either the filesystem doesn't support anything beyond the limit,
  or we don't want to use a >31b off_t with mmap.  (Perhaps
  the latter case is completely avoided by some glibc 64b trickery,
  but is that always true?)  Only the write rollback approach can address this.
* Creating .pack files that fit on e.g. CDROM etc.
The 2nd and 3rd cases are what I'm thinking about,
which is why the first version of my patch did both.

Anyway, now that I understand Nicolas's issues with the patchset,
and realize I agree with his concerns, I'm going to let this percolate
a little bit until I've got the least number of added behaviors and
command line options.  At the moment I'm leaning towards killing my
--blob-limit idea, keeping --pack-limit based on rollback but with the
additional twist that the first object written to a packfile is never
"rolled back" [meaning (a) everything is packed to make Nicolas happy,
(b) any illegally-sized pack has only one object and could be removed
to make me happy, and (c) my patchset has one less bug], and
adding --index-limit which is what normal people might use on a large
repository [and could be made the default later].  So I think what you
discuss is the most common use for limiting.

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

On Fri, 6 Apr 2007, Dana How wrote:

I agree with your arguments, but packs have different uses
and to me there are several differing (non-)reasons for --pack-limit.
* To split packs into smaller chunks when the .pack format is used as a
 communications protocol. But the discussion has converged to
 "we're not going to split packs at the transmitter". I agree with this
 conclusion,  since it would make the total transmission larger (loss
 of deltas).

I don't agree with it, since I don't think the delta advantage is 
noticeable once packs are "big enough". 

Splitting on the sending side means that it needs to split just in one 
place, rather than have the same logic in two different places.

I don't think it really "convered" on anything, I think the discussion 
just didn't continue along that thing.

 Since no .idx file is sent there is no requirement for --pack-limit here.

I really don't think the idx file is the only - or even primary - reason. 
We need *some* size limiter for a lot of reasons. If the idx file was the 
only reason, we should just switch to a new index format and forget about 
it.

But since there are *other* reasons that cannot go away, that doesn't 
obviate the need for size limits, so while I think the idx one is the 
*first* reason to do this, I don't think it is really the most important 
one, exactly because the idx file we *could* solve other ways.

* To avoid offsets larger than 31 bits in .idx files.  Your proposal,
 and what I was doing for --pack-limit && --stdout, is sufficient to
 address this.

Right. 

* Avoiding (e.g.) 2GB+ files when none already exist in the repository --
 either the filesystem doesn't support anything beyond the limit,
 or we don't want to use a >31b off_t with mmap.  (Perhaps
 the latter case is completely avoided by some glibc 64b trickery,
 but is that always true?)  Only the write rollback approach can address this.

I disagree violently. 

IN THEORY only write rollback can address that. But "theory" is not 
practice, and anybody who thinks that theory is even *relevant* here is 
missing the big picture.

If you use FATFS as the place to store your objects, it's really easy to 
just say: you can't have objects bigger than 2GB. That's the real life 
solution. The "theory" that you could have pack-files larger than 32 bits 
is just about as relevant as quantum mechanics is to designing the tensile 
strength of a sky-scraper. Sure, *in*theory* quantum mechanics matters, 
but in practice, you'd be crazy if you tried to convince anybody that it 
should be done using QM rather than traditional means.

Quite frankly, if you have objects that compress to >2GB, you're going to 
be in so much pain even on *other* filesystems, that I seriously doubt 
you'd want to track them using git anyway. And even if you want to, just 
tell people: don't use FATFS, because this repository is insane.

* Creating .pack files that fit on e.g. CDROM etc.
The 2nd and 3rd cases are what I'm thinking about,
which is why the first version of my patch did both.

Again, *in*practice*, for any sane situation, if you want to fit things on 
a CD-ROM, just give a limit of 600MB, and I can pretty much guarantee that 
you'll see a slop of just a percent or two for any realistic setup. And if 
it goes up to 660MB, you'll still fit on any CD.

So please ignore theory, when theory isn't relevant. Designing for 
something that practically cannot happen sounds pretty wrong.

(Of course, since you have a 55GB archive, you probably have an insane 
thing in the first place. My condoleances if so ;)

		Linus

On Fri, 6 Apr 2007, Dana How wrote:

* Avoiding (e.g.) 2GB+ files when none already exist in the repository --
 either the filesystem doesn't support anything beyond the limit,
 or we don't want to use a >31b off_t with mmap.  (Perhaps
 the latter case is completely avoided by some glibc 64b trickery,
 but is that always true?)  Only the write rollback approach can address this.

If the filesystem doesn't support anything beyond the limit, you cannot 
perform any rollback either.  Because if you roll back that's because 
you crossed the limit already.  But you're not supposed to even be able 
to cross that limit in the first place if the filesystem is limited.


Nicolas