{"thread":{"id":"35922","subject":"[PATCH] sha1_file: fix delta_stack memory leak in unpack_entry","startedAt":"2014-02-20T23:47:47Z","lastAt":"2014-02-22T15:44:05Z","messageCount":5,"participants":["Nguyễn Thái Ngọc Duy","Jeff King","Junio C Hamano","Thomas Rast"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"235115","messageId":"1392940067-4830-1-git-send-email-pclouds@gmail.com","threadId":"35922","inReplyTo":null,"subject":"[PATCH] sha1_file: fix delta_stack memory leak in unpack_entry","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2014-02-20T23:47:47Z","receivedAt":"2014-02-20T23:47:47Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This delta_stack array can grow to any length depending on the actual\ndelta chain, but we forget to free it. Normally it does not matter\nbecause we use small_delta_stack[] from stack and small_delta_stack\ncan hold 64-delta chains, more than standard --depth=50 in pack-objects.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n Found when trying to see if making some objects loose at this phase\n could help git-blame and how many objects will be loosened. Gotta go\n soon, didn't really test it, but I bet it'll work.\n\n sha1_file.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 6e8c05d..57ab15d 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2289,6 +2289,10 @@ void *unpack_entry(struct packed_git *p, off_t obj_offset,\n \t*final_size = size;\n \n \tunuse_pack(&w_curs);\n+\n+\tif (delta_stack != small_delta_stack)\n+\t\tfree(delta_stack);\n+\n \treturn data;\n }\n \n-- \n1.9.0.40.gaa8c3ea\n"},{"id":"235121","messageId":"20140221054148.GA24882@sigill.intra.peff.net","threadId":"35922","inReplyTo":"1392940067-4830-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] sha1_file: fix delta_stack memory leak in unpack_entry","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-21T05:41:48Z","receivedAt":"2014-02-21T05:41:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 21, 2014 at 06:47:47AM +0700, Nguyễn Thái Ngọc Duy wrote:\n\n> This delta_stack array can grow to any length depending on the actual\n> delta chain, but we forget to free it. Normally it does not matter\n> because we use small_delta_stack[] from stack and small_delta_stack\n> can hold 64-delta chains, more than standard --depth=50 in pack-objects.\n> \n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> Found when trying to see if making some objects loose at this phase\n> could help git-blame and how many objects will be loosened. Gotta go\n> soon, didn't really test it, but I bet it'll work.\n\nThis looks correct to me.\n\nFor fun, I measured \"git fsck\" on linux.git via massif. The peak memory\nusage dropped from 368MB to 306MB. That's probably an extreme case\nbecause it's a big repo and was packed with \"--aggressive\", but it's\nstill a nice improvement.\n\n-Peff\n"},{"id":"235147","messageId":"xmqqmwhk49sy.fsf@gitster.dls.corp.google.com","threadId":"35922","inReplyTo":"20140221054148.GA24882@sigill.intra.peff.net","subject":"Re: [PATCH] sha1_file: fix delta_stack memory leak in unpack_entry","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-21T18:09:33Z","receivedAt":"2014-02-21T18:09:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Feb 21, 2014 at 06:47:47AM +0700, Nguyễn Thái Ngọc Duy wrote:\n>\n>> This delta_stack array can grow to any length depending on the actual\n>> delta chain, but we forget to free it. Normally it does not matter\n>> because we use small_delta_stack[] from stack and small_delta_stack\n>> can hold 64-delta chains, more than standard --depth=50 in pack-objects.\n>> \n>> Signed-off-by: Nguyễn Thái Ngọc Duy \n>> ---\n>> Found when trying to see if making some objects loose at this phase\n>> could help git-blame and how many objects will be loosened. Gotta go\n>> soon, didn't really test it, but I bet it'll work.\n>\n> This looks correct to me.\n\nThis comes from abe601bb, right? The change looks correct to me, too.\n\n> For fun, I measured \"git fsck\" on linux.git via massif. The peak memory\n> usage dropped from 368MB to 306MB. That's probably an extreme case\n> because it's a big repo and was packed with \"--aggressive\", but it's\n> still a nice improvement.\n\nThanks.\n"},{"id":"235168","messageId":"20140222081231.GC1576@sigill.intra.peff.net","threadId":"35922","inReplyTo":"xmqqmwhk49sy.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] sha1_file: fix delta_stack memory leak in unpack_entry","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-22T08:12:31Z","receivedAt":"2014-02-22T08:12:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 21, 2014 at 10:09:33AM -0800, Junio C Hamano wrote:\n\n> >> This delta_stack array can grow to any length depending on the actual\n> >> delta chain, but we forget to free it. Normally it does not matter\n> >> because we use small_delta_stack[] from stack and small_delta_stack\n> >> can hold 64-delta chains, more than standard --depth=50 in pack-objects.\n> [...]\n> \n> This comes from abe601bb, right? The change looks correct to me, too.\n\nYes, it does.\n\n-Peff\n"},{"id":"235189","messageId":"87ob1zrw3e.fsf@thomasrast.ch","threadId":"35922","inReplyTo":"xmqqmwhk49sy.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] sha1_file: fix delta_stack memory leak in unpack_entry","fromName":"Thomas Rast","fromEmail":"tr@thomasrast.ch","sentAt":"2014-02-22T15:44:05Z","receivedAt":"2014-02-22T15:44:05Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Junio C Hamano writes:\n\n> Jeff King writes:\n>\n>> On Fri, Feb 21, 2014 at 06:47:47AM +0700, Nguyễn Thái Ngọc Duy wrote:\n>>\n>>> This delta_stack array can grow to any length depending on the actual\n>>> delta chain, but we forget to free it. Normally it does not matter\n>>> because we use small_delta_stack[] from stack and small_delta_stack\n>>> can hold 64-delta chains, more than standard --depth=50 in pack-objects.\n>>> \n>>> Signed-off-by: Nguyễn Thái Ngọc Duy \n>>> ---\n>>> Found when trying to see if making some objects loose at this phase\n>>> could help git-blame and how many objects will be loosened. Gotta go\n>>> soon, didn't really test it, but I bet it'll work.\n>>\n>> This looks correct to me.\n>\n> This comes from abe601bb, right? The change looks correct to me, too.\n\nOw, sorry about that. Thanks for the fix!\n\n-- \nThomas Rast\ntr@thomasrast.ch\n"}]}