{"thread":{"id":"23424","subject":"[PATCH 1/2] index-pack: smarter memory usage when resolving deltas","startedAt":"2010-04-12T02:57:26Z","lastAt":"2010-04-12T15:47:25Z","messageCount":6,"participants":["Nicolas Pitre","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"139305","messageId":"1271041047-32563-1-git-send-email-nico@fluxnic.net","threadId":"23424","inReplyTo":null,"subject":"[PATCH 1/2] index-pack: smarter memory usage when resolving deltas","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2010-04-12T02:57:26Z","receivedAt":"2010-04-12T02:57:26Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"In the same spirit as commit 9892bebafe, let's avoid allocating the full\nbuffer for the deflated data in get_data_from_pack() in order to inflate\nit.  Let's read and inflate the data in chunks instead to reduce memory\nusage.\n\nSigned-off-by: Nicolas Pitre <nico@fluxnic.net>\n---\n builtin/index-pack.c |   39 +++++++++++++++++++--------------------\n 1 files changed, 19 insertions(+), 20 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex b4cf8c5..c746d3b 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -359,34 +359,33 @@ static void *get_data_from_pack(struct object_entry *obj)\n {\n \toff_t from = obj[0].idx.offset + obj[0].hdr_size;\n \tunsigned long len = obj[1].idx.offset - from;\n-\tunsigned long rdy = 0;\n-\tunsigned char *src, *data;\n+\tunsigned char *data, inbuf[4096];\n \tz_stream stream;\n-\tint st;\n+\tint status;\n \n-\tsrc = xmalloc(len);\n-\tdata = src;\n-\tdo {\n-\t\tssize_t n = pread(pack_fd, data + rdy, len - rdy, from + rdy);\n-\t\tif (n < 0)\n-\t\t\tdie_errno(\"cannot pread pack file\");\n-\t\tif (!n)\n-\t\t\tdie(\"premature end of pack file, %lu bytes missing\",\n-\t\t\t    len - rdy);\n-\t\trdy += n;\n-\t} while (rdy < len);\n \tdata = xmalloc(obj->size);\n \tmemset(&stream, 0, sizeof(stream));\n+\tgit_inflate_init(&stream);\n \tstream.next_out = data;\n \tstream.avail_out = obj->size;\n-\tstream.next_in = src;\n-\tstream.avail_in = len;\n-\tgit_inflate_init(&stream);\n-\twhile ((st = git_inflate(&stream, Z_FINISH)) == Z_OK);\n+\n+\tdo {\n+\t\tssize_t n = (len < sizeof(inbuf)) ? len : sizeof(inbuf);\n+\t\tn = pread(pack_fd, inbuf, n, from);\n+\t\tif (n < 0)\n+\t\t\tdie_errno(\"cannot pread pack file\");\n+\t\tif (!n)\n+\t\t\tdie(\"premature end of pack file, %lu bytes missing\", len);\n+\t\tfrom += n;\n+\t\tlen -= n;\n+\t\tstream.next_in = inbuf;\n+\t\tstream.avail_in = n;\n+\t\tstatus = git_inflate(&stream, 0);\n+\t} while (len && status == Z_OK && !stream.avail_in);\n \tgit_inflate_end(&stream);\n-\tif (st != Z_STREAM_END || stream.total_out != obj->size)\n+\t/* This has been inflated OK when first encoumtered, so... */\n+\tif (status != Z_STREAM_END || stream.total_out != obj->size)\n \t\tdie(\"serious inflate inconsistency\");\n-\tfree(src);\n \treturn data;\n }\n \n-- \n1.7.1.rc1.237.ge1730.dirty\n"},{"id":"139306","messageId":"1271041047-32563-2-git-send-email-nico@fluxnic.net","threadId":"23424","inReplyTo":"1271041047-32563-1-git-send-email-nico@fluxnic.net","subject":"[PATCH 2/2] index-pack: rationalize unpack_entry_data()","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2010-04-12T02:57:27Z","receivedAt":"2010-04-12T02:57:27Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"Rework the loop to remove duplicated calls to use() and fill(), and\nto make the code easier to read.\n\nSigned-off-by: Nicolas Pitre <nico@fluxnic.net>\n---\n builtin/index-pack.c |   19 ++++++++-----------\n 1 files changed, 8 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex c746d3b..69b9167 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -266,27 +266,24 @@ static void unlink_base_data(struct base_data *c)\n \n static void *unpack_entry_data(unsigned long offset, unsigned long size)\n {\n+\tint status;\n \tz_stream stream;\n \tvoid *buf = xmalloc(size);\n \n \tmemset(&stream, 0, sizeof(stream));\n+\tgit_inflate_init(&stream);\n \tstream.next_out = buf;\n \tstream.avail_out = size;\n-\tstream.next_in = fill(1);\n-\tstream.avail_in = input_len;\n-\tgit_inflate_init(&stream);\n \n-\tfor (;;) {\n-\t\tint ret = git_inflate(&stream, 0);\n-\t\tuse(input_len - stream.avail_in);\n-\t\tif (stream.total_out == size && ret == Z_STREAM_END)\n-\t\t\tbreak;\n-\t\tif (ret != Z_OK)\n-\t\t\tbad_object(offset, \"inflate returned %d\", ret);\n+\tdo {\n \t\tstream.next_in = fill(1);\n \t\tstream.avail_in = input_len;\n-\t}\n+\t\tstatus = git_inflate(&stream, 0);\n+\t\tuse(input_len - stream.avail_in);\n+\t} while (status == Z_OK);\n \tgit_inflate_end(&stream);\n+\tif (stream.total_out != size || status != Z_STREAM_END)\n+\t\tbad_object(offset, \"inflate returned %d\", status);\n \treturn buf;\n }\n \n-- \n1.7.1.rc1.237.ge1730.dirty\n"},{"id":"139307","messageId":"4BC2BE09.7060809@viscovery.net","threadId":"23424","inReplyTo":"1271041047-32563-1-git-send-email-nico@fluxnic.net","subject":"Re: [PATCH 1/2] index-pack: smarter memory usage when resolving deltas","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2010-04-12T06:30:33Z","receivedAt":"2010-04-12T06:30:33Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 4/12/2010 4:57, schrieb Nicolas Pitre:\n> --- a/builtin/index-pack.c\n> +++ b/builtin/index-pack.c\n> @@ -359,34 +359,33 @@ static void *get_data_from_pack(struct object_entry *obj)\n>  {\n>  \toff_t from = obj[0].idx.offset + obj[0].hdr_size;\n>  \tunsigned long len = obj[1].idx.offset - from;\n> -\tunsigned long rdy = 0;\n> -\tunsigned char *src, *data;\n> +\tunsigned char *data, inbuf[4096];\n\nWith this tiny buffer we make way more pread() calls than we used to,\nright? This hurts emulated pread(), where we do at least 4 syscalls per\npread(). Shouldn't the buffer be much larger, like min(len,16*4096)?\n\n-- Hannes\n"},{"id":"139308","messageId":"4BC2BF69.8050100@viscovery.net","threadId":"23424","inReplyTo":"1271041047-32563-2-git-send-email-nico@fluxnic.net","subject":"Re: [PATCH 2/2] index-pack: rationalize unpack_entry_data()","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2010-04-12T06:36:25Z","receivedAt":"2010-04-12T06:36:25Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 4/12/2010 4:57, schrieb Nicolas Pitre:\n> -\tfor (;;) {\n> -\t\tint ret = git_inflate(&stream, 0);\n> -\t\tuse(input_len - stream.avail_in);\n> -\t\tif (stream.total_out == size && ret == Z_STREAM_END)\n> -\t\t\tbreak;\n> -\t\tif (ret != Z_OK)\n> -\t\t\tbad_object(offset, \"inflate returned %d\", ret);\n> +\tdo {\n>  \t\tstream.next_in = fill(1);\n>  \t\tstream.avail_in = input_len;\n> -\t}\n> +\t\tstatus = git_inflate(&stream, 0);\n> +\t\tuse(input_len - stream.avail_in);\n> +\t} while (status == Z_OK);\n>  \tgit_inflate_end(&stream);\n> +\tif (stream.total_out != size || status != Z_STREAM_END)\n> +\t\tbad_object(offset, \"inflate returned %d\", status);\n>  \treturn buf;\n\nIs stream.total_out still valid after inflateEnd() (and I mean \"valid by\ndefinition\", not just \"by accident\")? To stay on the safe side, you should\ncall git_inflate_end only after this last check.\n\n-- Hannes\n"},{"id":"139353","messageId":"alpine.LFD.2.00.1004121141480.7232@xanadu.home","threadId":"23424","inReplyTo":"4BC2BE09.7060809@viscovery.net","subject":"Re: [PATCH 1/2] index-pack: smarter memory usage when resolving deltas","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2010-04-12T15:45:24Z","receivedAt":"2010-04-12T15:45:24Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Mon, 12 Apr 2010, Johannes Sixt wrote:\n\n> Am 4/12/2010 4:57, schrieb Nicolas Pitre:\n> > --- a/builtin/index-pack.c\n> > +++ b/builtin/index-pack.c\n> > @@ -359,34 +359,33 @@ static void *get_data_from_pack(struct object_entry *obj)\n> >  {\n> >  \toff_t from = obj[0].idx.offset + obj[0].hdr_size;\n> >  \tunsigned long len = obj[1].idx.offset - from;\n> > -\tunsigned long rdy = 0;\n> > -\tunsigned char *src, *data;\n> > +\tunsigned char *data, inbuf[4096];\n> \n> With this tiny buffer we make way more pread() calls than we used to,\n> right? This hurts emulated pread(), where we do at least 4 syscalls per\n> pread(). Shouldn't the buffer be much larger, like min(len,16*4096)?\n\nWell, that is certainly a good compromize.  The buffer would need to be \nmalloc()'d and free()'d again though.\n\n\nNicolas\n"},{"id":"139354","messageId":"alpine.LFD.2.00.1004121145290.7232@xanadu.home","threadId":"23424","inReplyTo":"4BC2BF69.8050100@viscovery.net","subject":"Re: [PATCH 2/2] index-pack: rationalize unpack_entry_data()","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2010-04-12T15:47:25Z","receivedAt":"2010-04-12T15:47:25Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Mon, 12 Apr 2010, Johannes Sixt wrote:\n\n> Am 4/12/2010 4:57, schrieb Nicolas Pitre:\n> > -\tfor (;;) {\n> > -\t\tint ret = git_inflate(&stream, 0);\n> > -\t\tuse(input_len - stream.avail_in);\n> > -\t\tif (stream.total_out == size && ret == Z_STREAM_END)\n> > -\t\t\tbreak;\n> > -\t\tif (ret != Z_OK)\n> > -\t\t\tbad_object(offset, \"inflate returned %d\", ret);\n> > +\tdo {\n> >  \t\tstream.next_in = fill(1);\n> >  \t\tstream.avail_in = input_len;\n> > -\t}\n> > +\t\tstatus = git_inflate(&stream, 0);\n> > +\t\tuse(input_len - stream.avail_in);\n> > +\t} while (status == Z_OK);\n> >  \tgit_inflate_end(&stream);\n> > +\tif (stream.total_out != size || status != Z_STREAM_END)\n> > +\t\tbad_object(offset, \"inflate returned %d\", status);\n> >  \treturn buf;\n> \n> Is stream.total_out still valid after inflateEnd() (and I mean \"valid by\n> definition\", not just \"by accident\")? \n\nI don't see any indication to the contrary.\n\n> To stay on the safe side, you should call git_inflate_end only after \n> this last check.\n\nAgreed.  Will do.\n\n\nNicolas\n"}]}