From: Junio C Hamano <hidden> Date: 2017-08-14 17:08:14
Junio C Hamano [off-list ref] writes:
One interesting question is which of these two types we should use
for the size of objects Git uses.
Most of the "interesting" operations done by Git require that the
thing is in core as a whole before we can do anything (e.g. compare
two such things to produce delta, have one in core and apply patch),
so it is tempting that we deal with size_t, but at the lowest level
to serve as a SCM, i.e. recording the state of a file at each
version, we actually should be able to exceed the in-core
limit---both "git add" of a huge file whose contents would not fit
in-core and "git checkout" of a huge blob whose inflated contents
would not fit in-core should (in theory, modulo bugs) be able to
exercise the streaming interface to handle such case without holding
everything in-core at once. So from that point of view, even size_t
may not be the "correct" type to use.
A few additions to the above observations.
- We have varint that encodes how far the location from a delta
representation of an object to its base object in the packfile.
Both encoding and decoding sides in the current code use off_t to
represent this offset, so we can already reference an object that
is far in the same packfile as a base.
- I think it is OK in practice to limit the size of individual
objects to size_t (i.e. on 32-bit arch, you cannot interact with
a repository with an object whose size exceeds 4GB). Using off_t
would allow occasional ultra-huge objects that can only be added
and checked in via the streaming API on such a platform, but I
suspect that it may become too much of a hassle to maintain.
It may help reducing the maintenance if we introduced obj_size_t
that is defined to be size_t for now, so that we can later swap
it to ofs_t or some larger type when we know we do need to
support objects whose size cannot be expressed in size_t, but I
do not offhand know what the pros-and-cons with such an approach
would look like.
Thanks.
From: Ramsay Jones <hidden> Date: 2017-08-14 19:31:59
On 14/08/17 18:08, Junio C Hamano wrote:
Junio C Hamano [off-list ref] writes:
quoted
One interesting question is which of these two types we should use
for the size of objects Git uses.
Most of the "interesting" operations done by Git require that the
thing is in core as a whole before we can do anything (e.g. compare
two such things to produce delta, have one in core and apply patch),
so it is tempting that we deal with size_t, but at the lowest level
to serve as a SCM, i.e. recording the state of a file at each
version, we actually should be able to exceed the in-core
limit---both "git add" of a huge file whose contents would not fit
in-core and "git checkout" of a huge blob whose inflated contents
would not fit in-core should (in theory, modulo bugs) be able to
exercise the streaming interface to handle such case without holding
everything in-core at once. So from that point of view, even size_t
may not be the "correct" type to use.
A few additions to the above observations.
- We have varint that encodes how far the location from a delta
representation of an object to its base object in the packfile.
Both encoding and decoding sides in the current code use off_t to
represent this offset, so we can already reference an object that
is far in the same packfile as a base.
- I think it is OK in practice to limit the size of individual
objects to size_t (i.e. on 32-bit arch, you cannot interact with
a repository with an object whose size exceeds 4GB). Using off_t
would allow occasional ultra-huge objects that can only be added
and checked in via the streaming API on such a platform, but I
suspect that it may become too much of a hassle to maintain.
In a previous comment, I said that (on 32-bit Linux) it was likely
that an object of > 4GB could not be handled correctly anyway. (more
likely > 2GB). This was based on the code from (quite some) years ago.
In particular, before you added the "streaming API". So, maybe a 32-bit
arch _should_ be able to handle objects as large as the LFS API allows.
(Ignoring, for the moment, that I think anybody who puts files of that
size into an SCM probably gets what they deserve. :-P ).
The two patches I commented on, however, changed the type of some
variables from off_t to size_t. In general, the patches did not
seem to make anything worse, but these type changes could potentially
do harm. Hence my comment. (I still haven't tried the patches on my
32-bit Linux system. I only boot it up about once a week, and I would
rather wait until the patches are in the 'pu' branch before testing).
ATB,
Ramsay Jones
On Mon, Aug 14, 2017 at 08:31:50PM +0100, Ramsay Jones wrote:
On 14/08/17 18:08, Junio C Hamano wrote:
quoted
Junio C Hamano [off-list ref] writes:
quoted
One interesting question is which of these two types we should use
for the size of objects Git uses.
Most of the "interesting" operations done by Git require that the
thing is in core as a whole before we can do anything (e.g. compare
two such things to produce delta, have one in core and apply patch),
so it is tempting that we deal with size_t, but at the lowest level
to serve as a SCM, i.e. recording the state of a file at each
version, we actually should be able to exceed the in-core
limit---both "git add" of a huge file whose contents would not fit
in-core and "git checkout" of a huge blob whose inflated contents
would not fit in-core should (in theory, modulo bugs) be able to
exercise the streaming interface to handle such case without holding
everything in-core at once. So from that point of view, even size_t
may not be the "correct" type to use.
A few additions to the above observations.
- We have varint that encodes how far the location from a delta
representation of an object to its base object in the packfile.
Both encoding and decoding sides in the current code use off_t to
represent this offset, so we can already reference an object that
is far in the same packfile as a base.
- I think it is OK in practice to limit the size of individual
objects to size_t (i.e. on 32-bit arch, you cannot interact with
a repository with an object whose size exceeds 4GB). Using off_t
would allow occasional ultra-huge objects that can only be added
and checked in via the streaming API on such a platform, but I
suspect that it may become too much of a hassle to maintain.
In a previous comment, I said that (on 32-bit Linux) it was likely
that an object of > 4GB could not be handled correctly anyway. (more
likely > 2GB). This was based on the code from (quite some) years ago.
In particular, before you added the "streaming API". So, maybe a 32-bit
arch _should_ be able to handle objects as large as the LFS API allows.
(Ignoring, for the moment, that I think anybody who puts files of that
size into an SCM probably gets what they deserve. :-P ).
In general, yes.
I had a patch that allowed a 32 bit linux to commit a file >4GB.
There is however a restriction: The file must not yet be known to the
index. Otherwise the "diff" machinery kicks in, and tries to
compare the "old" and the "new" versions, which means that -both-
must fit into "memory" at the same time. Memory means here the available
address space rather then real memory.
So there may be room for improvements for 32 bit systems, but that is another
story, which can be developped independent of the ulong->size_t conversion.
The two patches I commented on, however, changed the type of some
variables from off_t to size_t. In general, the patches did not
seem to make anything worse, but these type changes could potentially
do harm. Hence my comment. (I still haven't tried the patches on my
32-bit Linux system. I only boot it up about once a week, and I would
rather wait until the patches are in the 'pu' branch before testing).
ATB,
Ramsay Jones
And here (in agreement with the answer from Junio):
We should not change any off_t into size_t, since that means a possible
downgrade.
Whenever an off_t is converted into size_t, a function in git-compat-util.h is
used:
static inline size_t xsize_t(off_t len)
{
if (len > (size_t) len)
die("Cannot handle files this big");
return (size_t)len;
}
Having written this, it may be a good idea to define a similar function
xulong() which is used when we do calls into zlib.
Thanks to Martin for working on this.
If there is a branch on a public repo, I can e.g. run the test suite on
different systems.
From: Ramsay Jones <hidden> Date: 2017-08-15 00:30:30
On 14/08/17 21:32, Torsten Bögershausen wrote:
In general, yes.
I had a patch that allowed a 32 bit linux to commit a file >4GB.
There is however a restriction: The file must not yet be known to the
index. Otherwise the "diff" machinery kicks in, and tries to
compare the "old" and the "new" versions, which means that -both-
must fit into "memory" at the same time. Memory means here the available
address space rather then real memory.
So there may be room for improvements for 32 bit systems, but that is another
story, which can be developped independent of the ulong->size_t conversion.
From: Martin Koegler <hidden> Date: 2017-08-16 20:22:56
On Mon, Aug 14, 2017 at 10:08:05AM -0700, Junio C Hamano wrote:
It may help reducing the maintenance if we introduced obj_size_t
that is defined to be size_t for now, so that we can later swap
it to ofs_t or some larger type when we know we do need to
support objects whose size cannot be expressed in size_t, but I
do not offhand know what the pros-and-cons with such an approach
would look like.
Where should the use of obj_size_t end and the use of size_t start?
We often determine a object size and then pass it to malloc.
We would start with a larger datatyp and then truncate for memory allocation, which use size_t.
Regards,
Martin
On Wed, Aug 16, 2017 at 10:22:39PM +0200, Martin Koegler wrote:
On Mon, Aug 14, 2017 at 10:08:05AM -0700, Junio C Hamano wrote:
quoted
It may help reducing the maintenance if we introduced obj_size_t
that is defined to be size_t for now, so that we can later swap
it to ofs_t or some larger type when we know we do need to
support objects whose size cannot be expressed in size_t, but I
do not offhand know what the pros-and-cons with such an approach
would look like.
Where should the use of obj_size_t end and the use of size_t start?
We often determine a object size and then pass it to malloc.
We would start with a larger datatyp and then truncate for memory allocation, which use size_t.
The truncation is done with xsize_t:
The "old" sha1_file.c has something like this:
idx_size = xsize_t(st.st_size);
I personally don't think that obj_size_t gives us so much.
There are "objects" which are "on disk", and they may have a length off_t,
And there are "objects" loaded into memory, and they have a length size_t.
And everybody can check that we use the right type.
Additionally I don't like it very much, when object size in memory is counted
in a 64 bit value (and this will happen if obj_size_ = off_t == 64bit)
Anyhow, to answer your question:
All calles xmalloc() must be prepared like this:
p = xmalloc(xsize_t(size_of_object));
That should do the trick.