Re: [PATCH v4 1/5] object-file: refactor write_loose_object() to read buffer from stream
On Fri, Dec 3, 2021 at 9:41 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
Show 40 quoted lines
>
>
> On Fri, Dec 03 2021, Han Xin wrote:
>
> > From: Han Xin <hanxin.hx@alibaba-inc.com>
> >
> > We used to call "get_data()" in "unpack_non_delta_entry()" to read the
> > entire contents of a blob object, no matter how big it is. This
> > implementation may consume all the memory and cause OOM.
> >
> > This can be improved by feeding data to "write_loose_object()" in a
> > stream. The input stream is implemented as an interface. In the first
> > step, we make a simple implementation, feeding the entire buffer in the
> > "stream" to "write_loose_object()" as a refactor.
> >
> > Helped-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> > Signed-off-by: Han Xin <hanxin.hx@alibaba-inc.com>
> > ---
> > object-file.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++----
> > object-store.h | 6 ++++++
> > 2 files changed, 55 insertions(+), 4 deletions(-)
> >
> > diff --git a/object-file.c b/object-file.c
> > index eb972cdccd..82656f7428 100644
> > --- a/object-file.c
> > +++ b/object-file.c
> > @@ -1860,8 +1860,26 @@ static int create_tmpfile(struct strbuf *tmp, const char *filename)
> > return fd;
> > }
> >
> > +struct simple_input_stream_data {
> > + const void *buf;
> > + unsigned long len;
> > +};
>
> I see why you picked "const void *buf" here, over say const char *, it's
> what "struct input_stream" uses.
>
> But why not use size_t for the length, as input_stream does?
>Yes, "size_t" will be better here.
Show 8 quoted lines
> > +static const void *feed_simple_input_stream(struct input_stream *in_stream, unsigned long *len)
> > +{
> > + struct simple_input_stream_data *data = in_stream->data;
> > +
> > + if (data->len == 0) {
>
> nit: if (!data->len)...
>Show 18 quoted lines
> > + *len = 0;
> > + return NULL;
> > + }
> > + *len = data->len;
> > + data->len = 0;
> > + return data->buf;
>
> But isn't the body of this functin the same as:
>
> *len = data->len;
> if (!len)
> return NULL;
> data->len = 0;
> return data->buf;
>
> I.e. you don't need the condition for setting "*len" if it's 0, then
> data->len is also 0. You just want to return NULL afterwards, and not
> set (harmless, but no need) data->len to 0)< or return data->buf.
Show 33 quoted lines
> > + struct input_stream in_stream = {
> > + .read = feed_simple_input_stream,
> > + .data = (void *)&(struct simple_input_stream_data) {
> > + .buf = buf,
> > + .len = len,
> > + },
> > + .size = len,
> > + };
>
> Maybe it's that I'm unused to it, but I find this a bit more readable:
>
> @@ -2013,12 +2011,13 @@ int write_object_file_flags(const void *buf, unsigned long len,
> {
> char hdr[MAX_HEADER_LEN];
> int hdrlen = sizeof(hdr);
> + struct simple_input_stream_data tmp = {
> + .buf = buf,
> + .len = len,
> + };
> struct input_stream in_stream = {
> .read = feed_simple_input_stream,
> - .data = (void *)&(struct simple_input_stream_data) {
> - .buf = buf,
> - .len = len,
> - },
> + .data = (void *)&tmp,
> .size = len,
> };
>
> Yes there's a temporary variable, but no denser inline casting. Also
> easier to strep through in a debugger (which will have the type
> information on "tmp".
>Show 44 quoted lines
> > int hash_object_file_literally(const void *buf, unsigned long len,
> > @@ -1977,6 +2006,14 @@ int hash_object_file_literally(const void *buf, unsigned long len,
> > {
> > char *header;
> > int hdrlen, status = 0;
> > + struct input_stream in_stream = {
> > + .read = feed_simple_input_stream,
> > + .data = (void *)&(struct simple_input_stream_data) {
> > + .buf = buf,
> > + .len = len,
> > + },
> > + .size = len,
> > + };
>
> ditto..
>
> > /* type string, SP, %lu of the length plus NUL must fit this */
> > hdrlen = strlen(type) + MAX_HEADER_LEN;
> > @@ -1988,7 +2025,7 @@ int hash_object_file_literally(const void *buf, unsigned long len,
> > goto cleanup;
> > if (freshen_packed_object(oid) || freshen_loose_object(oid))
> > goto cleanup;
> > - status = write_loose_object(oid, header, hdrlen, buf, len, 0, 0);
> > + status = write_loose_object(oid, header, hdrlen, &in_stream, 0, 0);
> >
> > cleanup:
> > free(header);
> > @@ -2003,14 +2040,22 @@ int force_object_loose(const struct object_id *oid, time_t mtime)
> > char hdr[MAX_HEADER_LEN];
> > int hdrlen;
> > int ret;
> > + struct simple_input_stream_data data;
> > + struct input_stream in_stream = {
> > + .read = feed_simple_input_stream,
> > + .data = &data,
> > + };
> >
> > if (has_loose_object(oid))
> > return 0;
> > buf = read_object(the_repository, oid, &type, &len);
> > + in_stream.size = len;
>
> Why are we setting this here?...
>Yes, putting "in_stream.size=len;" here was a stupid decision.
Show 18 quoted lines
> > if (!buf)
> > return error(_("cannot read object for %s"), oid_to_hex(oid));
>
> ...Insted of after this point, as we may error and never use it?
>
> > + data.buf = buf;
> > + data.len = len;
>
> Probably won't matter, just a nit...
>
> > +struct input_stream {
> > + const void *(*read)(struct input_stream *, unsigned long *len);
> > + void *data;
> > + size_t size;
> > +};
> > +
>
> Ah, and here's the size_t... :)