The motivations are to better support portable media,
older filesystems, and larger repositories without
awkward enormous packfiles.
When --pack-limit[=N] is specified and --stdout is not,
all bytes in the resulting packfile(s) appear at offsets
less than N (which defaults to 1<<31). The default
guarantees mmap(2) on 32b systems never sees signed off_t's.
The object stream may be broken into multiple packfiles
as a result, each properly and conventionally built.
When --stdout is also specified, all objects in the
resulting packfile(s) _start_ at offsets less than N.
All the packfiles appear concatenated on stdout,
and each has its object count set to 0. The behavior
without --stdout cannot be duplicated here since
lseek(2) is not generally possible on stdout. The end
of each pack in the concatenated whole can be detected by
asking, just before reading each object, if the next
20 bytes match the SHA-1 of what came before.
A future change might insert a null-byte EOF marker
(i.e. type=OBJ_NONE/length=0) before each pack's final SHA-1
or before only the final pack's SHA-1.
When --blob-limit=N is specified, blobs whose uncompressed
size is greater than or equal to N are omitted from the pack(s).
If --pack-limit is specified, --blob-limit is not, and
--stdout is not, then --blob-limit defaults to 1/4
of the --pack-limit.
To enable this, csum-file.c now has the ability to rollback
a checksummed write (see sha1mark/sha1undo).
Signed-off-by: Dana L. How <redacted>
---
builtin-pack-objects.c | 300 ++++++++++++++++++++++++++++++++++++++--------
builtin-unpack-objects.c | 2 +-
csum-file.c | 41 +++++++
csum-file.h | 6 +
git-repack.sh | 12 ++-
http-fetch.c | 2 +-
http-push.c | 2 +-
index-pack.c | 2 +-
sha1_file.c | 2 +-
9 files changed, 307 insertions(+), 62 deletions(-)
[Full patch attached since gmail insists on wordwrap and eating ws]
Thanks,
--
Dana L. How danahow@gmail.com +1 650 804 5991 cell
From: Nicolas Pitre <hidden> Date: 2016-06-15 22:43:03
On Wed, 4 Apr 2007, Dana How wrote:
The motivations are to better support portable media,
older filesystems, and larger repositories without
awkward enormous packfiles.
I wouldn't qualify "enormous" pack files as "awkward".
It will always be more efficient to have only one pack to deal with
(when possible of course).
When --pack-limit[=N] is specified and --stdout is not,
all bytes in the resulting packfile(s) appear at offsets
less than N (which defaults to 1<<31). The default
guarantees mmap(2) on 32b systems never sees signed off_t's.
The object stream may be broken into multiple packfiles
as a result, each properly and conventionally built.
This sounds fine. *However* how do you ensure that the second pack (or
subsequent packs) is self contained with regards to delta base objects
when it is _not_ meant to be a thin pack?
When --stdout is also specified, all objects in the
resulting packfile(s) _start_ at offsets less than N.
All the packfiles appear concatenated on stdout,
and each has its object count set to 0. The behavior
without --stdout cannot be duplicated here since
lseek(2) is not generally possible on stdout.
Please scrap that. There is simply no point making --pack-limit and
--stdout work together. If the amount of data to send over the GIT
protocol exceeds 4G (or whatever) it is the receiving end's business to
split it up _if_ it wants/has to. The alternative is just too ugly.
When --blob-limit=N is specified, blobs whose uncompressed
size is greater than or equal to N are omitted from the pack(s).
If --pack-limit is specified, --blob-limit is not, and
--stdout is not, then --blob-limit defaults to 1/4
of the --pack-limit.
Is this really useful?
If you have a pack size limit and a blob cannot make it even in a pack
of its
own then you're screwed anyway. It is much better to simply fail the
operation than leaving some blobs behind. IOW I don't see the
usefulness of this feature.
Nicolas
The motivations are to better support portable media,
older filesystems, and larger repositories without
awkward enormous packfiles.
I wouldn't qualify "enormous" pack files as "awkward".
It will always be more efficient to have only one pack to deal with
(when possible of course).
Yes. "(when possible of course)" refers to the remaining motivations
I didn't explicitly mention: the 32b offset limit in .idx files,
and keeping the mmap code working on a 32b system.
I realize there are better solutions in the pipeline,
but I'd like to address this now (for my own use) and hopefully
also create something useful for 4GB-limited filesystems,
USB sticks, etc.
quoted
When --pack-limit[=N] is specified and --stdout is not,
all bytes in the resulting packfile(s) appear at offsets
less than N (which defaults to 1<<31). The default
guarantees mmap(2) on 32b systems never sees signed off_t's.
The object stream may be broken into multiple packfiles
as a result, each properly and conventionally built.
This sounds fine. *However* how do you ensure that the second pack (or
subsequent packs) is self contained with regards to delta base objects
when it is _not_ meant to be a thin pack?
Good question. Search for "int usable_delta" in the patch.
With --pack-limit (offset_limit in C), you can use a delta if the base
is in the same pack and already written out. The first condition
addresses your concern, and the second handles the case
where the base object gets pushed to the next pack.
These restrictions should be loosened for --thin-pack
but I didn't do that yet.
Also, --pack-limit turns on --no-reuse-delta.
This is not necessary, but not doing it would have meant
hacking up even more conditions which I didn't want to do
on a newbie submission.
quoted
When --stdout is also specified, all objects in the
resulting packfile(s) _start_ at offsets less than N.
All the packfiles appear concatenated on stdout,
and each has its object count set to 0. The behavior
without --stdout cannot be duplicated here since
lseek(2) is not generally possible on stdout.
Please scrap that. There is simply no point making --pack-limit and
--stdout work together. If the amount of data to send over the GIT
protocol exceeds 4G (or whatever) it is the receiving end's business to
split it up _if_ it wants/has to. The alternative is just too ugly.
I have a similar but much weaker reaction, but Linus specifically asked for
this combination to work. So I made it work as well as possible
given no seeking.
quoted
When --blob-limit=N is specified, blobs whose uncompressed
size is greater than or equal to N are omitted from the pack(s).
If --pack-limit is specified, --blob-limit is not, and
--stdout is not, then --blob-limit defaults to 1/4
of the --pack-limit.
Is this really useful?
If you have a pack size limit and a blob cannot make it even in a pack
of its
own then you're screwed anyway. It is much better to simply fail the
operation than leaving some blobs behind. IOW I don't see the
usefulness of this feature.
I agree if --stdout is specified. This is why --pack-limit && --stdout
DON'T turn on --blob-limit if not specified.
However, if I'm building packs inside a non-(web-)published
repository, I find this useful. First of all, if there's some blob bigger
than the --pack-limit I must drop it anyway -- it's not clear to me that
the mmap window code works on 32b systems
with >2GB-sized objects in packs. An "all-or-nothing" limitation
wouldn't be helpful to me.
But blobs even close to the packfile limit don't seem all that useful
to pack either (this of course is a weaker argument).
In the sample (p4) checkout I'm testing on [i.e. no history],
I have 56K+ objects consuming ~55GB uncompressed;
there are 9 blobs over 500MB each uncompressed.
I'm guessing packing them is not a performance advantage,
and I certainly wouldn't want frequently-used objects to be
stuck between them. [ I guess my repo stats are going to
be a bit strange ;-) ]
Packing plays two roles: archive storage (long life) and
transmission (possibly short life).
These seem to pull the packing code in different directions.
Thanks,
--
Dana L. How danahow@gmail.com +1 650 804 5991 cell
From: Nicolas Pitre <hidden> Date: 2016-06-15 22:43:03
On Wed, 4 Apr 2007, Dana How wrote:
On 4/4/07, Nicolas Pitre [off-list ref] wrote:
quoted
On Wed, 4 Apr 2007, Dana How wrote:
quoted
The motivations are to better support portable media,
older filesystems, and larger repositories without
awkward enormous packfiles.
I wouldn't qualify "enormous" pack files as "awkward".
It will always be more efficient to have only one pack to deal with
(when possible of course).
Yes. "(when possible of course)" refers to the remaining motivations
I didn't explicitly mention: the 32b offset limit in .idx files,
and keeping the mmap code working on a 32b system.
I realize there are better solutions in the pipeline,
but I'd like to address this now (for my own use) and hopefully
also create something useful for 4GB-limited filesystems,
USB sticks, etc.
I think this is a valid feature to have, no problem there.
quoted
quoted
When --pack-limit[=N] is specified and --stdout is not,
all bytes in the resulting packfile(s) appear at offsets
less than N (which defaults to 1<<31). The default
guarantees mmap(2) on 32b systems never sees signed off_t's.
The object stream may be broken into multiple packfiles
as a result, each properly and conventionally built.
This sounds fine. *However* how do you ensure that the second pack (or
subsequent packs) is self contained with regards to delta base objects
when it is _not_ meant to be a thin pack?
Good question. Search for "int usable_delta" in the patch.
With --pack-limit (offset_limit in C), you can use a delta if the base
is in the same pack and already written out. The first condition
addresses your concern, and the second handles the case
where the base object gets pushed to the next pack.
These restrictions should be loosened for --thin-pack
but I didn't do that yet.
OK.
Also, --pack-limit turns on --no-reuse-delta.
This is not necessary, but not doing it would have meant
hacking up even more conditions which I didn't want to do
on a newbie submission.
Thing is delta reusing is one of the most important feature for good
performances so it has to work.
But let's take a moment to talk about your "newbie submission". This
patch is _way_ too large and covers too many things at once. You'll
have to split it in many smaller logical units pretty please. For
example, if you have to borrow code from fast-import then make a patch
that perform that code movement _only_. Then you can have another patch
that move code around within builtin-pack-objects.c without changing any
functionality just to prepare the way for the next patch which would
concentrate on the new feature only. Then make sure you have the
addition of the new capability separate from the patch that let other
part of GIT use it. Etc.
That would be much easier for us to comment on smaller patches, and
especially easier for you to rework one of the smaller patches than the
big one if need be.
I looked at your patch and there are things I like and other things I
don't like at all. Because it is a single large patch I may only NAK
the whole of it.
quoted
quoted
When --stdout is also specified, all objects in the
Please scrap that. There is simply no point making --pack-limit and
--stdout work together. If the amount of data to send over the GIT
protocol exceeds 4G (or whatever) it is the receiving end's business to
split it up _if_ it wants/has to. The alternative is just too ugly.
I have a similar but much weaker reaction, but Linus specifically asked for
this combination to work.
Linus is a total imcompetent who knows nothing about programming or good
taste. So never ever listen to what he says. He is wrong, always.
And in this case he's more wrong than usual.
;-)
So I made it work as well as possible
given no seeking.
The fact is that there is no point in imposing split packs on the
receiving end if it can accept a single pack just fine. Pack layout is
and must remain a local policy, not something that the remote end felt
was good for the peer. This is true for the treshold value to decide
whether fetched packs are kept as is or unpacked as loose objects. This
is the same issue for pack size limit.
And as you discovered yourself, it is quite messy to implement in
pack-objects when the output is a stream because you don't know in
advance how many objects will be sent so you have to invent special
markers to indicate the end of pack. This in turn doesn't let
index-pack know how much to expect and provide some progress report at
all. The appropriate location for the splitting of packs in a fetch
context is really within index-pack not in pack-objects. And it is
probably so much easier to do in index-pack anyway.
quoted
quoted
When --blob-limit=N is specified, blobs whose uncompressed
size is greater than or equal to N are omitted from the pack(s).
If --pack-limit is specified, --blob-limit is not, and
--stdout is not, then --blob-limit defaults to 1/4
of the --pack-limit.
Is this really useful?
If you have a pack size limit and a blob cannot make it even in a
pack of its own then you're screwed anyway. It is much better to
simply fail the operation than leaving some blobs behind. IOW I
don't see the usefulness of this feature.
I agree if --stdout is specified. This is why --pack-limit && --stdout
DON'T turn on --blob-limit if not specified.
Let's forget about --stdout. I hope I convinced you that it doesn't
make sense.
However, if I'm building packs inside a non-(web-)published
repository, I find this useful. First of all, if there's some blob bigger
than the --pack-limit I must drop it anyway -- it's not clear to me that
the mmap window code works on 32b systems
with >2GB-sized objects in packs. An "all-or-nothing" limitation
wouldn't be helpful to me.
But do you realize that if you drop even a single object you are
screwed? The fetching of a repo that is missing objects is a corrupt
fetch. You cannot just sent a bunch of objects and leave a couple
behind in the hope that the peer will never need them. For one the peer
would never be able to perform a successful git-fsck.
If you care about your local usage only then this feature is bogus as
well. The blob _has_ to exist as a loose object before ever being
packed. If you have blobs larger than 2GB or 4GB and your filesystem is
unable to cope with that then you're screwed already. You won't be able
to check them out, etc.
But blobs even close to the packfile limit don't seem all that useful
to pack either (this of course is a weaker argument).
In the extreme case where you have a blob that is near the pack size
limit then you'll end up with one pack per blob. No problem there. If
you're lucky then you might have 10 big blobs which size is near the
pack size limit, but because they delta well against each other you
might be able to stuff them all in a single pack.
And if a blob is larger than the pack limit then either you should
increase your pack size limit, or if the pack limit is already near the
filesystem capacity for a single file then the blob cannot exist on that
filesystem in the first place.
So you still have to convince me this is a useful feature.
Packing plays two roles: archive storage (long life) and
transmission (possibly short life).
Packing is also a _huge_ performance boost. Don't underestimate that.
Nicolas
From: Shawn O. Pearce <hidden> Date: 2016-06-15 22:43:03
Dana How [off-list ref] wrote:
it's not clear to me that
the mmap window code works on 32b systems
with >2GB-sized objects in packs.
Hmmph. Depends on the system.
For glibc we do try to set _FILE_OFFSET_BITS to 64, so that
even on 32 bit glibcs we pick up a 64 bit off_t. But really old
glibcs won't have any 64 bit off_t support no matter what we do.
And some systems don't know what _FILE_OFFSET_BITS is, so they
give us whatever size off_t they want.
Forget mmap. In a 32 bit off_t case there is absolutely no way
that open_packed_git_1 can verify the packfile signature (last
20 bytes) against the index if the packfile is larger than 2 GiB.
In this case we will have an off_t that is negative, subtract 20
from it, and its still probably negative. I doubt SEEK_SET will
like a negative offset. This of course assumes that the earlier
fstat call actually succeeded on such a file a large file with
such a small off_t.
If we get through that open_packed_git_1 and actually verify the
signature, we either have some random sequence in the middle of the
packfile that matches the signature in the index (sort of unlikely),
or our off_t is actually large enough to handle the window position
tests that the use_pack and in_window functions perform. In this
latter case we shouldn't have any problems with the mmap code.
--
Shawn.
From: Shawn O. Pearce <hidden> Date: 2016-06-15 22:43:03
Nicolas Pitre [off-list ref] wrote:
But let's take a moment to talk about your "newbie submission". This
patch is _way_ too large and covers too many things at once. You'll
have to split it in many smaller logical units pretty please.
Yes, absolutely! If I was doing this series I'd probably have this
broken into at least 5, maybe even 8 patches. There's a lot of
stuff here, and some of it was completely unrelated to each other.
Like a fix for lseek arguments? Uhh, that's so not what this patch
was about, but is still a good fix. Nice catch. Lets get it in,
but properly please.
I also noticed a lot of "/* and here we could do... */" markers.
I think these belong more in the commit message than in the code
itself, as the code gets really cluttered after a while with the
todo markers that nobody has done yet. Odds are, if you don't do
it in this series, it won't get done for quite a while, if ever.
Mention it in your commit message, so others are aware of it,
and leave it at that.
On Wed, 4 Apr 2007, Dana How wrote:
quoted
I have a similar but much weaker reaction, but Linus specifically asked for
this combination to work.
Linus is a total imcompetent who knows nothing about programming or good
taste. So never ever listen to what he says. He is wrong, always.
Now now, I wouldn't go that far...
And in this case he's more wrong than usual.
But yes, in this case I certainly I agree with you Nico. ;-)
The appropriate location for the splitting of packs in a fetch
context is really within index-pack not in pack-objects. And it is
probably so much easier to do in index-pack anyway.
Or in a push context. I have started down the road of doing the
pack splitting in index-pack, but never was able to finish it.
It is totally doable in index-pack, but it turned out to be more
work than I had time for when I started it.
I still have the topic laying in my repository, and it is pushed
to my fastimport.git fork on repo.or.cz, if someone wants to take
a look at it. The patch is far from complete, hence no patch to
mailing list.
quoted
But blobs even close to the packfile limit don't seem all that useful
to pack either (this of course is a weaker argument).
So you still have to convince me this is a useful feature.
Me too. Nico's right. And lets just assume we are in a bad case
where we need to put one (and only one) such blob into each packfile.
The packfile on disk will be an additional 32 bytes larger than if
it were to stay a loose file, and that's assuming the loose file was
created using the newer-style loose file format. This is unlikely
to cause a large disk space overhead.
Yea, we to have an extra SHA-1 we have to compute when we created
the file, but then we also have another SHA-1 to verify that the
zlib stream is not corrupt, *without* unpacking the zlib stream.
That in and of itself could be useful for an fsck, we could do a
faster rule that says "if there's only a couple of blobs in this
packfile, and the thing is *big*, just do a check of the packfile
SHA-1 and assume the blobs are OK". So there actually might be
value to that extra SHA-1 after all.
--
Shawn.
For glibc we do try to set _FILE_OFFSET_BITS to 64
I repeat: that's _broken_.
It's in no way portable. It's a glibc horror. It should not be used.
It was a quick hack, but the real way to do it is to use "loff_t" and
"llseek".
But there simply isn't any way to do mmap() or pread() portably outside
the 32-bit area. So there are good reasons why we should just limit
pack-files to 32-bits on 32-bit architectures.
So I think that Dana's approach is just fundamentally correct. Yeah, we
should probably have a 64-bit index as a *possibility*, but it simply
isn't a replacement for "keep packs under 2GB in size".
Linus
From: Shawn O. Pearce <hidden> Date: 2016-06-15 22:43:03
Linus Torvalds [off-list ref] wrote:
On Thu, 5 Apr 2007, Shawn O. Pearce wrote:
quoted
For glibc we do try to set _FILE_OFFSET_BITS to 64
I repeat: that's _broken_.
It's in no way portable. It's a glibc horror. It should not be used.
It was a quick hack, but the real way to do it is to use "loff_t" and
"llseek".
Sure, OK, but that libc function doesn't exist on Mac OS X:
man llseek:
This function is Linux-specific, and should not be used in programs
intended to be portable.
So we'd need our own horror to wrap llseek as an lseek fake-alike
anyway. That's what that glibc horror does, and we didn't have to
write that code. :-)
But there simply isn't any way to do mmap() or pread() portably outside
the 32-bit area. So there are good reasons why we should just limit
pack-files to 32-bits on 32-bit architectures.
Not unless your off_t is 64 bits, no. If it is 64 bits then you
should be able to do a pread or mmap starting past the first 2 GiB.
You just might not be able to ask for a mmap that exceeds 2 GiB in
size, as your size_t may not be that large. E.g., Darwin/Mac OS X.
Hence the sliding window mmap.
So I think that Dana's approach is just fundamentally correct. Yeah, we
should probably have a 64-bit index as a *possibility*, but it simply
isn't a replacement for "keep packs under 2GB in size".
I'm not disagreeing. Some filesystems (FAT on a USB stick, Dana's
example) just don't want files larger than 2 GiB. So keeping them
small has a number of advantages. Plus they are easier to burn on
DVD: 2 packs per DVD. ;-)
I was simply trying to point out that the mmap code isn't broken
if the off_t is able to handle a file of that size; and if it can't
then other things are broken, like a simple lseek.
--
Shawn.
Sure, OK, but that libc function doesn't exist on Mac OS X:
My bad. It's *not* linux-specific like the OSX man-page apparently says,
it's very traditional. But the right name is "lseek64()" (and offt64_t for
the size).
Of course, OSX didn't have some of the backwards-compatibility issues with
decades ago, so they just made off_t 64-bit by default. Maybe they don't
even bother to do the trivial portability things to support programs that
try to be portable..
Anyway, we should use open64(), lseek64() and friends to be as portable as
possible.. And if some system doesn't have them, just use the normal ops,
and pray that they are already 64-bit safe..
Linus
From: Shawn O. Pearce <hidden> Date: 2016-06-15 22:43:03
Linus Torvalds [off-list ref] wrote:
On Thu, 5 Apr 2007, Shawn O. Pearce wrote:
quoted
Sure, OK, but that libc function doesn't exist on Mac OS X:
My bad. It's *not* linux-specific like the OSX man-page apparently says,
it's very traditional. But the right name is "lseek64()" (and offt64_t for
the size).
Sorry for the confusion, that manpage snippet came from a Gentoo/x86
system. But I digress, you are right, the right interfaces to be
using here is lseek64 and open64.
Now those also don't eixst on OSX, because as you pointed out, they
have no legacy to deal with and are just using sizeof off_t == 8.
So we'd probably have to do something like:
#ifndef _LFS_LARGEFILE
#define open64 open
#define lseek64 lseek
#endif
and then start using the open64/lseek64 variants instead. Or do
the reverse #define's. ;-)
--
Shawn.