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 [off-list ref] wrote:

On Fri, Dec 03 2021, Han Xin wrote:

quoted

From: Han Xin <redacted>

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 [off-list ref]
Signed-off-by: Han Xin <redacted>
---
 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.

quoted

+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)...

Will apply.

quoted

+             *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.

Will apply.

quoted

+     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".

Will apply.

quoted

 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..

quoted

      /* 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.

quoted

      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?

quoted

+     data.buf = buf;
+     data.len = len;

Probably won't matter,  just a nit...

quoted

+struct input_stream {
+     const void *(*read)(struct input_stream *, unsigned long *len);
+     void *data;
+     size_t size;
+};
+

Ah, and here's the size_t... :)