{"thread":{"id":"34926","subject":"[PATCH] pack-objects: no crc check when the cached version is used","startedAt":"2013-09-13T11:03:00Z","lastAt":"2013-09-14T03:18:45Z","messageCount":5,"participants":["Nguyễn Thái Ngọc Duy","Junio C Hamano","Thomas Rast","Duy Nguyen","Nicolas Pitre"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"227621","messageId":"1379070180-15947-1-git-send-email-pclouds@gmail.com","threadId":"34926","inReplyTo":null,"subject":"[PATCH] pack-objects: no crc check when the cached version is used","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-09-13T11:03:00Z","receivedAt":"2013-09-13T11:03:00Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Current code makes pack-objects always do check_pack_crc() in\nunpack_entry() even if right after that we find out there's a cached\nversion and pack access is not needed. Swap two code blocks, search\nfor cached version first, then check crc.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n sha1_file.c | 20 ++++++++++----------\n 1 file changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 8c2d1ed..4955724 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2126,6 +2126,16 @@ void *unpack_entry(struct packed_git *p, off_t obj_offset,\n \t\tint i;\n \t\tstruct delta_base_cache_entry *ent;\n \n+\t\tent = get_delta_base_cache_entry(p, curpos);\n+\t\tif (eq_delta_base_cache_entry(ent, p, curpos)) {\n+\t\t\ttype = ent->type;\n+\t\t\tdata = ent->data;\n+\t\t\tsize = ent->size;\n+\t\t\tclear_delta_base_cache_entry(ent);\n+\t\t\tbase_from_cache = 1;\n+\t\t\tbreak;\n+\t\t}\n+\n \t\tif (do_check_packed_object_crc && p->index_version > 1) {\n \t\t\tstruct revindex_entry *revidx = find_pack_revindex(p, obj_offset);\n \t\t\tunsigned long len = revidx[1].offset - obj_offset;\n@@ -2140,16 +2150,6 @@ void *unpack_entry(struct packed_git *p, off_t obj_offset,\n \t\t\t}\n \t\t}\n \n-\t\tent = get_delta_base_cache_entry(p, curpos);\n-\t\tif (eq_delta_base_cache_entry(ent, p, curpos)) {\n-\t\t\ttype = ent->type;\n-\t\t\tdata = ent->data;\n-\t\t\tsize = ent->size;\n-\t\t\tclear_delta_base_cache_entry(ent);\n-\t\t\tbase_from_cache = 1;\n-\t\t\tbreak;\n-\t\t}\n-\n \t\ttype = unpack_object_header(p, &w_curs, &curpos, &size);\n \t\tif (type != OBJ_OFS_DELTA && type != OBJ_REF_DELTA)\n \t\t\tbreak;\n-- \n1.8.2.82.gc24b958\n"},{"id":"227640","messageId":"xmqq7gekk24q.fsf@gitster.dls.corp.google.com","threadId":"34926","inReplyTo":"1379070180-15947-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] pack-objects: no crc check when the cached version is used","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-13T18:28:05Z","receivedAt":"2013-09-13T18:28:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> Current code makes pack-objects always do check_pack_crc() in\n> unpack_entry() even if right after that we find out there's a cached\n> version and pack access is not needed. Swap two code blocks, search\n> for cached version first, then check crc.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n\nInteresting.\n\nThis is only triggered inside pack-objects, which would read a lot\nof data from existing packs, and the overhead for looking up the\nentry from the revindex, faulting in the actual packdata, and\ncomputing and comparing the crc would not be trivial, especially as\nthe cost is incurred over many objects we need to untangle in the\ndelta chain. If you have interesting numbers to show how much this\nimproves the performance, I am curious to see it.\n\nGood spotting ;-)\n\n> sha1_file.c | 20 ++++++++++----------\n> 1 file changed, 10 insertions(+), 10 deletions(-)\n>\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 8c2d1ed..4955724 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -2126,6 +2126,16 @@ void *unpack_entry(struct packed_git *p, off_t obj_offset,\n> \t\tint i;\n> \t\tstruct delta_base_cache_entry *ent;\n> \n> +\t\tent = get_delta_base_cache_entry(p, curpos);\n> +\t\tif (eq_delta_base_cache_entry(ent, p, curpos)) {\n> +\t\t\ttype = ent->type;\n> +\t\t\tdata = ent->data;\n> +\t\t\tsize = ent->size;\n> +\t\t\tclear_delta_base_cache_entry(ent);\n> +\t\t\tbase_from_cache = 1;\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> \t\tif (do_check_packed_object_crc && p->index_version > 1) {\n> \t\t\tstruct revindex_entry *revidx = find_pack_revindex(p, obj_offset);\n> \t\t\tunsigned long len = revidx[1].offset - obj_offset;\n> @@ -2140,16 +2150,6 @@ void *unpack_entry(struct packed_git *p, off_t obj_offset,\n> \t\t\t}\n> \t\t}\n> \n> -\t\tent = get_delta_base_cache_entry(p, curpos);\n> -\t\tif (eq_delta_base_cache_entry(ent, p, curpos)) {\n> -\t\t\ttype = ent->type;\n> -\t\t\tdata = ent->data;\n> -\t\t\tsize = ent->size;\n> -\t\t\tclear_delta_base_cache_entry(ent);\n> -\t\t\tbase_from_cache = 1;\n> -\t\t\tbreak;\n> -\t\t}\n> -\n> \t\ttype = unpack_object_header(p, &w_curs, &curpos, &size);\n> \t\tif (type != OBJ_OFS_DELTA && type != OBJ_REF_DELTA)\n> \t\t\tbreak;\n"},{"id":"227654","messageId":"87k3ikct1e.fsf@inf.ethz.ch","threadId":"34926","inReplyTo":"xmqq7gekk24q.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] pack-objects: no crc check when the cached version is used","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2013-09-13T21:26:21Z","receivedAt":"2013-09-13T21:26:21Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Junio C Hamano writes:\n\n> Nguyễn Thái Ngọc Duy writes:\n>\n>> Current code makes pack-objects always do check_pack_crc() in\n>> unpack_entry() even if right after that we find out there's a cached\n>> version and pack access is not needed. Swap two code blocks, search\n>> for cached version first, then check crc.\n[...]\n>\n> Interesting.\n>\n> This is only triggered inside pack-objects, which would read a lot\n> of data from existing packs, and the overhead for looking up the\n> entry from the revindex, faulting in the actual packdata, and\n> computing and comparing the crc would not be trivial, especially as\n> the cost is incurred over many objects we need to untangle in the\n> delta chain. If you have interesting numbers to show how much this\n> improves the performance, I am curious to see it.\n\nI can't see anything wrong with the patch, but then I haven't stared too\nhard. (It seems that my conversion around abe601b (sha1_file: remove\nrecursion in unpack_entry, 2013-03-27) was faithful on this point, the\nproblem has existed for longer than that.)\n\nI tried the perf script below, but at least for the git repo the only\nthing I can see is noise.\n\n--- 8< --- t/perf/p5300-pack-object.sh --- 8< ---\n#!/bin/sh\n\ntest_description=\"Tests object packing performance\"\n\n. ./perf-lib.sh\n\ntest_perf_default_repo\n\ntest_perf 'pack-objects on commits in HEAD' '\n\tgit rev-list HEAD |\n\tgit pack-objects --stdout >/dev/null\n'\n\ntest_perf 'pack-objects on all of HEAD' '\n\tgit rev-list --objects HEAD |\n\tgit pack-objects --stdout >/dev/null\n'\n\ntest_done\n"},{"id":"227665","messageId":"CACsJy8BctT3C+oW9a5azzFUru89j-NZ-kGK_wgxhsjRZEKnq_g@mail.gmail.com","threadId":"34926","inReplyTo":"87k3ikct1e.fsf@inf.ethz.ch","subject":"Re: [PATCH] pack-objects: no crc check when the cached version is used","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-09-14T01:04:56Z","receivedAt":"2013-09-14T01:04:56Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Sep 14, 2013 at 4:26 AM, Thomas Rast wrote:\n> Junio C Hamano writes:\n>\n>> Nguyễn Thái Ngọc Duy writes:\n>>\n>>> Current code makes pack-objects always do check_pack_crc() in\n>>> unpack_entry() even if right after that we find out there's a cached\n>>> version and pack access is not needed. Swap two code blocks, search\n>>> for cached version first, then check crc.\n> [...]\n>>\n>> Interesting.\n>>\n>> This is only triggered inside pack-objects, which would read a lot\n>> of data from existing packs, and the overhead for looking up the\n>> entry from the revindex, faulting in the actual packdata, and\n>> computing and comparing the crc would not be trivial, especially as\n>> the cost is incurred over many objects we need to untangle in the\n>> delta chain. If you have interesting numbers to show how much this\n>> improves the performance, I am curious to see it.\n\nNo I don't have any timing numbers. I just updated the code to see how\nmany times crc is checked and how many times we find a cached version\nafter crc is checked. The numbers with git.git are 353535 and 113257\nrespectively. IOW we could reduce the number of crc checks by 30%.\n\n>\n> I can't see anything wrong with the patch, but then I haven't stared too\n> hard. (It seems that my conversion around abe601b (sha1_file: remove\n> recursion in unpack_entry, 2013-03-27) was faithful on this point, the\n> problem has existed for longer than that.)\n>\n> I tried the perf script below, but at least for the git repo the only\n> thing I can see is noise.\n\n--stdout does not set do_check_packed_object_crc, you need to run\npack-objects without --stdout (i.e. the real use case is repack)\n\n>\n> --- 8< --- t/perf/p5300-pack-object.sh --- 8< ---\n> #!/bin/sh\n>\n> test_description=\"Tests object packing performance\"\n>\n> . ./perf-lib.sh\n>\n> test_perf_default_repo\n>\n> test_perf 'pack-objects on commits in HEAD' '\n> git rev-list HEAD |\n> git pack-objects --stdout >/dev/null\n> '\n>\n> test_perf 'pack-objects on all of HEAD' '\n> git rev-list --objects HEAD |\n> git pack-objects --stdout >/dev/null\n> '\n>\n> test_done\n\n\n\n-- \nDuy\n"},{"id":"227667","messageId":"alpine.LFD.2.03.1309132302350.20709@syhkavp.arg","threadId":"34926","inReplyTo":"CACsJy8BctT3C+oW9a5azzFUru89j-NZ-kGK_wgxhsjRZEKnq_g@mail.gmail.com","subject":"Re: [PATCH] pack-objects: no crc check when the cached version is used","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2013-09-14T03:18:45Z","receivedAt":"2013-09-14T03:18:45Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Sat, 14 Sep 2013, Duy Nguyen wrote:\n\n> On Sat, Sep 14, 2013 at 4:26 AM, Thomas Rast wrote:\n> > I tried the perf script below, but at least for the git repo the only\n> > thing I can see is noise.\n> \n> --stdout does not set do_check_packed_object_crc, you need to run\n> pack-objects without --stdout (i.e. the real use case is repack)\n\nAnd for those who might wonder why, you can have a look at the \ndescription for commit 0e8189e2708b. This was probably one of my best \ncommit logs ever. ;-)\n\nThis commit also provides a hint about the cost of over-checking the \nCRC.\n\n\nNicolas\n"}]}