Re: [PATCH v4 1/5] object-file: refactor write_loose_object() to read buffer from stream
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Dec 3, 2021, 13:28 UTC
- Message-ID
- <211203.86r1atst50.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <20211203093530.93589-2-chiyutianyi@gmail.com>
On Fri, Dec 03 2021, Han Xin wrote:
Show 30 quoted lines
> 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?
Show 5 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 6 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 8 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 13 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..
Show 25 quoted lines
> /* 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?...
> 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...
Show 6 quoted lines
> +struct input_stream {
> + const void *(*read)(struct input_stream *, unsigned long *len);
> + void *data;
> + size_t size;
> +};
> +Ah, and here's the size_t... :)